Skip to content
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

runtime: Go 1.5.4/1.6.1 breaks running on Windows Nano Server #15286

Closed
jstarks opened this issue Apr 13, 2016 · 12 comments
Closed

runtime: Go 1.5.4/1.6.1 breaks running on Windows Nano Server #15286

jstarks opened this issue Apr 13, 2016 · 12 comments

Comments

@jstarks
Copy link

jstarks commented Apr 13, 2016

This regression occurred due to change b8af3fd associated with #14959. These changes, which add the LOAD_LIBRARY_SEARCH_SYSTEM32 flag to LoadLibraryEx calls to system DLLs, are incompatible with Windows Nano Server, which is currently in preview release. Go programs fail to load because shell32.dll cannot be found in the course of calling CommandLineToArgvW to parse the command line.

Nano Server does not have shell32.dll or many other DLLs that are part of standard Windows desktop and Windows Server installations. Nano Server is based on OneCore and as such doesn't have shell functionality; the API in question is moved to a different DLL. OneCore replaces explicit references to system DLLs with "API sets", which are DLL-like things that reference logical sets of APIs that may be hosted by different DLLs on different Windows platforms. For example, CommandLineToArgvW is in api-ms-win-shcore-obsolete-l1-1-0. (I don't know why it's in something marked "obsolete", that seems strange to me, too).

Of course, most software out there is not going to start linking to the new API sets any time soon, so Nano Server also has a mechanism to forward from the old DLLs to the new API sets. This mechanism is known internally as "reverse forwarders". These reverse forwarders allow Go software (and a lot of other open source software) to run on Nano Server without changes.

Unfortunately, the loader will not try to load these reverse forwarders if you pass LOAD_LIBRARY_SEARCH_SYSTEM32. This means that while Go 1.5.3 programs worked great on Nano Server, programs built with 1.5.4 will not. This is quite unfortunate for us since we are trying to support Docker on Nano Server, and Docker has just moved to Go 1.5.4 for the security fixes.

We are working internally at Microsoft to see if/how we can support LOAD_LIBRARY_SEARCH_SYSTEM32 with reverse forwarders. I am hopeful that by RTM of Nano Server we will be able to improve this messy situation. But for now, this change has broken us pretty badly.

One proposal for a workaround would be to revert to the old behavior if kernel32.dll cannot be loaded with LOAD_LIBRARY_SEARCH_SYSTEM32. That would be a pretty good indication that you're on a system that doesn't have a standard set of Windows DLLs, and that you shouldn't pass this flag. I can put together a proposed fix today if that's likely to be acceptable.

@bradfitz
Copy link
Contributor

Sorry, I'd never heard of Nano Server before this. I wrote b8af3fd but I did so blindly using MSDN docs (where I didn't see any reference to Nano Server), and testing on Go's trybot buidlers. Another contributor (@alexbrainman) verified it still passes on Windows XP, since we don't have a Windows XP build machine anymore (I'm working on resurrecting one).

If this Nano Server is something we should support, I suppose we'll need builders for it. I filed #15287 for that.

Our bar for Go point releases is very high. I don't think we would issue a point release for an unreleased in-development operating system, even for our Docker friends. We do, however, have another point release coming for all the other Go 1.6.x non-security bugs (1.6.2: https://github.com/golang/go/issues?utf8=%E2%9C%93&q=milestone%3AGo1.6.2+), so it's possible we could put some minimal change in there for that.

But...

We are working internally at Microsoft to see if/how we can support LOAD_LIBRARY_SEARCH_SYSTEM32 with reverse forwarders.

... it does sound like a Windows problem. We're using a that flag bit to say "use a system DLL, not the malicious DLL in the user's Downloads folder". If you moved Windows system DLLs around and need to remap things, I expect Windows to still map things and yet honor our intention with that flag.

@bradfitz bradfitz added this to the Unplanned milestone Apr 13, 2016
@bradfitz
Copy link
Contributor

I'm marking this as Unplanned (which we use to just mean it's been read, basically), but send a change for review (https://golang.org/doc/contribute.html) and we can consider for 1.6.2 if it arrives soon.

@jstarks
Copy link
Author

jstarks commented Apr 13, 2016

OK, thank you. I understand that this is a difficult request. I agree that this is a Windows problem, and ultimately we need a Windows solution. In the short term, though, we are a bit stuck. I should have an update later today or tomorrow.

@bradfitz bradfitz changed the title Go 1.5.4/1.6.1 breaks running on Windows Nano Server runtime: Go 1.5.4/1.6.1 breaks running on Windows Nano Server Apr 13, 2016
@bradfitz
Copy link
Contributor

@jstarks, I sent you CLA info to your microsoft.com email address I saw from Github. Let me know if you didn't get it.

@rsc
Copy link
Contributor

rsc commented Apr 14, 2016

Note that we're unlikely to issue 1.5.5 for this (Go 1.5 is unsupported except for critical security fixes, and this is a non-security problem in an unreleased piece of software). Even if Docker is using 1.5.4, it seems like it should be possible (trivial even) for the Windows Nano Server build to use 1.5.3. Again, Go 1.5 is not supported (see https://golang.org/doc/devel/release.html#policy).

I'm also somewhat skeptical of including a fix in 1.6.2, since again this is an unreleased piece of software, there is a plausible workaround (use 1.5.3 or 1.6), the functionality works fine on standard Windows, and it sounds like Windows Nano Server might fix the missing functionality before its official release. Especially since we now understand this section to be a security-critical part of the code, letting the fix work through the standard major release process for Go 1.7 seems preferable than rushing in a last-minute change.

@alexbrainman
Copy link
Member

@jstarks please try https://go-review.googlesource.com/22054. I just followed your proposed solution description. I don't have Nano Server to test my CL. And it is your fight to get this change (if accepted) into whatever release you want.

Alex

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/22054 mentions this issue.

tiborvass pushed a commit to tiborvass/docker that referenced this issue Apr 16, 2016
For context: golang/go#15286

This commit downloads go1.5.3 in addition to go1.5.4 in order to
workaround the issue.

It is not expected to do a Docker release without a proper fix, however
this should help unblock Docker development on Windows TP5.

Signed-off-by: Tibor Vass <[email protected]>
@jstarks
Copy link
Author

jstarks commented Apr 18, 2016

Thanks bradfitz, alexbrainman, and rsc for the quick and helpful response on this. Alex -- I think there is a small problem with my proposal, but see below for an alternative that should be simpler anyway.

Docker is working around this for now by rolling back to Go 1.5.3 when building for Windows. This takes the pressure off getting a fix into Go, but of course is not ideal in the long term. I believe we can move to Golang 1.6.x for Docker when building on Windows -- Docker for Linux is stuck on 1.5.x due to some net/http compatibility issues that do not apply for Windows: moby/moby#20865.

For Nano Server RTM, we may be able to fix this in the OS, but given our release timelines this will add some complexity that the OS team would like to avoid. We'd like you to consider a fix in Go 1.6.2 as an alternative. Edit: We are evaluating solutions to determine the simplest working one; I'll post back with updates when I have them.

@alexbrainman
Copy link
Member

@jstarks I will wait for you to decide what you want before doing anything else.

Alex

@jstarks
Copy link
Author

jstarks commented May 6, 2016

Sorry for the long delay. Obviously we've missed the Go 1.6.2 window, and probably the Go 1.7 window. However, we've figured out how to fix this in Windows, so I'm closing this issue. Thanks again for everyone's help.

It did occur to me while looking at this that there is very little reason for Go to load shell32.dll to begin with. With some small changes we can eliminate that and at least one other DLL; this has the advantage of significantly improving the launch performance of Go processes on Windows. I will open a separate issue for that soon and provide some fixes I've prepared.

@jstarks jstarks closed this as completed May 6, 2016
@bradfitz
Copy link
Contributor

bradfitz commented May 6, 2016

Thanks for the update!

@alexbrainman
Copy link
Member

there is very little reason for Go to load shell32.dll to begin with. With some small changes we can eliminate that and at least one other DLL; this has the advantage of significantly improving the launch performance of Go processes on Windows. I will open a separate issue for that soon and provide some fixes I've prepared.

Please do. Thank you.
(We, obviously, won't be able to change anything until after go1.7 is released).

Alex

jstarks added a commit to jstarks/docker that referenced this issue May 20, 2016
This works around golang/go#15286 by explicitly loading shell32.dll at
load time, ensuring that syscall can load it dynamically during process
startup.

Signed-off-by: John Starks <[email protected]>
runcom pushed a commit to runcom/docker that referenced this issue May 25, 2016
This works around golang/go#15286 by explicitly loading shell32.dll at
load time, ensuring that syscall can load it dynamically during process
startup.

Signed-off-by: John Starks <[email protected]>
Signed-off-by: Antonio Murdaca <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants