syscall: SysProcAttr{ NoInheritHandles: true } broken in 1.17 on Windows

[JohanKnutzen]

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

go version go1.17 windows/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output

$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\knutz\AppData\Local\go-build
set GOENV=C:\Users\knutz\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\knutz\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\knutz\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.17
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\code\fd\agent_svc\go.mod
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\knutz\AppData\Local\Temp\go-build1048178528=/tmp/go-build -gno-record-gcc-switches

What did you do?

package main

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

func main() {
	cmd := exec.Command("notepad.exe")
	cmd.SysProcAttr = &syscall.SysProcAttr{
		NoInheritHandles: true,
	}
	err := cmd.Start()
	log.Printf("err: %v", err)
}

What did you expect to see?

2021/08/28 11:59:56 err: <nil>

What did you see instead?

2021/08/28 12:00:02 err: fork/exec C:\Windows\system32\notepad.exe: The parameter is incorrect.

The problem

NoInheritHandles was added in 1.16:
1e3b535#diff-ec673c10a75fe2d2faa9c788e03823294b263c68cc3de50f4a0865a53e28f3a3

Between that commit and 1.17 there was a series of commits by @zx2c4:
https://github.com/golang/go/commits/master/src/syscall/exec_windows.go

I don't fully understand the reasoning behind the changes but they are documented here: 2d76081#diff-ec673c10a75fe2d2faa9c788e03823294b263c68cc3de50f4a0865a53e28f3a3

These changes are not compatible with NoInheritHandles because they seem to add handles explicitly on this line:

go/src/syscall/exec_windows.go

Line 395 in f4cd001

err = updateProcThreadAttribute(si.ProcThreadAttributeList, 0, _PROC_THREAD_ATTRIBUTE_HANDLE_LIST, unsafe.Pointer(&fd[0]), uintptr(len(fd))*unsafe.Sizeof(fd[0]), nil, nil)

Windows doesn't accept adding handles + NoInheritHandles which I guess makes sense.

How to solve this?

I'm not sure what the new commits do by @zx2c4 or how they are expected to act when NoInheritHandles is true. I would assume that no manipulation of handles/adding handles should happen. You can toggle between a non-working/working state by changing this:

// Do not accidentally inherit more than these handles.
if len(fd) > 0 {
	err = updateProcThreadAttribute(si.ProcThreadAttributeList, 0, _PROC_THREAD_ATTRIBUTE_HANDLE_LIST, unsafe.Pointer(&fd[0]), uintptr(len(fd))*unsafe.Sizeof(fd[0]), nil, nil)
	if err != nil {
		return 0, 0, err
	}
}

to

// Do not accidentally inherit more than these handles.
if len(fd) > 0 && !sys.NoInheritHandles {
	err = updateProcThreadAttribute(si.ProcThreadAttributeList, 0, _PROC_THREAD_ATTRIBUTE_HANDLE_LIST, unsafe.Pointer(&fd[0]), uintptr(len(fd))*unsafe.Sizeof(fd[0]), nil, nil)
	if err != nil {
		return 0, 0, err
	}
}

in this file:

go/src/syscall/exec_windows.go

Line 393 in f4cd001

// Do not accidentally inherit more than these handles.

@zx2c4 - What do you think? Would it be possible for you to fix this? Otherwise I can submit a PR with the above quick fix but I'm not sure how that influences the rest.

[zx2c4]

I'll be back from travels in about a week and will fix it. Sorry for the regression.

I suppose the issue stems from exec.Command passing some pipes as std handles?

[JohanKnutzen]

@zx2c4 Could be, len(fd) seems never to be 0

Also @zx2c4 don't sweat it, we'll stick to 1.16 in the meantime. Enjoy your travels!

[bcmills]

@gopherbot, please backport to Go 1.17. This is a regression from Go 1.16.

[gopherbot]

Backport issue(s) opened: #48074 (for 1.16), #48075 (for 1.17).

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]

Change https://golang.org/cl/350411 mentions this issue: syscall: do not use handle lists on windows when NoInheritHandles is true

[gopherbot]

Change https://golang.org/cl/350416 mentions this issue: [release-branch.go1.17] syscall: do not use handle lists on windows when NoInheritHandles is true