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

os/exec: (*SysProcAttr).Foreground causes the spawned process to hang #37217

Closed
miquella opened this issue Feb 13, 2020 · 4 comments
Closed

os/exec: (*SysProcAttr).Foreground causes the spawned process to hang #37217

miquella opened this issue Feb 13, 2020 · 4 comments
Assignees
Milestone

Comments

@miquella
Copy link

@miquella miquella commented Feb 13, 2020

What version of Go are you using (go version)?

$ go version
go version go1.13.6 linux/amd64

Does this issue reproduce with the latest release?

Yes — tried with go1.14rc1

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/markse/.cache/go-build"
GOENV="/home/markse/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/markse"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build088413182=/tmp/go-build -gno-record-gcc-switches"

What did you do?

package main

import (
    "os"
    "os/exec"
    // "os/signal"
    "syscall"
)

func main() {
    // This does make it work, but shouldn't be necessary and causes side-effects
    // signal.Ignore(syscall.SIGTTIN, syscall.SIGTTOU)

    cmd := exec.Command("echo", "hello")
    cmd.SysProcAttr = &syscall.SysProcAttr{
        Foreground: true,
    }
    cmd.Stdin, cmd.Stdout, cmd.Stderr = os.Stdin, os.Stdout, os.Stderr
    cmd.Run()
}

What did you expect to see?

The process should execute echo and display hello.

What did you see instead?

The process hangs.

ps shows the child process as suspended and doesn't appear to have called execve yet:

$ ps j
 PPID   PID  PGID   SID TTY      TPGID STAT   UID   TIME COMMAND

 6072 25553 25553  6072 pts/0    25553 Dl+   1000   0:00 ./foreground
25553 25558 25558  6072 pts/0    25553 T     1000   0:00 ./foreground

Explanation

From what we can determine, we believe this is what is happening:

  1. The process is forked (ref)
  2. The child calls setpgid/SYS_SETPGID, creating a new background process group (ref)
  3. The child calls tcsetpgrp/TIOCSPGRP, causing the process to be suspended (ref)

While a process in the foreground process group is allowed to call tcsetpgrp, a process in a background process group has an additional hurdle. From the man page for tcsetpgrp:

If tcsetpgrp() is called by a member of a background process group in its session, and the calling process is not blocking or ignoring SIGTTOU, a SIGTTOU signal is sent to all members of this background process group.

Which explains why the process is getting suspended, as the default action for SIGTTOU is to stop the process. This also explains when ignoring SIGTTOU before spawning the process fixes the problem. However, this has an undesired side-effect: the subsequently exec'd process then ignores SIGTTOU as well.

Conclusion

Unfortunately, because all of this work is done in the child fork, there is no way for Go user code to reset the ignored SIGTTOU before exec'ing the process.

So there is no way to have a user ignore the SIGTTOU signal without an adverse effect on the child process.

Proposal: if the SIGTTOU signal could be ignored for the duration of the tcsetpgrp/TIOCSPGRP call and restored to its prior state immediately following, we believe this would elicit the expected behavior without adverse side-effects.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented May 23, 2020

Thank you for this bug report @miquella, and apologies that we didn't get to look at this during the Go1.15 cycle. I shall re-triage this issue for Go1.16.

@odeke-em odeke-em modified the milestones: Go1.15, Go1.16 May 23, 2020
@odeke-em odeke-em self-assigned this May 23, 2020
pupper68k added a commit to pupper68k/go that referenced this issue Oct 28, 2020
Currently we call tcsetpgrp from child thread. This causes bugs like Github issue golang#37217.
We can instead call it from the parent thread, since the child pgrp is either found in the SysProcAttr or equal to the child pid.
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 28, 2020

Change https://golang.org/cl/266057 mentions this issue: syscall: call tcsetpgrp from parent thread when spawning a foreground child process

@odeke-em odeke-em modified the milestones: Go1.16, Go1.17 Dec 25, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 27, 2021

Change https://golang.org/cl/313653 mentions this issue: syscall: restore signal mask after setting foreground process group

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 27, 2021

Change https://golang.org/cl/314271 mentions this issue: syscall: move TestForegroundSignal create call out of goroutine

gopherbot pushed a commit that referenced this issue Apr 27, 2021
That way the skip takes effect.

Also ignore the result of calling TIOCSPGRP when cleaing up TestForeground.
It has started to fail for some reason, and the result doesn't matter.

Also call TIOCSPGRP to clean up in TestForegroundSignal.

For #37217

Change-Id: I2e4282d7d91ad9a198eeb12cef01c2214c2a98c2
Reviewed-on: https://go-review.googlesource.com/c/go/+/314271
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants