Skip to content

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

Closed
Closed
syscall: be more helpful with Windows DLLs fail to load due to missing environment variables#25210
Labels
FrozenDueToAgeNeedsInvestigationSomeone must examine and confirm this is a valid issue and not a duplicate of an existing one.OS-Windowshelp wanted
Milestone
Go1.13

Description

@bradfitz
bradfitz
opened on 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

Activity

  1. bradfitz
    added
    help wanted
    OS-Windows
    on May 2, 2018
  2. bradfitz
    added this to the Go1.11 milestone on May 2, 2018
  3. bradfitz
    added
    NeedsInvestigationSomeone must examine and confirm this is a valid issue and not a duplicate of an existing one.
    on May 2, 2018

  4. bradfitz commented on May 2, 2018

    @bradfitz
    bradfitz
    on May 2, 2018
    ContributorAuthor

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

  5. bradfitz commented on May 2, 2018

    @bradfitz
    bradfitz
    on May 2, 2018
    ContributorAuthor

    Notably, that calculateMultipartOverhead runs at init time.

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

  6. bradfitz commented on May 2, 2018

    @bradfitz
    bradfitz
    on May 2, 2018
    ContributorAuthor

    /cc @mpl

  7. bradfitz commented on May 2, 2018

    @bradfitz
    bradfitz
    on May 2, 2018
    ContributorAuthor

    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)

  8. bradfitz
    changed the title [-]crypto/rand: CryptAcquireContext: Provider DLL failed to initialize correctly[/-] [+]syscall: be more helpful with Windows DLLs fail to load due to missing environment variables[/+] on May 2, 2018

  9. alexbrainman commented on May 2, 2018

    @alexbrainman
    alexbrainman
    on May 2, 2018
    Member

    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

  10. bradfitz commented on May 2, 2018

    @bradfitz
    bradfitz
    on May 2, 2018
    ContributorAuthor

    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?

  11. bradfitz
    added a commit that references this issue on May 3, 2018

    all: Windows fixes (don't listen on file descriptors in test.World, etc)

    12894d4

  12. alexbrainman commented on May 3, 2018

    @alexbrainman
    alexbrainman
    on May 3, 2018
    Member

    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

  13. bradfitz commented on May 4, 2018

    @bradfitz
    bradfitz
    on May 4, 2018
    ContributorAuthor

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

  14. ianlancetaylor
    modified the milestones: Go1.11, Go1.12 on Jun 29, 2018

  15. rsc commented on Jul 19, 2018

    @rsc
    rsc
    on Jul 19, 2018
    Contributor

    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.

  16. gopherbot commented on Jul 19, 2018

    @gopherbot
    gopherbot
    on Jul 19, 2018
    Contributor

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

  17. gopherbot
    added a commit that references this issue on Jul 19, 2018

    cmd/go: preserve %SYSTEMROOT% in TestScript on Windows

    d3ca4c8

  18. alexbrainman commented on Jul 20, 2018

    @alexbrainman
    alexbrainman
    on Jul 20, 2018
    Member

    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

  19. bradfitz commented on Jul 20, 2018

    @bradfitz
    bradfitz
    on Jul 20, 2018
    ContributorAuthor

    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.

  20. andybons
    modified the milestones: Go1.12, Go1.13 on Feb 12, 2019

  21. gopherbot commented on Apr 29, 2019

    @gopherbot
    gopherbot
    on Apr 29, 2019
    Contributor

    Change https://golang.org/cl/174318 mentions this issue: os/exec: always set SYSTEMROOT on Windows if not listed in Cmd.Env

  22. bradfitz
    self-assigned this
    on Apr 29, 2019
  23. gopherbot
    closed this as completedin 06c9ccdon Apr 29, 2019
  24. golang
    locked and limited conversation to collaborators on Apr 28, 2020
  25. gopherbot
    added
    FrozenDueToAge
    on Apr 28, 2020
  26. rsc
    unassigned
    bradfitz
    on Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    FrozenDueToAgeNeedsInvestigationSomeone must examine and confirm this is a valid issue and not a duplicate of an existing one.OS-Windowshelp wanted

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions