Skip to content
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.

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

syscall: ExtraFiles FD used to set controlling TTY instead of SysProcAttr.Ctty field #29458

Open
sipsma opened this issue Dec 29, 2018 · 30 comments

Comments

@sipsma
Copy link

commented Dec 29, 2018

What did you do?

In some corner cases, cmd/exec will start a child process with a controlling tty FD from the ExtraFiles field of Cmd instead of the FD specified in SysProcAttr.Ctty.

This occurs when the FD number of SysProcAttr.Ctty in the parent is 0, 1, 2 or 3+i, where i is an index that has been populated in the ExtraFiles field of the Cmd struct.

This happens because the dup2 loop of forkAndExecInChild1 runs before the ioctl calls to set the Ctty. When making the ioctl calls, it's still is using the Ctty FD value passed in from the parent, so if the child process happens to dup2 over that FD value, the Ctty passed from the parent is closed and the child's ExtraFile FD is used instead of the Ctty FD configured by the parent. This usually results in a ENOTTY error (unless the ExtraFile happens to also be a TTY).

The following reproducing code (compiled with CGo enabled, on Linux w/ glibc) first creates a child process where the bug doesn't occur and then one where the bug does occur. When the bug occurs, forkAndExecInChild1 will incorrectly use the parent's FD 10 (/dev/null) when making the ioctl to setup the ctty even though SysProcAttr configured it to use the parent's FD 11.

package main

import (
        /*
                #include <pty.h>
                #cgo LDFLAGS: -lutil
        */
        "C"

        "fmt"
        "os"
        "os/exec"
        "syscall"
)

func main() {
    // Case with expected behavior.
    // The parent process opens files with descriptors set to:
    // * 5 -> /dev/null
    // * 7 -> PTY (from under /dev/pts/)
    // The child process is configured with the following valid mappings from the parent
    // * 6 in the child -> 5 in the parent (/dev/null)
    // * Ctty in the child -> 7 in the parent (PTY)
    runChildWithCtty(5, 6, 7)

    // Case where the bug occurs (child process fails to start with ENOTTY).
    // The parent process opens files with descriptors set to:
    // * 10 -> /dev/null
    // * 11 -> PTY (from under /dev/pts/)
    // The child process is configured with the following valid mappings from the parent
    // * 11 in the child -> 10 in the parent (/dev/null)
    // * Ctty in the child -> 11 in the parent (PTY)
    runChildWithCtty(10, 11, 11)
}

// Run a child process (arbitrarily /bin/true) with
// * An ExtraFile where the FD is parentExtraFileFdNum in the parent and will be set to childExtraFileFdNum in the child
// * A Ctty where the PTY has FD num set to parentPtyFdNum
func runChildWithCtty(parentExtraFileFdNum int, childExtraFileFdNum int, parentPtyFdNum int) {
        childCmd := exec.Command("/bin/true")
        childCmd.ExtraFiles = make([]*os.File, childExtraFileFdNum-2)

        childCmd.ExtraFiles[childExtraFileFdNum-3] = openNormalFileAtFd(parentExtraFileFdNum)
        childCmd.SysProcAttr = &syscall.SysProcAttr{
                Setsid: true,
                Setctty: true,
                Ctty: openPtyAtFd(parentPtyFdNum),
        }

        err := childCmd.Run()
        if err != nil {
                panic(fmt.Sprintf("failed to run child process with ParentExtraFileFdNum=%d, ChildExtraFileFd=%d, ParentPtyFd=%d: %v", parentExtraFileFdNum, childExtraFileFdNum, parentPtyFdNum, err))
        } else {
                fmt.Printf("successfully ran child process with ParentExtraFileFdNum=%d, ChildExtraFileFd=%d, ParentPtyFd=%d\n\n", parentExtraFileFdNum, childExtraFileFdNum, parentPtyFdNum)
        }
}

