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

cmd/link: Go linker may be vulnerable to Windows kernel bug #24169

Closed
randomascii opened this issue Feb 28, 2018 · 4 comments
Closed

cmd/link: Go linker may be vulnerable to Windows kernel bug #24169

randomascii opened this issue Feb 28, 2018 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows

Comments

@randomascii
Copy link

For more details on this bug see this blog post:
https://randomascii.wordpress.com/2018/02/25/compiler-bug-linker-bug-windows-kernel-bug/

The Windows kernel has a rare bug which sometimes leads to recently created binaries crashing due to lack of a FlushFileBuffers call in the kernel. This happens when a PE file (DLL or EXE) is created using a file mapping and then is executed shortly afterwards (or LoadLibrary for a DLL). The symptom is that pages of zeroes get mapped in to some of the pages instead of valid code. This is rare and only happens during periods of very high disk activity, but it was seen on about 3-4% of Chrome builds on my workstation when using goma. It shows up when we generate a binary as part of the build (perhaps protoc.exe) and then run it immediately afterwards - occasionally the freshly generated binary crashes.

This bug was confirmed with Microsoft who are working on a fix. The interim workaround is to call FlushFileBuffers on the output file when finished linking it. This workaround was implemented in Chromium using a Python wrapper and this has been 100% effective. The change can be seen here:

https://chromium-review.googlesource.com/c/chromium/src/+/876683/10/

The tracking bug (20+ months in the making) can be found here:

https://bugs.chromium.org/p/chromium/issues/detail?id=644525#c42

A request for the VS team to add this workaround to their linker is tracked in this link - apparently they have added the workaround and it will ship later this year.

https://developercommunity.visualstudio.com/content/problem/191590/linker-needs-workaround-for-windows-kernel-bug.html

It's a nasty bug. Luckily it is quite rare. Luckily it is also quite easy to work around.

@ALTree ALTree changed the title Go linker may be vulnerable to Windows kernel bug cmd/link: Go linker may be vulnerable to Windows kernel bug Feb 28, 2018
@ALTree ALTree added OS-Windows NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 28, 2018
@davecheney
Copy link
Contributor

davecheney commented Feb 28, 2018 via email

@alexbrainman
Copy link
Member

@randomascii could you describe exactly what the problem is, please?

From your https://randomascii.wordpress.com/2018/02/25/compiler-bug-linker-bug-windows-kernel-bug/ you say: The underlying bug is that if a program writes a PE file (EXE or DLL) using memory mapped file I/O and ...

Go linker does not use memory mapped file IO, it opens file with CreateFile and writes to it. Why do you think Go has this bug?

Alex

@randomascii
Copy link
Author

Waiting for a Microsoft fix could take a very long time, especially for older operating systems like Windows 7. But! If the Go linker doesn't use memory mapped file I/O then it is not vulnerable to this bug so this issue can be closed. I filed the issue here because both lld-link and VC++'s link.exe hit the bug so I therefore assumed that most linkers would be vulnerable, but I'm glad to be proved wrong.

@ianlancetaylor
Copy link
Contributor

The Go linker should use memory mapped file I/O, it just doesn't.

I'll go ahead and close this. Thanks for bringing it to our attention.

@golang golang locked and limited conversation to collaborators Feb 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Projects
None yet
Development

No branches or pull requests

6 participants