syscall: if Setctty, require that Ctty be a child descriptor

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 #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>

diff --git a/doc/go1.15.html b/doc/go1.15.html
index 977c2815ac517290a691fe3181488dd18c510fa3..af0b3ba6ac1bf96b8e32701617d4e95d7cfb61e7 100644 (file)
--- a/doc/go1.15.html
+++ b/doc/go1.15.html
@@ -351,6 +351,28 @@ TODO
     </p>
 </dl><!-- sync -->
 
+<dl id="syscall"><dt><a href="/pkg/syscall/">syscall</a></dt>
+  <dd>
+    <p><!-- CL 231638 -->
+      On Unix systems, functions that use
+      <a href="/pkg/syscall/#SysProcAttr"><code>SysProcAttr</code></a>
+      will now reject attempts to set both the <code>Setctty</code>
+      and <code>Foreground</code> fields, as they both use
+      the <code>Ctty</code> field but do so in incompatible ways.
+      We expect that few existing programs set both fields.
+    </p>
+    <p>
+      Setting the <code>Setctty</code> field now requires that the
+      <code>Ctty</code> field be set to a file descriptor number in the
+      child process, as determined by the <code>ProcAttr.Files</code> field.
+      Using a child descriptor always worked, but there were certain
+      cases where using a parent file descriptor also happened to work.
+      Some programs that set <code>Setctty</code> will need to change
+      the value of <code>Ctty</code> to use a child descriptor number.
+    </p>
+  </dd>
+</dl>
+
 <dl id="testing"><dt><a href="/pkg/testing/">testing</a></dt>
   <dd>
     <p><!-- CL 226877, golang.org/issue/35998 -->

diff --git a/src/os/signal/signal_cgo_test.go b/src/os/signal/signal_cgo_test.go
index 3c23090489f953dd89260208eccf12a864a1e9cc..849a96ec0ec167b2722b32d7c3edb6004a7decf8 100644 (file)
--- a/src/os/signal/signal_cgo_test.go
+++ b/src/os/signal/signal_cgo_test.go
@@ -98,7 +98,7 @@ func TestTerminalSignal(t *testing.T) {
        cmd.SysProcAttr = &syscall.SysProcAttr{
                Setsid:  true,
                Setctty: true,
-               Ctty:    int(slave.Fd()),
+               Ctty:    0,
        }
 
        if err := cmd.Start(); err != nil {

diff --git a/src/syscall/exec_unix.go b/src/syscall/exec_unix.go
index b3798b6e04e55811c0ba5a01325561246737d5f4..0345af44f9e53f9b82bcd738caadab2bbf746ed4 100644 (file)
--- a/src/syscall/exec_unix.go
+++ b/src/syscall/exec_unix.go
@@ -9,6 +9,7 @@
 package syscall
 
 import (
+       errorspkg "errors"
        "internal/bytealg"
        "runtime"
        "sync"
@@ -187,6 +188,15 @@ func forkExec(argv0 string, argv []string, attr *ProcAttr) (pid int, err error)
                }
        }
 
+       // Both Setctty and Foreground use the Ctty field,
+       // but they give it slightly different meanings.
+       if sys.Setctty && sys.Foreground {
+               return 0, errorspkg.New("both Setctty and Foreground set in SysProcAttr")
+       }
+       if sys.Setctty && sys.Ctty >= len(attr.Files) {
+               return 0, errorspkg.New("Setctty set but Ctty not valid in child")
+       }
+
        // Acquire the fork lock so that no other threads
        // create new fds that are not yet close-on-exec
        // before we fork.

diff --git a/src/syscall/exec_unix_test.go b/src/syscall/exec_unix_test.go
index 33614f5221233e4533818d117951355d69e99f11..4eb3c5c6c8a691f5dee1dd60bec2dbcd882122bb 100644 (file)
--- a/src/syscall/exec_unix_test.go
+++ b/src/syscall/exec_unix_test.go
@@ -213,3 +213,31 @@ func TestForeground(t *testing.T) {
 
        signal.Reset()
 }
+
+// Test a couple of cases that SysProcAttr can't handle. Issue 29458.
+func TestInvalidExec(t *testing.T) {
+       t.Parallel()
+       t.Run("SetCtty-Foreground", func(t *testing.T) {
+               t.Parallel()
+               cmd := create(t)
+               cmd.proc.SysProcAttr = &syscall.SysProcAttr{
+                       Setctty:    true,
+                       Foreground: true,
+                       Ctty:       0,
+               }
+               if err := cmd.proc.Start(); err == nil {
+                       t.Error("expected error setting both SetCtty and Foreground")
+               }
+       })
+       t.Run("invalid-Ctty", func(t *testing.T) {
+               t.Parallel()
+               cmd := create(t)
+               cmd.proc.SysProcAttr = &syscall.SysProcAttr{
+                       Setctty: true,
+                       Ctty:    3,
+               }
+               if err := cmd.proc.Start(); err == nil {
+                       t.Error("expected error with invalid Ctty value")
+               }
+       })
+}