Skip to content

pty: add GetPtyFromFile as safer GetPty #86

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 20, 2025
Merged

pty: add GetPtyFromFile as safer GetPty #86

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9dd67e1
gha: bump containerd/project-checks to v1.2.2
cyphar May 19, 2025
12ba745
tc: make internal handlers take File interface
cyphar May 19, 2025
c79e45e
pty: add GetPtyFromFile as safer GetPty
cyphar May 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ jobs:
go install github.com/golangci/golangci-lint/cmd/[email protected]

- name: Project Checks
uses: containerd/project-checks@v1.1.0
uses: containerd/project-checks@v1.2.2
with:
working-directory: src/github.com/containerd/console

Expand Down
19 changes: 17 additions & 2 deletions console_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ func TestWinSize(t *testing.T) {
}
}

func TestConsolePty(t *testing.T) {
console, slavePath, err := NewPty()
func testConsolePty(t *testing.T, newPty func() (Console, string, error)) {
console, slavePath, err := newPty()
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -100,3 +100,18 @@ func TestConsolePty(t *testing.T) {
t.Errorf("unexpected output %q", out)
}
}

func TestConsolePty_NewPty(t *testing.T) {
testConsolePty(t, NewPty)
}

func TestConsolePty_NewPtyFromFile(t *testing.T) {
testConsolePty(t, func() (Console, string, error) {
// Equivalent to NewPty().
f, err := openpt()
if err != nil {
return nil, "", err
}
return NewPtyFromFile(f)
})
}
9 changes: 9 additions & 0 deletions console_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,15 @@ func NewPty() (Console, string, error) {
if err != nil {
return nil, "", err
}
return NewPtyFromFile(f)
}

// NewPtyFromFile creates a new pty pair, just like [NewPty] except that the
// provided [os.File] is used as the master rather than automatically creating
// a new master from /dev/ptmx. The ownership of [os.File] is passed to the
// returned [Console], so the caller must be careful to not call Close on the
// underlying file.
func NewPtyFromFile(f File) (Console, string, error) {
slave, err := ptsname(f)
if err != nil {
return nil, "", err
Expand Down
5 changes: 2 additions & 3 deletions tc_darwin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package console

import (
"fmt"
"os"

"golang.org/x/sys/unix"
)
Expand All @@ -30,12 +29,12 @@ const (

// unlockpt unlocks the slave pseudoterminal device corresponding to the master pseudoterminal referred to by f.
// unlockpt should be called before opening the slave side of a pty.
func unlockpt(f *os.File) error {
func unlockpt(f File) error {
return unix.IoctlSetPointerInt(int(f.Fd()), unix.TIOCPTYUNLK, 0)
}

// ptsname retrieves the name of the first available pts for the given master.
func ptsname(f *os.File) (string, error) {
func ptsname(f File) (string, error) {
n, err := unix.IoctlGetInt(int(f.Fd()), unix.TIOCPTYGNAME)
if err != nil {
return "", err
Expand Down
5 changes: 2 additions & 3 deletions tc_freebsd_cgo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ package console

import (
"fmt"
"os"

"golang.org/x/sys/unix"
)
Expand All @@ -39,7 +38,7 @@ const (

// unlockpt unlocks the slave pseudoterminal device corresponding to the master pseudoterminal referred to by f.
// unlockpt should be called before opening the slave side of a pty.
func unlockpt(f *os.File) error {
func unlockpt(f File) error {
fd := C.int(f.Fd())
if _, err := C.unlockpt(fd); err != nil {
C.close(fd)
Expand All @@ -49,7 +48,7 @@ func unlockpt(f *os.File) error {
}

// ptsname retrieves the name of the first available pts for the given master.
func ptsname(f *os.File) (string, error) {
func ptsname(f File) (string, error) {
n, err := unix.IoctlGetInt(int(f.Fd()), unix.TIOCGPTN)
if err != nil {
return "", err
Expand Down
5 changes: 2 additions & 3 deletions tc_freebsd_nocgo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ package console

import (
"fmt"
"os"

"golang.org/x/sys/unix"
)
Expand All @@ -42,12 +41,12 @@ const (

// unlockpt unlocks the slave pseudoterminal device corresponding to the master pseudoterminal referred to by f.
// unlockpt should be called before opening the slave side of a pty.
func unlockpt(f *os.File) error {
func unlockpt(f File) error {
panic("unlockpt() support requires cgo.")
}

// ptsname retrieves the name of the first available pts for the given master.
func ptsname(f *os.File) (string, error) {
func ptsname(f File) (string, error) {
n, err := unix.IoctlGetInt(int(f.Fd()), unix.TIOCGPTN)
if err != nil {
return "", err
Expand Down
5 changes: 2 additions & 3 deletions tc_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package console

import (
"fmt"
"os"
"unsafe"

"golang.org/x/sys/unix"
Expand All @@ -31,7 +30,7 @@ const (

// unlockpt unlocks the slave pseudoterminal device corresponding to the master pseudoterminal referred to by f.
// unlockpt should be called before opening the slave side of a pty.
func unlockpt(f *os.File) error {
func unlockpt(f File) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we can rather drop the File interface and just use *os.File, assuming that it is the only implementation of the interface

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm it was introduced due to the GC stuff

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is an remarkably common issue to run into with *os.File. It's a shame there isn't a properly supported way of doing this other than hacking around with runtime.SetFinalizer which is (imho) incredibly dodgy.

To be fair, even Rust only really solved this issue in 1.63 with OwnedFd and BorrowedFd...

var u int32
// XXX do not use unix.IoctlSetPointerInt here, see commit dbd69c59b81.
if _, _, err := unix.Syscall(unix.SYS_IOCTL, f.Fd(), unix.TIOCSPTLCK, uintptr(unsafe.Pointer(&u))); err != 0 {
Expand All @@ -41,7 +40,7 @@ func unlockpt(f *os.File) error {
}

// ptsname retrieves the name of the first available pts for the given master.
func ptsname(f *os.File) (string, error) {
func ptsname(f File) (string, error) {
var u uint32
// XXX do not use unix.IoctlGetInt here, see commit dbd69c59b81.
if _, _, err := unix.Syscall(unix.SYS_IOCTL, f.Fd(), unix.TIOCGPTN, uintptr(unsafe.Pointer(&u))); err != 0 {
Expand Down
5 changes: 2 additions & 3 deletions tc_netbsd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package console

import (
"bytes"
"os"

"golang.org/x/sys/unix"
)
Expand All @@ -31,12 +30,12 @@ const (
// unlockpt unlocks the slave pseudoterminal device corresponding to the master pseudoterminal referred to by f.
// unlockpt should be called before opening the slave side of a pty.
// This does not exist on NetBSD, it does not allocate controlling terminals on open
func unlockpt(f *os.File) error {
func unlockpt(f File) error {
return nil
}

// ptsname retrieves the name of the first available pts for the given master.
func ptsname(f *os.File) (string, error) {
func ptsname(f File) (string, error) {
ptm, err := unix.IoctlGetPtmget(int(f.Fd()), unix.TIOCPTSNAME)
if err != nil {
return "", err
Expand Down
6 changes: 2 additions & 4 deletions tc_openbsd_cgo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@
package console

import (
"os"

"golang.org/x/sys/unix"
)

Expand All @@ -34,7 +32,7 @@ const (
)

// ptsname retrieves the name of the first available pts for the given master.
func ptsname(f *os.File) (string, error) {
func ptsname(f File) (string, error) {
ptspath, err := C.ptsname(C.int(f.Fd()))
if err != nil {
return "", err
Expand All @@ -44,7 +42,7 @@ func ptsname(f *os.File) (string, error) {

// unlockpt unlocks the slave pseudoterminal device corresponding to the master pseudoterminal referred to by f.
// unlockpt should be called before opening the slave side of a pty.
func unlockpt(f *os.File) error {
func unlockpt(f File) error {
if _, err := C.grantpt(C.int(f.Fd())); err != nil {
return err
}
Expand Down
6 changes: 2 additions & 4 deletions tc_openbsd_nocgo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@
package console

import (
"os"

"golang.org/x/sys/unix"
)

Expand All @@ -39,10 +37,10 @@ const (
cmdTcSet = unix.TIOCSETA
)

func ptsname(f *os.File) (string, error) {
func ptsname(f File) (string, error) {
panic("ptsname() support requires cgo.")
}

func unlockpt(f *os.File) error {
func unlockpt(f File) error {
panic("unlockpt() support requires cgo.")
}
5 changes: 2 additions & 3 deletions tc_zos.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package console

import (
"os"
"strings"

"golang.org/x/sys/unix"
Expand All @@ -29,11 +28,11 @@ const (
)

// unlockpt is a no-op on zos.
func unlockpt(_ *os.File) error {
func unlockpt(File) error {
return nil
}

// ptsname retrieves the name of the first available pts for the given master.
func ptsname(f *os.File) (string, error) {
func ptsname(f File) (string, error) {
return "/dev/ttyp" + strings.TrimPrefix(f.Name(), "/dev/ptyp"), nil
}