// open a pty up and dup2 it to the requested FD num
func openPtyAtFd(wantedFd int) int {
        m := C.int(0)
        s := C.int(0)

        _, err := C.openpty(&m, &s, nil, nil, nil)
        if err != nil {
                panic(fmt.Sprintf("failed to open pty: %v", err))
        }

        goS := int(s)

        if goS != wantedFd {
                err = syscall.Dup2(goS, wantedFd)
                if err != nil {
                        panic(fmt.Sprintf("failed to dup2: %v", err))
                }
                syscall.Close(goS)
        }

        return wantedFd
}

// open /dev/null and dup2 it to the requested FD num
func openNormalFileAtFd(wantedFd int) *os.File {
        f, err := os.Open(os.DevNull)
        if err != nil {
                panic(fmt.Sprintf("failed to open devnull: %v", err))
        }

        actualFd := int(f.Fd())

        if actualFd != wantedFd {
                err = syscall.Dup2(actualFd, wantedFd)
                if err != nil {
                        panic(fmt.Sprintf("failed to dup2: %v", err))
                }
                f.Close()
                return os.NewFile(uintptr(wantedFd), f.Name())
        } else {
                return f
        }
}

This might be in something of a grey area as to whether it's a bug or expected behavior, but I'd consider this a bug because:

  • When I configure the SysProcAttr.Ctty field, I am providing an FD of the parent, so it's very surprising that in some corner cases it ends up instead being a reference to an FD in the child process
  • To workaround this behavior, our codebase in which this was originally encountered now has to jump through a bunch of hoops to check that an FD number of the parent (the pty) never accidentally has the same value of completely unrelated FDs (set in ExtraFiles) that will be set in the child. It seems like the whole point of the interface given by cmd/exec is to make the FD numbers of the parent and child independent of one another and only related by a mapping, saving users from having to think about corner cases of FD inheritance.
    • We don't have direct control over the initial FD values returns by various syscalls, so we end up having to either dup FDs in a loop until they no longer conflict between the Ctty field and ExtraFiles or make the FD numbers of our child process ExtraFiles dynamically configurable. Neither is ideal.

What did you expect to see?

I expected both cases in the reproducing code to work, making the ioctl call to set the ctty using the SysProcAttr field as configured in the parent process.

Running strace -f -b execve -e 'trace=desc' ./main, this is the (filtered) output in the working case:

[pid 15680] openat(AT_FDCWD, "/dev/null", O_RDONLY|O_CLOEXEC) = 3
[pid 15680] dup2(3, 5)                  = 5
[pid 15680] close(3)                    = 0
[pid 15680] open("/dev/ptmx", O_RDWR)   = 3
[pid 15680] close(7)                    = 0
[pid 15680] close(6)                    = 0
[pid 15680] close(6)                    = 0
[pid 15680] open("/dev/pts/13", O_RDWR|O_NOCTTY) = 6
[pid 15680] dup2(6, 7)                  = 7
[pid 15680] close(6)                    = 0
[pid 15680] openat(AT_FDCWD, "/dev/null", O_RDONLY|O_CLOEXEC) = 6
[pid 15680] openat(AT_FDCWD, "/dev/null", O_WRONLY|O_CLOEXEC) = 8
[pid 15680] openat(AT_FDCWD, "/dev/null", O_WRONLY|O_CLOEXEC) = 9
[pid 15680] pipe2([10, 11], O_CLOEXEC)  = 0
Process 15687 attached
[pid 15687] dup2(5, 10)                 = 10
[pid 15687] dup2(6, 0)                  = 0
[pid 15687] dup2(8, 1)                  = 1
[pid 15687] dup2(9, 2)                  = 2
[pid 15687] close(3)                    = 0
[pid 15687] close(4)                    = 0
[pid 15687] close(5)                    = 0
[pid 15687] dup2(10, 6)                 = 6
[pid 15687] ioctl(7, TIOCSCTTY, 1)      = 0

What did you see instead?

In the case where the bug occurs, this is the filtered strace output:

[pid 15680] openat(AT_FDCWD, "/dev/null", O_RDONLY|O_CLOEXEC) = 6
[pid 15680] dup2(6, 10)                 = 10
[pid 15680] close(6)                    = 0
[pid 15680] open("/dev/ptmx", O_RDWR)   = 6
[pid 15680] open("/dev/pts/14", O_RDWR|O_NOCTTY) = 8
[pid 15680] dup2(8, 11)                 = 11
[pid 15680] close(8)                    = 0
[pid 15680] openat(AT_FDCWD, "/dev/null", O_RDONLY|O_CLOEXEC <unfinished ...>
[pid 15680] <... openat resumed> )      = 8
[pid 15680] openat(AT_FDCWD, "/dev/null", O_WRONLY|O_CLOEXEC) = 9
[pid 15680] openat(AT_FDCWD, "/dev/null", O_WRONLY|O_CLOEXEC) = 12
[pid 15680] pipe2([13, 14], O_CLOEXEC)  = 0
Process 15688 attached
[pid 15688] dup2(10, 13)                = 13
[pid 15688] dup2(8, 0)                  = 0
[pid 15688] dup2(9, 1)                  = 1
[pid 15688] dup2(12, 2)                 = 2
[pid 15688] close(3)                    = 0
[pid 15688] close(4)                    = 0
[pid 15688] close(5)                    = 0
[pid 15688] close(6)                    = 0
[pid 15688] close(7)                    = 0
[pid 15688] close(8)                    = 0
[pid 15688] close(9)                    = 0
[pid 15688] close(10)                   = 0
[pid 15688] dup2(13, 11)                = 11
[pid 15688] ioctl(11, TIOCSCTTY, 1)     = -1 ENOTTY (Inappropriate ioctl for device)

Even though the child process does call the ioctl on FD 11 at the end, 11 was previously overwritten by a dup2 call (from 13, which itself was dup2'd from 10, which is a FD configured in ExtraFiles, not Ctty).

So the end effect is that the parent's FD 10 from ExtraFiles was used as the Ctty instead of the parent's FD 11.

Does this issue reproduce with the latest release (go1.11.4)?

Yes

System details

go version go1.11.4 linux/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/sipsma/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/sipsma/go"
GOPROXY=""
GORACE=""
GOROOT="/local/home/sipsma/go"
GOTMPDIR=""
GOTOOLDIR="/local/home/sipsma/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
GOROOT/bin/go version: go version go1.11.4 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.11.4
uname -sr: Linux 4.9.124-0.1.ac.198.73.329.metal1.x86_64
/lib64/libc.so.6: GNU C Library stable release version 2.12, by Roland McGrath et al.
gdb --version: GNU gdb (GDB) Amazon Linux (7.2-50.11.amzn1)

@ianlancetaylor ianlancetaylor changed the title ExtraFiles FD used to set controlling TTY instead of SysProcAttr.Ctty field syscall: ExtraFiles FD used to set controlling TTY instead of SysProcAttr.Ctty field Dec 30, 2018

@ianlancetaylor ianlancetaylor added this to the Go1.13 milestone Dec 30, 2018

@gopherbot

This comment has been minimized.

Copy link

commented May 25, 2019

Change https://golang.org/cl/178919 mentions this issue: syscall: use Ctty before fd shuffle

@gopherbot gopherbot closed this in 103b5b6 May 30, 2019

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

I'm starting to wonder whether this is actually a bug. The change in behavior has broken a couple of existing programs.

Perhaps we should instead clearly define the Ctty field as being the file descriptor number in the child, not the parent. Then the code before CL 178919 would work fine, provided we can always assume that the ctty should be a descriptor that is open in the child. If you wrote your code with that understanding, would that clarify matters?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

Marking as release-blocker to decide whether to keep or revert the change.

@sipsma

This comment has been minimized.

Copy link
Author

commented Jun 21, 2019

@ianlancetaylor I see where you're coming from but still have some concerns.

IIRC The behavior before CL 178919 was that sometimes the parent FD was used and sometimes the child FD was used. So even if you go with the model of Ctty specifying the child FD, I think there would still need to be a different fix to ensure that it's only the child FD that is ever used.

Given the above, I wonder if changing this behavior with that different fix would end up just breaking a different set of existing programs. It seems entirely possible to me that programs out there are using this interface but their parent process and child process always have FDs open in the right pattern such that their child ends up using a parent FD as a Ctty.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

It seems to me that before CL 178919 we always used the child FD. In what circumstances would you say that we used the parent FD?

@sipsma

This comment has been minimized.

Copy link
Author

commented Jun 21, 2019

In my original post, the first test case, runChildWithCtty(5, 6, 7), results in the parent FD 7 being used as the Ctty of the child. FD 7 isn't set in the child process.

The second test case, runChildWithCtty(10, 11, 11) has the behavior you're describing where the child FD is used. I think it's the inconsistency in behavior that's the real bug.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

OK, I'm not sure what happens when the controlling terminal is closed. Is it meaningful to set the ctty of a process and then close that descriptor?

@sipsma

This comment has been minimized.

Copy link
Author

commented Jun 21, 2019

Yeah I believe there's no requirement at least on Linux (not sure about Posix in general) that the child actually keep the FD open.

The original context in which I encountered this bug was a parent process on Linux that managed a bunch of children, each one of which was in its own session with its own Ctty. The parent process kept the Ctty FD open after the child spawned until it wanted the child process's session to end, at which time the parent closed the Ctty (which results in SIGHUP being sent to every process in the child's session). This worked consistently in the "use parent FD as Ctty" case. It was a very rare occurrence that the code would randomly hit the "use child FD as Ctty" case and then things would break.

