os: ERROR_NO_SYSTEM_RESOURCES when making child process use os.Stdout on Windows 7

[AkarinVS]

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

$ go version
go version devel +41cf18eda7 Fri Apr 2 19:45:52 2021 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes. (Actually, no, this is a new regression, see below.)

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

The windows program is cross compiled from linux/amd64 to windows/amd64.
The windows version is Windows 7 (version 6.1.7601), vanilla installation.

What did you do?

The following command tries to execute cmd /c echo abc and let the child process use os.Stdout.
If there is no command line argument, it will simply set cmd.Stdout = os.Stdout;
otherwise, it will create a goroutine to copy cmd.StdoutPipe() to os.Stdout.
https://play.golang.org/p/rzblvBHqLb2

package main

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

func main() {
	cmd := exec.Command("cmd", "/c", "echo", "abc")
	if len(os.Args) == 1 {
		println("bug")
		cmd.Stdout = os.Stdout
	} else {
		println("io.Copy")
		outpipe, err := cmd.StdoutPipe()
		if err != nil {
			log.Fatal(err)
		}
		defer outpipe.Close()
		go io.Copy(os.Stdout, outpipe)
	}
	err := cmd.Run()
	if err != nil {
		log.Fatal(err)
	}
}

What did you expect to see?

C:\> test.exe
bug
abc

C:\> test.exe test
io.Copy
abc

What did you see instead?

C:\> test.exe
bug
2021/05/02 00:01:02 fork/exec c:\windows\system32\cmd.exe: Insufficient system resources exist to complete the requested service.

C:\> test.exe test
io.Copy
abc

This problem reproduces every time, even when the system is freshly rebooted, so error message is likely bogus.
Also, this bug also applies to other command line programs, and it's not specific to cmd.exe.

