CC @AndrewGMorgan

1. Does it make a difference if you remove the call to runtime.LockOSThread?
2. You are doing a lot of operations in makeCommand that are normally done by setting fields in cmd.SysProcAttr. That is, instead of calling syscall.Setgroups, set cmd.SysProcAttr.Credential.Groups. To be clear, there may still be a bug here as syscall.Setgroups shouldn't hang, but it appears that the code would be better written if it used the SysProcAttr fields.

Assign this to me. I'll try to figure out what is going on.

First, go1.16 was buggy. The first version that had no known issues with things like runtime.AllThreadsSyscall() and/or the corresponding cgo redirected use of C.setgroups() was go1.16.4.

I'll have to figure out the docker thing - debugging in that environment might take some setup on my part.

Based on the go env output CGO_ENABLED=1 which implies that the runtime.AllThreadsSyscall() support is not being used in this case. Instead, syscall.Setgroups() is being redirected to the C.setgroups() function in go1.16+.

In that and all prior versions of Go, calling C.setgroups() was observed to periodically hang (#42494), later in the go1.16 minor releases, we also found some other hang issues when interrupt handling was in use: #43149 and #44193 discuss aspects of this. This code appears to have both signal handling and the implicit use of the C.setgroups() function. As noted above, I would not expect this to work reliably before go1.16.4.

@ianlancetaylor gives the best advice for restructuring this code: that the SysProcAttr configuration for the cmd is really the way to execute the program with different credentials from the server. Setting up process credentials is really tricky with Go's runtime. I think your best bet is to go with Ian's advice.

As to the code as it stands, it looks like this code is trying to work with two models of privilege at the same time.

• One is it uses its own Setuid() things in server/util.go, which are OS thread specific and will only work reliably if the running thread is runtime.LockOSThread()ed.
• The other model is using syscall.Setgroups() which, is not thread specific but process-shared (changes the groups of all threads in the runtime).

If you really want to roll your own with Raw system calls, I think you would do well to write your own Setgroups() function so it only affects the running thread. So far as I am aware, things like syscall.Setrlimit() and os.Chdir() are process-wide so even in these cases, you will also have some interesting process/thread interactions.

Based on the go env output CGO_ENABLED=1

I'm sorry that I haven't set CGO_ENABLED=0 for this go env command. This problem won't happen while CGO_ENABLED=1. You can see I set CGO_ENABLED=0 in the docker build, which will cause this hanging problem.

First, go1.16 was buggy. The first version that had no known issues with things like runtime.AllThreadsSyscall() and/or the corresponding cgo redirected use of C.setgroups() was go1.16.4.

I'll have to figure out the docker thing - debugging in that environment might take some setup on my part.

This problem also happened in the latest go1.17.5. Also, I've tried just now, go1.16.12 has also hang.

The io.Copy() angle is interesting. Assign this bug to me, I'll post an update when I can reproduce things. If you can find a smaller example that also hangs that would also be much appreciated.

@AndrewGMorgan I've assigned this issue to you per your request.

There is a great deal of code in this combined system.

I've been trying to isolate the code actually tripping whatever the issue is and build a minimal reproduction by skipping lots of it. If I remove the pty stuff and hard code the client to not request session.RequestPty(), things seem to successfully syscall.Setgroups and progress beyond that:

2021/12/15 21:19:48 ========= 2 ==========: [10 18 54 1000]
2021/12/15 21:19:48 ========= 3 ==========: 1000
2021/12/15 21:19:48 ========= 4 ==========: 1000
2021/12/15 21:19:48 Execute

Yes, it seems pty may influence the behavior of Setgroups, but I don't know why

How reproducible is this failure mode? I'm not running under Docker since I want to debug in a familiar environment. Is this bug report claiming that Docker is a necessary ingredient to the failure mode?

Are you 100% sure they are not locking up somewhere in the log.WithError(...).Warn() ? The forest of dependencies and extra imports from the logrus thing make me worried the problem could be something else entirely...

When you say it reproduces with go1.17.5 #50113 (comment) I wonder if you could do the following while in the locked-up state?

$ sudo gdb ./name-of-server-binary
(gdb) attach <PID OF RUNNING SERVER BINARY>
(gdb) info th

I re-enabled the pty package, but completely replaced the logrus package use with the standard log.Print() and log.Printf() functions to cut down on the amount of code I need to follow, and again things appear to work just fine:

2021/12/16 17:22:39 ========= 2 ==========: [10 18 54 1000]
2021/12/16 17:22:39 ========= 3 ==========: 1000
2021/12/16 17:22:39 ========= 4 ==========: 1000
2021/12/16 17:22:39 Execute
2021/12/16 17:22:57 Exit successfully
2021/12/16 17:22:57 session quited
2021/12/16 17:22:57 [copy sshChan to pty] ~~~~~~~~~~ 1.1.2 ~~~~~~~~~

In your write-up, you say:

	log.Infof("========= 2 ==========: %+v", lr.GIDs)
	if len(lr.GIDs) != 0 {
		if err := syscall.Setgroups(lr.GIDs); err != nil { // <- Server hangs here
			log.WithError(err).Warn("Setgroups")
		}
	}

So, since things work for me when I run with golang's default log.Print(), I'm wondering if the problem is actually on the next line?

FWIW When I ran go mod tidy after reinserting the pty handling code it also included v1.1.8.

I managed to reproduce a hang in a manner that I can debug a bit. It appears to involve the main.CopyWithContext.func1 () line io.Copy(a,b). That is the goroutine invoked with a=s.sshChan and b=s.pty.

func CopyWithContext(ctx context.Context, a, b io.ReadWriteCloser) {
	log.Infof("~~~~~~~~~~ 1 ~~~~~~~~~")
	ctx, cancel := context.WithCancel(ctx)
	go func() {
		log.Infof("[copy sshChan to pty] ~~~~~~~~~~ 1.1.1 ~~~~~~~~~")
		io.Copy(a, b)         <----- THIS LINE
		cancel()
		log.Infof("[copy sshChan to pty] ~~~~~~~~~~ 1.1.2 ~~~~~~~~~")
	}()
	go func() {
		log.Infof("[copy pty to sshChan] ~~~~~~~~~~ 1.2.1 ~~~~~~~~~")
		io.Copy(b, a)
		cancel()
		log.Infof("[copy pty to sshChan] ~~~~~~~~~~ 1.2.1 ~~~~~~~~~")
	}()
	<-ctx.Done()
	a.Close()
	b.Close()
	log.Infof("~~~~~~~~~~~~ 2 ~~~~~~~~~~~~~")
}

Yes, this function may influence the behavior. But I don't how why this function may due to the hanging problem

Thanks for this bug report and the reproducer.

Some code spelunking and web surfing later, this looks like a deadlock issue with blocking read system calls. It seems to share some underlying similarity to #38618 for example. However, the victim-vector here is that the syscall.AllThreadsSyscall() needs to execute code on every thread, but one thread (the one blocked reading s.pty) stubbornly refuses to exit the kernel, so the all threads syscall can never complete.

It appears that the core of the issue is that os.OpenFile() etc code doesn't treat s.pty in a pollable way. That is, the way the "github.com/kr/pty" package opens the pseudo terminal and operates on its s.pty.Fd(), it has blocking semantics. As explained in #38618 there is a workaround. I've implemented a sketch of this workaround with a patch (see below) to your server code (which drops the "github.com/kr/pty" package in favor of inlining the minimally required pty stuff for Linux).

This user code workaround is not that satisfying as a solution because it leaves open the possibility that the all threads syscall mechanism can deadlock in other situations like this. Hopefully, however, it can unblock (no pun intended!) your work.

I'd also urge you to reconsider using the fragile single-OS thread syscall.RawSyscall()'s to set uid and gid, but the syscall.AllThreadsSyscall() backed syscall.Setgroups() implementation for setting groups. This feels like crossing beams in a way that probably won't end well. You should take a look at using the SysProcAttr mechanism as @ianlancetaylor suggests above, or embrace something like the cap (shameless plug) package to support proper Linux privilege management within your server.

I'm next going to explore if I can create a more minimal reproducer for this deadlock and then explore the patch @ianlancetaylor briefly suggested in #38618 which seems like it will cover more corner cases than this user code workaround.

From 073948275eadec1b3de96195fed04e3edd54b9e0 Mon Sep 17 00:00:00 2001
From: "Andrew G. Morgan" <morgan@kernel.org>
Date: Sat, 15 Jan 2022 18:13:27 -0800
Subject: [PATCH] Inline a non-blocking pty configuration

This is a workaround for a deadlock in the syscall.AllThreadsSyscall()
implementation when one of the OS threads refuses to exit the kernel.

See https://github.com/golang/go/issues/50113 for details.

Signed-off-by: Andrew G. Morgan <morgan@kernel.org>
---
 server/pty.go | 94 ++++++++++++++++++++++++++++++++++++++++++++++-----
 server/tcp.go |  1 +
 2 files changed, 86 insertions(+), 9 deletions(-)

diff --git a/server/pty.go b/server/pty.go
index 9b6d951..50431d5 100644
--- a/server/pty.go
+++ b/server/pty.go
@@ -2,11 +2,12 @@ package main
 
 import (
 	"context"
+	"fmt"
 	"io"
+	"os"
 	"syscall"
 	"unsafe"
 
-	krpty "github.com/kr/pty"
 	log "github.com/sirupsen/logrus"
 	"golang.org/x/crypto/ssh"
 )
@@ -20,6 +21,85 @@ type PtyReq struct {
 	TermModes   string
 }
 
+// openNonBlocking returns a filedescriptor opened in non-blocking
+// mode and a file pointing to it.
+func openNonBlocking(path string, mode int) (fd uintptr, f *os.File, err error) {
+	var p int
+	p, err = syscall.Open("/dev/ptmx", mode, 0)
+	if err != nil {
+		return
+	}
+	fd = uintptr(p)
+
+	defer func() {
+		if err != nil {
+			if f != nil {
+				f.Close()
+				f = nil
+			} else {
+				syscall.Close(p)
+			}
+			fd = ^uintptr(0)
+		}
+	}()
+
+	err = syscall.SetNonblock(p, true)
+	if err != nil {
+		return
+	}
+
+	f = os.NewFile(fd, path)
+	return
+}
+
+var badFd = ^uintptr(0)
+
+//go:uintptrescapes
+func ioctl(a1, a2, a3 uintptr) (err error) {
+	_, _, errno := syscall.Syscall(syscall.SYS_IOCTL, a1, a2, a3)
+	if errno == 0 {
+		return nil
+	}
+	return errno
+}
+
+// open configures a pty for use by the server.
+func open() (fd uintptr, pty, tty *os.File, err error) {
+	if fd, pty, err = openNonBlocking("/dev/ptmx", syscall.O_RDWR); err != nil {
+		fd = badFd
+		return
+	}
+
+	// In case of error after this point, make sure we close the pty and tty.
+	defer func() {
+		if err != nil {
+			if pty != nil {
+				pty.Close()
+				pty = nil
+			}
+			if tty != nil {
+				tty.Close()
+				tty = nil
+			}
+			fd = badFd
+		}
+	}()
+
+	var n uint
+	if err = ioctl(fd, syscall.TIOCGPTN, uintptr(unsafe.Pointer(&n))); err != nil {
+		return
+	}
+	sname := fmt.Sprint("/dev/pts/", n)
+
+	var u int
+	if err = ioctl(fd, syscall.TIOCSPTLCK, uintptr(unsafe.Pointer(&u))); err != nil {
+		return
+	}
+
+	tty, err = os.OpenFile(sname, syscall.O_RDWR|syscall.O_NOCTTY, 0)
+	return
+}
+
 func (s *session) handlePtyReq(ctx context.Context, req *ssh.Request) bool {
 	if s.pty != nil {
 		return false
@@ -32,12 +112,12 @@ func (s *session) handlePtyReq(ctx context.Context, req *ssh.Request) bool {
 		return false
 	}
 
-	if s.pty, s.tty, err = krpty.Open(); err != nil {
-		log.WithError(err).Error("krpty.Open")
+	if s.ptyFd, s.pty, s.tty, err = open(); err != nil {
+		log.WithError(err).Error("pty open")
 		return false
 	}
 
-	if err = setWinsize(s.pty.Fd(), ptyReq.Width, ptyReq.Height); err != nil {
+	if err = setWinsize(s.ptyFd, ptyReq.Width, ptyReq.Height); err != nil {
 		log.WithError(err).Error("Set window initial size")
 		return false
 	}
@@ -55,11 +135,7 @@ func setWinsize(fd uintptr, w, h uint32) error {
 		y      uint16 // unused
 	}
 	ws := &Winsize{Width: uint16(w), Height: uint16(h), x: 0, y: 0}
-	_, _, errno := syscall.Syscall(syscall.SYS_IOCTL, fd, uintptr(syscall.TIOCSWINSZ), uintptr(unsafe.Pointer(ws)))
-	if errno != 0 {
-		return errno
-	}
-	return nil
+	return ioctl(fd, uintptr(syscall.TIOCSWINSZ), uintptr(unsafe.Pointer(ws)))
 }
 
 func CopyWithContext(ctx context.Context, a, b io.ReadWriteCloser) {
diff --git a/server/tcp.go b/server/tcp.go
index 880ab40..dc79da8 100644
--- a/server/tcp.go
+++ b/server/tcp.go
@@ -64,6 +64,7 @@ func prepared(req *ssh.Request, ch chan<- int) {
 type session struct {
 	sshConn  *ssh.ServerConn
 	sshChan  ssh.Channel
+	ptyFd    uintptr
 	pty, tty *os.File
 	cmd      *exec.Cmd
 }
-- 
2.34.1

Perhaps a more elegant workaround is to completely avoid the need to ever call s.pty.Fd(). It is a side effect of this call that forces the reads to become blocking and non-pollable.

While we still inline the pseudo terminal setup to avoid the blocking triggered by "github.com/kr/pty", the following server patch implements the ioctl functionality as operating on the *os.File directly in such a way that it does not introduce non-pollable reads. That is, this doesn't require the rest of the code to ever pay attention to the raw file descriptor. As such, the code can use os.OpenFile() as per the original code:

From 12c4649cc5eb502cee3c21b387d40a07a62c9192 Mon Sep 17 00:00:00 2001
From: "Andrew G. Morgan" <morgan@kernel.org>
Date: Sat, 15 Jan 2022 22:08:07 -0800
Subject: [PATCH] Inline pollable pty configuration code

This workaround avoids calling s.pty.Fd() and thus avoids s.pty from
becoming non-pollable. In this way we avoid a deadlock in the
syscall.AllThreadsSyscall() implementation when one of the OS threads
refuses to exit the kernel.

See https://github.com/golang/go/issues/50113 for details.

Signed-off-by: Andrew G. Morgan <morgan@kernel.org>
---
 server/pty.go | 69 +++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 59 insertions(+), 10 deletions(-)

diff --git a/server/pty.go b/server/pty.go
index 9b6d951..dbb365a 100644
--- a/server/pty.go
+++ b/server/pty.go
@@ -2,11 +2,12 @@ package main
 
 import (
 	"context"
+	"fmt"
 	"io"
+	"os"
 	"syscall"
 	"unsafe"
 
-	krpty "github.com/kr/pty"
 	log "github.com/sirupsen/logrus"
 	"golang.org/x/crypto/ssh"
 )
@@ -20,6 +21,58 @@ type PtyReq struct {
 	TermModes   string
 }
 
+//go:uintptrescapes
+func ioctlFile(f *os.File, a1, a2 uintptr) error {
+	s, err := f.SyscallConn()
+	if err != nil {
+		return err
+	}
+	s.Control(func(fd uintptr) {
+		_, _, errno := syscall.Syscall(syscall.SYS_IOCTL, fd, a1, a2)
+		if errno == 0 {
+			return
+		}
+		err = errno
+	})
+	return err
+}
+
+// open configures a pty for use by the server.
+func open() (pty, tty *os.File, err error) {
+	if pty, err = os.OpenFile("/dev/ptmx", syscall.O_RDWR, 0); err != nil {
+		return
+	}
+
+	// In case of error after this point, make sure we close the
+	// pty and tty.
+	defer func() {
+		if err != nil {
+			if pty != nil {
+				pty.Close()
+				pty = nil
+			}
+			if tty != nil {
+				tty.Close()
+				tty = nil
+			}
+		}
+	}()
+
+	var n uint
+	if err = ioctlFile(pty, syscall.TIOCGPTN, uintptr(unsafe.Pointer(&n))); err != nil {
+		return
+	}
+	sname := fmt.Sprint("/dev/pts/", n)
+
+	var u int
+	if err = ioctlFile(pty, syscall.TIOCSPTLCK, uintptr(unsafe.Pointer(&u))); err != nil {
+		return
+	}
+
+	tty, err = os.OpenFile(sname, syscall.O_RDWR|syscall.O_NOCTTY, 0)
+	return
+}
+
 func (s *session) handlePtyReq(ctx context.Context, req *ssh.Request) bool {
 	if s.pty != nil {
 		return false
@@ -32,12 +85,12 @@ func (s *session) handlePtyReq(ctx context.Context, req *ssh.Request) bool {
 		return false
 	}
 
-	if s.pty, s.tty, err = krpty.Open(); err != nil {
-		log.WithError(err).Error("krpty.Open")
+	if s.pty, s.tty, err = open(); err != nil {
+		log.WithError(err).Error("pty open")
 		return false
 	}
 
-	if err = setWinsize(s.pty.Fd(), ptyReq.Width, ptyReq.Height); err != nil {
+	if err = setWinsize(s.pty, ptyReq.Width, ptyReq.Height); err != nil {
 		log.WithError(err).Error("Set window initial size")
 		return false
 	}
@@ -47,7 +100,7 @@ func (s *session) handlePtyReq(ctx context.Context, req *ssh.Request) bool {
 }
 
 // setWinsize sets the size of the given pty.
-func setWinsize(fd uintptr, w, h uint32) error {
+func setWinsize(f *os.File, w, h uint32) error {
 	type Winsize struct {
 		Height uint16
 		Width  uint16
@@ -55,11 +108,7 @@ func setWinsize(fd uintptr, w, h uint32) error {
 		y      uint16 // unused
 	}
 	ws := &Winsize{Width: uint16(w), Height: uint16(h), x: 0, y: 0}
-	_, _, errno := syscall.Syscall(syscall.SYS_IOCTL, fd, uintptr(syscall.TIOCSWINSZ), uintptr(unsafe.Pointer(ws)))
-	if errno != 0 {
-		return errno
-	}
-	return nil
+	return ioctlFile(f, uintptr(syscall.TIOCSWINSZ), uintptr(unsafe.Pointer(ws)))
 }
 
 func CopyWithContext(ctx context.Context, a, b io.ReadWriteCloser) {
-- 
2.34.1

It seems like a general problem if a blocking system call can block syscall.Setgroups (although not necessarily a very important one). Would it be OK to have a ragged edge ending the call, in which Setgroups can return even though not every thread has yet executed the call, as long as the thread is sure to execute the call before returning to Go code? If not, then I guess we could use a signal, although that seems like a lot of code for a very rare case.

The problem with delaying a syscall like this it that while this thread is blocked there may be another call to something that also needs to run a privileged operation on all threads. Typically, these sorts of things come in two or more back to back privilege manipulation sequences - each building on the last. I think we'd end up needing to manage a queue of pending changes per thread for that to have a hope. Also, there are some strange kernel ABI issues with stracing and other things when the privilege of a process' threads get out of sync.

I don't much like the complexity of augmenting a signal but it is unclear to me how else to break out of the syscall.

First, going to develop a simpler test case. Then I'll explore ways to address this. We can see how minimal a fix develops. Fortunately, there is a work around for this case. Hopefully, that can be adapted for any others that emerge while investigating...

Here is a minimal reproducer. No privilege needed, no non-standard packages. It needs to be compiled CGO_ENABLED=0 (since syscall.AllThreadsSyscall() always fails when compiled CGO_ENABLED=1):

package main

import (
	"log"
	"os"
	"syscall"
	"time"
)

const prSetKeepCaps = 8

func main() {
	r, w, err := os.Pipe()
	if err != nil {
		log.Fatalf("failed to obtain pipe: %v", err)
	}
	data := make([]byte, 2+r.Fd())
	go r.Read(data)
	time.Sleep(500 * time.Millisecond)
	syscall.AllThreadsSyscall(syscall.SYS_PRCTL, prSetKeepCaps, 1, 0)
	w.Close()
	r.Close()
}

This change to go (at HEAD) makes that minimal test run without deadlocking:

$ git diff --ignore-space-change
diff --git a/src/runtime/proc.go b/src/runtime/proc.go
index 7509f7632f..eb40ed3af4 100644
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -1758,6 +1758,11 @@ func syscall_runtime_doAllThreadsSyscall(fn func(bool) bool) {
                                // next real wakeup will occur after
                                // startTheWorldGC() is called.
                                notewakeup(&mp.park)
+                               if mp.curg != nil {
+                                       // Thread blocked in syscall?
+                                       // (See golang.org/issue/50113.)
+                                       preemptM(mp)
+                               }
                        }
                        unlock(&mp.mFixup.lock)
                }
diff --git a/src/runtime/signal_unix.go b/src/runtime/signal_unix.go
index 08f266cc67..4f6d6652f3 100644
--- a/src/runtime/signal_unix.go
+++ b/src/runtime/signal_unix.go
@@ -616,13 +616,20 @@ func sighandler(sig uint32, info *siginfo, ctxt unsafe.Pointer, gp *g) {
                return
        }
 
-       if sig == sigPreempt && debug.asyncpreemptoff == 0 {
+       if sig == sigPreempt {
+               if mDoFixup() {
+                       // Preempted to unblock thread out of syscall
+                       // from syscall_runtime_doAllThreadsSyscall().
+                       return
+               }
+               if debug.asyncpreemptoff == 0 {
                        // Might be a preemption signal.
                        doSigPreempt(gp, c)
                        // Even if this was definitely a preemption signal, it
                        // may have been coalesced with another signal, so we
                        // still let it through to the application.
                }
+       }
 
        flags := int32(_SigThrow)
        if sig < uint32(len(sigtable)) {

The original code runs for a while with the above patch to go, but eventually crashes. I'm still investigating.

I think we do want something like the patch in #50113 (comment). The ability for a blocking system call anywhere in the program to block execution is AllThreadsSyscall is a quite painful limitation, particularly since most users won't have the full-program awareness to know if there program might ever execute a blocking system call.

(Some system calls may not even be expected to block indefinitely, but expect to be woken by an action on another thread. But since we have stopped the world, that action never occurs and this thread remains blocked forever, causing a deadlock).

cc @aclements

Change https://golang.org/cl/381534 mentions this issue: syscall: Fix deadlock for syscall.AllThreadsSyscall()

Re point 3 of #50113 (comment) there is a corner case that I can't quite get my head around. Namely, when the code is racing thread creation. That is, when the mp entry is waiting for its procid to be filled in, there is nothing to direct the signal at. Currently, we call the fixup just after mp.procid is set. So, peppering explicit fixup calls is sort of needed for cases like this.

For the above cl, I've included a slightly more stressful variant of the minimal test case #50113 (comment) . I've also validated that it the deadlocking workload pointed to at the top of this bug (https://github.com/weixiao-huang/golang-setgroups-hang) also works without deadlock when this change is applied.

@gopherbot, please backport to Go 1.17

@gopherbot, please backport to Go 1.16

Backport issue(s) opened: #50976 (for 1.16).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot, please backport to Go 1.17

Change https://golang.org/cl/383434 mentions this issue: syscall, runtime: reimplement AllThreadsSyscall using only signals.

1.17 backport issue #51077

Change https://go.dev/cl/383996 mentions this issue: runtime: move doAllThreadsSyscall to os_linux.go

Change https://go.dev/cl/383999 mentions this issue: runtime/internal/syscall: new package for linux

We ended up with:

• a (back-portable) point fix: https://golang.org/cl/381534
• a rewrite from scratch: https://golang.org/cl/383434

The Go team has opted to abandon the current implementation (leaving this present bug in 1.16, 1.17), and adopt the substantial rewrite in 1.18.

For this present issue, for earlier releases, the following options are available:

• a user code workaround (related to not using .Fd())
• to compile CGO_ENABLED=1
• for posterity, a workaround patch for go 1.16 and 1.17 is attached here: c871275.diff.zip.