I can see that others were probably in the reverse situation and relied on the "use child FD as Ctty" case, but it seems like this may be a choice between who you want to break. I'm obviously a bit biased, but the "use parent FD as Ctty" behavior seems more straightforward. You don't need to coordinate between the Ctty and ExtraFiles fields in the parent and the child doesn't need to have an extra FD sitting around it may not have any use for.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

Thanks, that seems persuasive.

@neild

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

This change has broken https://github.com/google/goexpect/, and I'm not certain that package was doing anything wrong.

The goexpect package sets up a SysProcAddr here:

	cmd := exec.Command(command[0], command[1:]...)
	// This ties the commands Stdin,Stdout & Stderr to the virtual terminal we created
	cmd.Stdin, cmd.Stdout, cmd.Stderr = pty.Slave, pty.Slave, pty.Slave
	// New process needs to be the process leader and control of a tty
	cmd.SysProcAttr = &syscall.SysProcAttr{
		Setsid:  true,
		Setctty: true}

SysProcAttr.Ctty is 0. Prior to this change, that sets the controlling terminal to cmd.Stdin (since Ctty refers to the FD in the child). After this change, it sets the controlling terminal to os.Stdin (Ctty refers to the FD in the parent).

Perhaps it would have been best if SysProcAttr.Ctty had always referred to a FD in the parent, but unless I'm missing something changing it breaks working code that wasn't doing anything wrong.

@sipsma

IIRC The behavior before CL 178919 was that sometimes the parent FD was used and sometimes the child FD was used.

I don't think this is correct; from my read the controlling TTY was always set in the child immediately before calling exec.

@neild neild reopened this Jun 25, 2019

@sipsma

This comment has been minimized.

Copy link
Author

commented Jun 25, 2019

@neild I don't think there's any debate that the Ctty was always consistently set in the child. The issue was whether the FD being used to set the Ctty was a reference to an FD opened in the parent or a reference to an FD opened in the child.

The behavior before the fix was inconsistent; if you specified Ctty: 4 (for example), sometimes you would end up referring to what the parent has open at FD 4, sometimes you would end up referring to what the child has opened as FD 4.

Given this inconsistency needs to be fixed and will end up breaking behavior no matter what, my preference is the behavior that results in the clearest+simplest interface and the one with the least extraneous requirements, but that decision is up to the Go maintainers obviously. I'll just also mention that the code you posted would be updated to

cmd.SysProcAttr = &syscall.SysProcAttr{
		Setsid:  true,
		Setctty: true,
                Ctty: pty.Slave}

so while I agree the fix breaks your existing code, the updates required are at least pretty minimal and result in being more explicit (if that's any consolation).

@sipsma

This comment has been minimized.

Copy link
Author

commented Jun 25, 2019

This does bring up the point that it would probably be highly beneficial for the docs to make the behavior very explicit. Right now they are pretty ambiguous as to whether the Ctty FD is a reference to the parent or child, which is part of the source of confusion here: https://golang.org/pkg/syscall/#SysProcAttr

@neild

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

The behavior before the fix was inconsistent; if you specified Ctty: 4 (for example), sometimes you would end up referring to what the parent has open at FD 4, sometimes you would end up referring to what the child has opened as FD 4.

If you specified Ctty: 4, how would you get anything other than the child's FD 4? The controlling terminal is set in the child immediately before calling exec. It is a reference to a FD in the child.

Perhaps your point is that you don't have control over what FD 4 in the child is. But you do have control over FDs 0, 1, and 2, which is precisely the case which is broken here.

@sipsma

This comment has been minimized.

Copy link
Author

commented Jun 25, 2019

Perhaps your point is that you don't have control over what FD 4 in the child is.

Yes, in fact if you are a user just going by the docs, FD 4 is not even supposed to be open in the child (once the final child process is exec'd it will be closed via CLOEXEC). That's why it seemed entirely reasonable to assume that the FD must refer the FD as set in the parent. It wouldn't make sense to rely on a 100% internal implementation detail such as the fact that an FD from the parent still happens to be opened in the child process at a given time.

Again, I agree your code is broken by this change as is, but switching the behavior the other way (make Ctty always refer to child FDs`) is still a breaking change, just for a different set of cases.

@neild

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

Also, your suggested fix is not universally accurate (although it would work here).

Consider existing code:

cmd.Stdin, cmd.Stdout, cmd.Stderr = f0, f1, f2
cmd.SysProcAttr = &syscall.SysProcAttr{
  Setsid:  true,
  Setctty: true,
  Ctty: 0,
}

In Go 1.12 and earlier, this will always and consistently set the child's ctty to f0. (First f0 is shuffled to FD 0, second we set the ctty.)

After the change to redefine Ctty, this needs to be written as:

cmd.Stdin, cmd.Stdout, cmd.Stderr = f0, f1, f2
cmd.SysProcAttr = &syscall.SysProcAttr{
  Setsid:  true,
  Setctty: true,
  Ctty: f0.Fd(),
}

This will also work in Go 1.12 and earlier, unless f0's FD in the parent is 1 or 2. In Go 1.12 we will:

  1. Shuffle f0 to FD 0, setting the original FD of f0 to close-on-exec.
  2. Shuffle f1 and f2 to FDs 1 and 2. If f0's original FD is 1 or 2, it will be clobbered.
  3. Set the ctty to whatever f0's original FD is.

This is perhaps an obscure case, but it indicates that there is no general way to write code which is safe with both the old and new APIs. (Also, it's the exact corner case which motivates this change.)

Again, I agree your code is broken by this change as is, but switching the behavior the other way (make Ctty always refer to child FDs`) is still a breaking change, just for a different set of cases

Prior to this change, it is possible to write code which safely sets the controlling terminal in the child.

After this change, it is very difficult to write such code which is correct with both older and newer Go versions.

@sipsma

This comment has been minimized.

Copy link
Author

commented Jun 25, 2019

After this change, it is very difficult to write such code which is correct with both older and newer Go versions.

Agree, you would need an implementation for go1.12 and older and a separate one for go1.13, which can be done with build tags AFAIK but is nonetheless unfortunate as it's code that's going to need to sit around until you are willing to tell your users that go 1.13+ is a strict requirement.

However, for the other set of users who would be broken by a switch to always using the child FD, they now would need to permanently have code that sets the Ctty FD in ExtraFiles and update their child process to close the file they don't actually need (or accept that another file is going to be opened in the child and possibly be passed down to descendants).

Either way, some set of users of the library are going to end having to include more extraneous code in their codebase because of the previous inconsistent behavior. It's just going to come down to which the maintainers want to impose I suppose.

@neild

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

for users broken by the switch to always using the child FD,

This is not a switch. This is the previous behavior. SysProcAttr.Ctty has always referred to a file descriptor in the child. CL/178919 redefines it, breaking previously correct programs. This change should be reverted.

@sipsma

This comment has been minimized.

Copy link
Author

commented Jun 25, 2019

SysProcAttr.Ctty has always referred to a file descriptor in the child.

But it was inconsistent whether it referred to a file that was actually specified by the user to be opened in the child or it was another FD that temporarily existed due to an internal implementation detail, which is what needed to be fixed.

I've made my views clear and can't personally make a judgement call on which users the maintainers should break. I would just would say that if CL/178919 is reverted, then there still needs to be another fix to make Ctty consistently refer to a FD that is specified in ExtraFiles (which like I said before will just end up breaking another set of existing users and impose extra requirements on them).

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

I hadn't considered the necessity of calling the Fd method, which has side effects. I think it may be better to revert CL 178919, document Ctty as being the child's descriptor number, and accept that there is no way to start a child with a closed ctty descriptor. Perhaps for 1.14 we can return an error if Ctty is set to a descriptor number that does not match Files.

@sipsma

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

@ianlancetaylor I can understand (though respectfully disagree with) reverting CL 178919 and replacing it with a change that returns an error in the case where ExtraFiles doesn't have the Ctty being specified.

However, only reverting it and not fixing the inconsistent behavior (one way or another) concerns me as a user of the library. For me, this appeared as a rare bug that just caused an error, but there are other scenarios in which this inconsistent behavior is worse.

Say a user of the library meant to set a pty file as FD 3 via ExtraFiles and set Ctty: 3, but they made a mistake and forgot to append it or accidentally provided nil to ExtraFiles. Without any fix, the child will now subtly end up using the parent's FD 3. If that FD is a tty for something else, what should have just been an error instead succeeds and results in a child process running with a random other controlling TTY that just happened to also be open at the time in the parent. The implications here run the gambit from just creating an extremely hard to diagnose bug to potential security concerns. This really seems like something that should be addressed in the code, not just docs, if at all possible before 1.14.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

I think this change has proven to be fragile, breaking various existing packages, and I think it's too late in the 1.13 release cycle to mess with it. That is why I suggest fixing it in 1.14. While I agree that the current behavior has its problems, at least 1.13 won't be any worse than any previous release.

@sipsma

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

Okay, that's your call, then I'd just ask that it's clearly documented that it's Undefined Behavior if you set Ctty to an FD that was not set in Stdin/Stdout/Stderr or ExtraData (might be an error, might succeed with a random other FD from the parent).

@gopherbot

This comment has been minimized.

Copy link

commented Jun 26, 2019

Change https://golang.org/cl/183939 mentions this issue: Revert "syscall: use Ctty before fd shuffle"

@neild

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

For 1.14, I'd suggest:

  • Document Ctty as referring to an FD in the child.
  • Return an error from os.StartProcess when ProcAttr.Sys.Ctty >= len(ProcAttr.Files). This may break some existing programs, but only ones relying on an undocumented internal implementation detail; the value in catching subtle bugs is probably worth it.
  • Maybe: If we want to support setting the ctty to an FD which is closed in the child, then add a field to syscall.SysProcAttr that allows specifying the FD in the parent; perhaps a bool indicating that Ctty is an FD in the parent. Alternatively, say that we don't support that and you need to pass the ctty FD to the child.

gvisor-bot added a commit to google/gvisor that referenced this issue Jun 26, 2019

Always set SysProcAttr.Ctty to an FD in the child's FD table.
Go was going to change the behavior of SysProcAttr.Ctty such that it must be an
FD in the *parent* FD table:
https://go-review.googlesource.com/c/go/+/178919/

However, after some debate, it was decided that this change was too
backwards-incompatible, and so it was reverted.
golang/go#29458

The behavior going forward is unchanged: the Ctty FD must be an FD in the
*child* FD table.

PiperOrigin-RevId: 255214544

gvisor-bot added a commit to google/gvisor that referenced this issue Jun 26, 2019

Always set SysProcAttr.Ctty to an FD in the child's FD table.
Go was going to change the behavior of SysProcAttr.Ctty such that it must be an
FD in the *parent* FD table:
https://go-review.googlesource.com/c/go/+/178919/

However, after some debate, it was decided that this change was too
backwards-incompatible, and so it was reverted.
golang/go#29458

The behavior going forward is unchanged: the Ctty FD must be an FD in the
*child* FD table.

PiperOrigin-RevId: 255214544
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

We should make doc changes and add a test for 1.13. Functionality changes like returning an error should wait for 1.14.

gvisor-bot added a commit to google/gvisor that referenced this issue Jun 26, 2019

Always set SysProcAttr.Ctty to an FD in the child's FD table.
Go was going to change the behavior of SysProcAttr.Ctty such that it must be an
FD in the *parent* FD table:
https://go-review.googlesource.com/c/go/+/178919/

However, after some debate, it was decided that this change was too
backwards-incompatible, and so it was reverted.
golang/go#29458

The behavior going forward is unchanged: the Ctty FD must be an FD in the
*child* FD table.

PiperOrigin-RevId: 255228476
@gthelen

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

I assume eventually we want to fix parent-FD assumptions in:

  1. golang's src/os/signal/signal_cgo_test.go:
 	cmd.SysProcAttr = &syscall.SysProcAttr{
 		Setsid:  true,
 		Setctty: true,
-		Ctty:    int(slave.Fd()),
 	}
  1. https://github.com/u-root/u-root/blob/a1fc735dba9d6ede16612b9c042338b24b618e8b/cmds/exp/rush/rush.go#L101

  2. https://github.com/kr/pty/blob/b6e1bdd4a4f88614e0c6e5e8089c7abed98aae17/run.go#L50

gopherbot pushed a commit that referenced this issue Jun 27, 2019

Revert "syscall: use Ctty before fd shuffle"
This reverts commit 103b5b6.

Reason for revert: Breaks valid existing programs.

Updates #29458

Change-Id: I7ace4ae404cf2a8b0e15e646663c50115f74b758
Reviewed-on: https://go-review.googlesource.com/c/go/+/183939
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Greg Thelen <gthelen@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

I just noticed that if the SysProcAttr.Foreground field is set, then all three implementations look at .Ctty before adjusting the file descriptors. And, as noted above, if SysProcAttr.Setctty is set then they look at .Ctty after adjusting the file descriptors. Argh.

@gopherbot

This comment has been minimized.

Copy link

commented Jul 8, 2019

Change https://golang.org/cl/185242 mentions this issue: syscall: document that Ctty is a child descriptor

@sipsma

This comment has been minimized.

Copy link
Author

commented Jul 9, 2019

I just noticed that if the SysProcAttr.Foreground field is set, then all three implementations look at .Ctty before adjusting the file descriptors. And, as noted above, if SysProcAttr.Setctty is set then they look at .Ctty after adjusting the file descriptors. Argh.

So in the case that Foreground is set to true, Ctty is always a reference to the file as set in the Parent's FD table? Yeah, that definitely worsens the inconsistency quite a bit (and makes it much harder to accurately document what Ctty actually means).

Looking around, it looks like a golang unit test currently relies on that behavior. The u-root code @gthelen linked to before also appears to have had this assumption for a while.

Given the whole previous discussion plus this new finding, is it in the cards to just leave all the current behavior as is for the sake of backwards compatibility but in Go 1.14+ deprecate the fields related to Ctty (or perhaps SysProcAttr as a whole) and introduce a new interface that has more consistent and flexible behavior?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Yeah, I don't think there is anything we can do in the 1.13 timeframe here. Even the doc change is wrong.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.13, Go1.14 Jul 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.