Debugging revealed that the error is returned by CreateProcessW, and some (e.g. rprichard/win32-console-docs#6) mentions that "CreateProcessW fails with ERROR_NO_SYSTEM_RESOURCES when used with EXTENDED_STARTUPINFO_PRESENT and an attempt is made to restrict the inherited handles. The problem appears only on Win7 OS as in your tests."

[ianlancetaylor]

Do you know whether this has ever worked? That is, do you know whether it works with older releases of Go? Thanks.

[AkarinVS]

Actually, this is a new regression, sorry I didn't check the latest release before filing the issue. This regression is introduced by revision 2d76081. https://golang.org/cl/288297 /cc @zx2c4 @alexbrainman

[zx2c4]

Does https://golang.org/cl/316269 fix this issue?

[AkarinVS]

Tested on Windows 7, version go1.17-f7d61bd050 (CL 316269) does not fix the issue. The error is still the same.

[alexbrainman]

@AkarinVS thank you for reporting the issue. I can reproduce it on Windows 7.

I googled and I found other developers already encountered similar issue. 

https://bugzilla.mozilla.org/show_bug.cgi?id=1460995

bazelbuild/bazel#8676

https://github.com/bazelbuild/bazel/pull/8793/files

https://github.com/rprichard/win32-console-docs/blob/master/src/HandleTests/CreateProcess_InheritList.cc

It appears that PROC_THREAD_ATTRIBUTE_HANDLE_LIST cannot be used with console handles on Windows 7.

I am not sure what to do here.

We can, probably, use old pre-PROC_THREAD_ATTRIBUTE_HANDLE_LIST code for this situation. But what do we do about new features that are not supported without PROC_THREAD_ATTRIBUTE_HANDLE_LIST? And that would require restoring some of our before-PROC_THREAD_ATTRIBUTE_HANDLE_LIST code.

If we decide to restore our old CreateProcess code, perhaps we could leave current version of CreateProcess as is. But, if it returns ERROR_NO_SYSTEM_RESOURCES, then call old version of CreateProcess code. This should not punish majority of users for the benefits of small group of Windows 7 users.

We could also use GetFileType to decide which version of the code to run. But I am bit fuzzy about this logic. For example, I printed GetFileType of stdin, stdout and stderr while running program above, and that is what I see:

X:\>type a
diff --git a/src/syscall/exec_windows.go b/src/syscall/exec_windows.go
index 253e9e8c1f..c6c39933b6 100644
--- a/src/syscall/exec_windows.go
+++ b/src/syscall/exec_windows.go
@@ -321,6 +321,10 @@ func StartProcess(argv0 string, argv []string, attr *ProcAttr) (pid int, handle
        fd := make([]Handle, len(attr.Files))
        for i := range attr.Files {
                if attr.Files[i] > 0 {
+
+       ft, _ := GetFileType(Handle(attr.Files[i]))
+       println("ft=", ft)
+
                        err := DuplicateHandle(p, Handle(attr.Files[i]), parentProcess, &fd[i], 0, true, DUPLICATE_SAME_ACCESS)
                        if err != nil {
                                return 0, 0, err

X:\>test
bug
ft= 2
ft= 2
ft= 2
2021/05/07 12:15:05 fork/exec C:\Windows\system32\cmd.exe: Insufficient system resources exist to complete the requested service.

X:\>test a
io.Copy
ft= 2
ft= 3
ft= 2
abc

X:\>

Alex

[AkarinVS]

If it's too hard to solve properly, perhaps we just try CreateProcess two times. If the first one fails with ERROR_NO_SYSTEM_RESOURCES, then we create goroutines to do the copy and retry one more time? Though I'm not sure whether transparently using goroutines to copy between the file descriptors is acceptable or not.

[alexbrainman]

If it's too hard to solve properly, perhaps we just try CreateProcess two times. If the first one fails with ERROR_NO_SYSTEM_RESOURCES, then we create goroutines to do the copy and retry one more time?

We don't need to use goroutines here. If first CreateProcess fails with ERROR_NO_SYSTEM_RESOURCES, we could call CreateProcess second time immediately, on the same goroutine.

Alex

[AkarinVS]

By using goroutine to copy, I basically mean this: // ... when the first CreateProcess fails, we pretend to do this. outpipe, err := cmd.StdoutPipe() if err != nil { // handle the error } go func () { io.Copy(os.Stdout, outpipe) // how to handle errors?? outpipe.Close() }() // and then CreateProcess again, but this time, the stdout will be pipe, so it should succeed. And similarly for Stderr.

[alexbrainman]

By using goroutine to copy, I basically mean this: ...

Sure. You can do this in your program.

Unfortunately the problem is in syscall.StartProcess. For example, some program might call syscall.StartProcess directly. We want to fix that program too.

Alex

[AkarinVS]

We can do that inside syscall.StartProcess, if we don't want to keep two copies of the code (before and after CL 288297). The question is, does the issue affect only stdin/stdout/stderr or it affects all console fds inherited by the child process?

[alexbrainman]

The question is, does the issue affect only stdin/stdout/stderr or it affects all console fds inherited by the child process?

If we don't use STARTUPINFOEXW, there are no other fds exported from Go parent process to its child but stdin, stdout and stderr.

Alex

[zx2c4]

I suspect the thing to do on Windows 7 might be to actually exclude console handles from PROC_THREAD_ATTRIBUTE_HANDLE_LIST, because on Windows 7, they're not real kernel handles but rather special user mode handles with the top bits set. I need to investigate a bit, but I think this might be solvable.

https://github.com/rprichard/win32-console-docs/blob/master/README.md has some great notes and https://github.com/rprichard/win32-console-docs/blob/master/src/HandleTests/CreateProcess_InheritList.cc is instructive.

[gopherbot]

Change https://golang.org/cl/319310 mentions this issue: syscall: do not pass console handles to PROC_THREAD_ATTRIBUTE_HANDLE_LIST on Windows 7

[zx2c4]

Just posted a fix. Please try that out and let me know if it does the trick?

[AkarinVS]

go1.17-4bdecdca17 (CL 319310 patch set 4) fixes the issue for me. Tested both amd64 and 386 on Windows 7. I'd suggest adding a test to CL 319310 though. Thanks for the fix.

[zx2c4]

I'd suggest adding a test to CL 319310 though.

I'd like this too, but it's a bit hard: it would need to write to a console window, which on windows 7 is a user space concept that involves the GUI, which we probably don't want as part of tests. If you have any ideas on how to avoid UI but still test console handles, do let me know.

[networkimprov]

@ianlancetaylor is it acceptable to import x/sys/windows in a Go stdlib test?

[ianlancetaylor]

Well, we are currently vendoring x/sys/windows under cmd, so I suppose that it wouldn't be a disaster to move that vendoring up a level. In general though I would prefer to stop vendoring x/sys/windows rather than start using it even more.

[alexbrainman]

it would need to write to a console window ... which we probably don't want as part of tests.

And why we could not make https://play.golang.org/p/rzblvBHqLb2 into a test?

Alex

[zx2c4]

@alexbrainman That relies on observing that the console window has the text. Doing that programatically in Go usually involves using a pipe, but on Windows 7, a pipe handle is a kernel object, so things will work expected. The bug manifests when using a console handle, rather than a pipe handle, which is not a kernel object on Windows 7. So this means the test passes when you see the text written on a console. How do you plan to check that programatically? There's actually ReadConsoleOuput, which might work, with selecting a rectangle. I started playing with that the other day, but it seemed like a waste of time. You'd need the UI too, anyway, to even have a console (psuedo consoles are new in Windows 10), and I don't think we want UIs in our tests.

So, I have no interest in writing a test case for this. If you want to try to figure it out, go for it, but I'm not going to spend the time.

However, https://golang.org/cl/319310 fixes the issue, and I'd appreciate it if you could review that so that we can submit it.

[alexbrainman]

@alexbrainman ...

I replied on https://go-review.googlesource.com/c/go/+/319310/

Alex

[gopherbot]

Change https://golang.org/cl/325112 mentions this issue: syscall: regenerate zsyscall_windows.go