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
Comments
Change https://golang.org/cl/178919 mentions this issue: |
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 |
Marking as release-blocker to decide whether to keep or revert the change. |
@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. |
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? |
In my original post, the first test case, The second test case, |
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? |
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. |
Thanks, that seems persuasive. |
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}
Perhaps it would have been best if
I don't think this is correct; from my read the controlling TTY was always set in the child immediately before calling |
@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 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
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). |
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 |
If you specified 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. |
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 Again, I agree your code is broken by this change as is, but switching the behavior the other way (make |
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 After the change to redefine 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
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.)
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. |
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. |
This is not a switch. This is the previous behavior. |
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). |
I hadn't considered the necessity of calling the |
@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 |
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. |
It seems that this change breaks the following patch to github.com/creack/pty: Which is necessary to support interactive programs which open |
@ddevault If Right now the creack code is setting |
It looks like creack was able to put together this fix: creack/pty#97 |
OK, looks like that version works because it leaves the |
The Ctty field is a child descriptor number when Setctty is set, but a parent descriptor when Foreground is set. This is absurd but changing either behavior breaks existing programs. With this change we at least document how it works. For golang#29458 Change-Id: If9cf0a1a1e6ed0d4a4edae5043016d5b4ee3308b Reviewed-on: https://go-review.googlesource.com/c/go/+/229768 Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Damien Neil <dneil@google.com>
Ctty was always handled as a child descriptor, but in some cases passing a parent descriptor would also work. This depended on unpredictable details of the implementation. Reject those cases to avoid confusion. Also reject setting both Setctty and Foreground, as they use Ctty in incompatible ways. It's unlikely that any programs set both fields, as they don't make sense together. Fixes golang#29458 Change-Id: Ieba2d625711fd4b82c8e65e1feed02fd1fb25e6d Reviewed-on: https://go-review.googlesource.com/c/go/+/231638 Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Damien Neil <dneil@google.com>
The fix is needed to mitigate consequences of golang/go#29458 "golang breaking change" that causes following test failures on etcd end: --- FAIL: TestCtlV2Set (0.00s) ctl_v2_test.go:552: could not start etcd process cluster (fork/exec ../../bin/etcd: Setctty set but Ctty not valid in child) --- FAIL: TestCtlV2SetQuorum (0.00s) ctl_v2_test.go:552: could not start etcd process cluster (fork/exec ../../bin/etcd: Setctty set but Ctty not valid in child) --- FAIL: TestCtlV2SetClientTLS (0.00s) ctl_v2_test.go:552: could not start etcd process cluster (fork/exec ../../bin/etcd: Setctty set but Ctty not valid in child)
The fix is needed to mitigate consequences of golang/go#29458 "golang breaking change" that causes following test failures on etcd end: --- FAIL: TestCtlV2Set (0.00s) ctl_v2_test.go:552: could not start etcd process cluster (fork/exec ../../bin/etcd: Setctty set but Ctty not valid in child) --- FAIL: TestCtlV2SetQuorum (0.00s) ctl_v2_test.go:552: could not start etcd process cluster (fork/exec ../../bin/etcd: Setctty set but Ctty not valid in child) --- FAIL: TestCtlV2SetClientTLS (0.00s) ctl_v2_test.go:552: could not start etcd process cluster (fork/exec ../../bin/etcd: Setctty set but Ctty not valid in child)
The fix is needed to mitigate consequences of golang/go#29458 "golang breaking change" that causes following test failures on etcd end: --- FAIL: TestCtlV2Set (0.00s) ctl_v2_test.go:552: could not start etcd process cluster (fork/exec ../../bin/etcd: Setctty set but Ctty not valid in child) --- FAIL: TestCtlV2SetQuorum (0.00s) ctl_v2_test.go:552: could not start etcd process cluster (fork/exec ../../bin/etcd: Setctty set but Ctty not valid in child) --- FAIL: TestCtlV2SetClientTLS (0.00s) ctl_v2_test.go:552: could not start etcd process cluster (fork/exec ../../bin/etcd: Setctty set but Ctty not valid in child)
See golang/go#29458 and https://go-review.googlesource.com/c/go/+/231638. Go 1.15 added a check for a child spawning bug, where ctty was set to a wrong file descriptor. `Ctty` in `SysProcAttr` should be set to a file descriptor _in a child process_, but we were setting it to a file descriptor _in a parent process_. This was working by accident, but is now caught by `os/exec`. Instead, leave it as 0, which will default it to the file descriptor of stdin. In our case, stdin is already set to the tty.
See golang/go#29458 and https://go-review.googlesource.com/c/go/+/231638. Go 1.15 added a check for a child spawning bug, where ctty was set to a wrong file descriptor. `Ctty` in `SysProcAttr` should be set to a file descriptor _in a child process_, but we were setting it to a file descriptor _in a parent process_. This was working by accident, but is now caught by `os/exec`. Instead, leave it as 0, which will default it to the file descriptor of stdin. In our case, stdin is already set to the tty.
`Ctty` in `SysProcAttr` should be set to a file descriptor _in a child process_, but we were setting it to a file descriptor _in a parent process_. This was working by accident, but is now caught by `os/exec`. Instead, leave it as 0, which will default it to the fd of stdin. In our case, stdin is already set to the tty. See: golang/go#29458
`Ctty` in `SysProcAttr` should be set to a fd in a child process, but we were setting it to a fd in the parent process. This was working by accident, but now a panic is raised by `os/exec`. See: golang/go#29458
`Ctty` in `SysProcAttr` should be set to a fd in a child process, but we were setting it to a fd in the parent process. This was working by accident, but now a panic is raised by `os/exec`. See: golang/go#29458
Starting with golang 1.5, setting Ctty value result in `Setctty set but Ctty not valid in child` error, as part of golang/go#29458 . This commit lifts the fix in creack/pty#97 .
Upgrade to golang 1.15 Starting with golang 1.5, setting Ctty value result in `Setctty set but Ctty not valid in child` error, as part of golang/go#29458 . This commit lifts the fix in creack/pty#97 .
Upgrade to golang 1.15 Starting with golang 1.5, setting Ctty value result in `Setctty set but Ctty not valid in child` error, as part of golang/go#29458 . This commit lifts the fix in creack/pty#97 .
Fix tty issue in go 1.15.x golang/go#29458 creack/pty#96
What did you do?
In some corner cases,
cmd/exec
will start a child process with a controlling tty FD from theExtraFiles
field of Cmd instead of the FD specified inSysProcAttr.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 theExtraFiles
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, theCtty
passed from the parent is closed and the child'sExtraFile
FD is used instead of theCtty
FD configured by the parent. This usually results in aENOTTY
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 thoughSysProcAttr
configured it to use the parent's FD 11.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:
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 processExtraFiles
) that will be set in the child. It seems like the whole point of the interface given bycmd/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.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:What did you see instead?
In the case where the bug occurs, this is the filtered strace output:
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
, notCtty
).So the end effect is that the parent's FD 10 from
ExtraFiles
was used as theCtty
instead of the parent's FD 11.Does this issue reproduce with the latest release (go1.11.4)?
Yes
System details
The text was updated successfully, but these errors were encountered: