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

syscall: be more helpful with Windows DLLs fail to load due to missing environment variables #25210

Open
bradfitz opened this Issue May 2, 2018 · 12 comments

Comments

Projects
None yet
5 participants
@bradfitz
Member

bradfitz commented May 2, 2018

I've been trying to get the Perkeep tests passing on Windows and just hit this flake with Go 1.10 that I've never seen before or found other reports of:

C:\Users\brad\go\src\perkeep.org>go test  ./pkg/test/integration
2018/05/01 20:47:44 Running make.go to build perkeep binaries for testing...
2018/05/01 20:47:47 Ran make.go.
2018/05/01 20:47:48 Running make.go to build perkeep binaries for testing...
2018/05/01 20:47:52 Ran make.go.
panic: CryptAcquireContext: Provider DLL failed to initialize correctly.

goroutine 1 [running]:
mime/multipart.randomBoundary(0x70, 0x9466a0)
        C:/Go/src/mime/multipart/writer.go:82 +0x147
mime/multipart.NewWriter(0xa4f2e0, 0xc04218c7e0, 0x30)
        C:/Go/src/mime/multipart/writer.go:30 +0x2d
perkeep.org/pkg/client.calculateMultipartOverhead(0x9c4770)
        C:/Users/brad/go/src/perkeep.org/pkg/client/upload.go:96 +0x52
--- FAIL: TestAndroidCamputFile (0.07s)
        camlistore_test.go:266: pk-put exited uncleanly: exit status 2
--- FAIL: TestCamputUploadOnce (0.19s)
        camput_test.go:125: wrong pk-put output; wanted sha1-381c42a63078ef49a2f1808318dbbafbb31a81d5, got sha1-68374998520d18899df837f7541d890fdf40d799
FAIL
FAIL    perkeep.org/pkg/test/integration        19.078s

C:\Users\brad\go\src\perkeep.org>go version
go version go1.10.1 windows/amd64

Any ideas?

/cc @alexbrainman

@bradfitz bradfitz added this to the Go1.11 milestone May 2, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 2, 2018

Actually, I'm hitting this reliably with Windows 10, Go 1.10.1, and Perkeep git rev https://camlistore.googlesource.com/camlistore/+/b9ab98d60de97f2e78913aecb587dd6013189eaa

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 2, 2018

Notably, that calculateMultipartOverhead runs at init time.

Is one not allowed to use crypto/rand on Windows at init time?

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 2, 2018

/cc @mpl

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 2, 2018

Ah, we had a child process that was being run in an integration test with an incomplete environment.

I discovered it because after I worked around this issue (by calculating the calculateMultipartOverhead lazily on first use with a sync.Once instead of at init time), then my test started failing with different but related errors about network initialization.

Could we repurpose this bug about making such Windows debugging adventures easier?

Can we log or return a better error somewhere when a DLL fails to load and we additionally have missing environment variables?

(sorry for talking to myself)

@bradfitz bradfitz changed the title from crypto/rand: CryptAcquireContext: Provider DLL failed to initialize correctly to syscall: be more helpful with Windows DLLs fail to load due to missing environment variables May 2, 2018

@alexbrainman

This comment has been minimized.

Member

alexbrainman commented May 2, 2018

Can we log or return a better error somewhere when a DLL fails to load and we additionally have missing environment variables?

I suppose the problem here is that syscall.CryptAcquireContext requires some environment variable set. We could find out what it needs and then check syscall.CryptAcquireContext error number, and if error number matches and that particular environment variable set, we could return different error message. I am just not sure, if that would be OK to change error message returned, because other people could be expecting that error.

But really - passing nearly empty environment to you child process on Windows is looking for trouble. I wish our API would stop us doing that.

Alex

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 2, 2018

But really - passing nearly empty environment to you child process on Windows is looking for trouble. I wish our API would stop us doing that.

Yeah, that's another possibility: on Windows we could force certain environment variables to be inherited in os/exec. Perhaps if they're not mentioned at all, we auto-add them, but if the user really, really wants to run the child process with an empty environment key, they can set it to the empty string?

bradfitz added a commit to perkeep/perkeep that referenced this issue May 3, 2018

all: Windows fixes (don't listen on file descriptors in test.World, etc)
test/integration: don't listen on file descriptors.
make.go: unrelated, but options to make it much faster.
internal/images: t.Skip on HEIC dependency failures

Fixes #1140
Updates golang/go#25210

Change-Id: I8092155411826d6ed1f8d85230b753d1369044af
@alexbrainman

This comment has been minimized.

Member

alexbrainman commented May 3, 2018

Perhaps it has been discussed before, but why does os.Environ() returns []string ? Shouldn't it return map[string]string ? map[string]string would work on Unix. Unfortunately Windows does not distinguish between PATH and path environment variables, so map[string]string is not good for Windows.

Alex

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 4, 2018

@alexbrainman, let's leave such discussion to newly-opened #25252

@rsc

This comment has been minimized.

Contributor

rsc commented Jul 19, 2018

I don't think we should do anything in syscall but I think it would be appropriate for os/exec to take care of this and say that unless SYSTEMROOT (or SystemRoot, systemroot, etc) is specified explicitly on Windows, it's copied in implicitly.

@gopherbot

This comment has been minimized.

gopherbot commented Jul 19, 2018

Change https://golang.org/cl/124858 mentions this issue: cmd/go: preserve %SYSTEMROOT% in TestScript on Windows

gopherbot pushed a commit that referenced this issue Jul 19, 2018

cmd/go: preserve %SYSTEMROOT% in TestScript on Windows
Windows networking doesn't work without this environment variable (#25210).

Re-enable TestScript on Windows, and fix two minor failures.

Fixes #26457.

Change-Id: Id9bea49dfb58403195c29c3d831a532ef0f9a233
Reviewed-on: https://go-review.googlesource.com/124858
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@alexbrainman

This comment has been minimized.

Member

alexbrainman commented Jul 20, 2018

I think it would be appropriate for os/exec to take care of this and say that unless SYSTEMROOT .... is specified explicitly on Windows, it's copied in implicitly.

Too much magic for me. If I don't include SYSTEMROOT in my environment, I expect os/exec not to put it there.

Also how do you know that SYSTEMROOT is what is missing to fix your failure? What about other environment variables? Different libs / packages installed on my computer adds different environment variables. How are you going to support these libs / packages?

I think it is much simpler, when creating child process, just copy parent environment and add / remove / modify whatever environment variables you like.

Alex

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jul 20, 2018

os/exec is a high-level helper package. It's quite common for people to forget to copy the parent environment, so I think it's fine to auto-add critical environment variables that aren't mentioned explicitly.

Also how do you know that SYSTEMROOT is what is missing to fix your failure? What about other environment variables? Different libs / packages installed on my computer adds different environment variables. How are you going to support these libs / packages?

So far it's been implicated several times now. I think fixing the common ones is enough. We don't have to do all of them.

I think it is much simpler, when creating child process, just copy parent environment and add / remove / modify whatever environment variables you like.

In retrospect that should've been the API from the beginning (a list of modifications, rather than the new environment completely), but that's not where we're at.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment