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

go-fuzz: set fd inheritance properly for Go 1.17+ on Windows #330

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thepudds
Copy link
Collaborator

@thepudds thepudds commented Dec 2, 2021

During Go 1.17 development, fd inheritance on Windows was changed in:

CL 288297 - "syscall: restrict inherited handles on Windows"

As a result, running go-fuzz with Go 1.17 on at least some Windows versions caused errors like:

write to testee failed: write |1: The pipe is being closed

The fix is to properly set SysProcAttr.AdditionalInheritedHandles. The fix here is modeled on the suggestion from Jason Donenfeld in a later CL 320050.

Once the unit tests were updated to run against Go 1.17 but before the fix in this PR, the unit tests failed for Windows on Go 1.17 while passing on Go 1.16. With this PR, the unit tests now pass for Windows on both Go 1.16 & 1.17.

Fixes #328

During Go 1.17 development, fd inheritance on Windows was changed in:
    CL 288297 - "syscall: restrict inherited handles on Windows"
    https://golang.org/cl/288297

Running go-fuzz with Go 1.17 on at least some Windows versions caused errors like:
    "write to testee failed: write |1: The pipe is being closed"

The fix is to properly set SysProcAttr.AdditionalInheritedHandles, which is modeled
after the suggestion from Jason Donenfeld in CL 320050:
    https://go-review.googlesource.com/c/go/+/320050/-1..3#message-ed1be75fda3d32c5ff2bd037b951a875cb07c3db

Fixes dvyukov#328
@klauspost
Copy link

I can confirm this fixes the issue on 1.17.6 windows/amd64

@thepudds
Copy link
Collaborator Author

Hi @dvyukov, just wanted to check in on this. Josh gave it a +1 above, and the original reporter confirmed it fixed it for them, as well as @klauspost above.

Do you want to review this as well, and/or are you OK with me merging it?

Just looking for a brief comment. Many balls in the air of course. 😅

@dvyukov
Copy link
Owner

dvyukov commented Feb 21, 2022

I would move only the AdditionalInheritedHandles to separate files, the rest seems to be the same (?) so no need to duplicate.
But otherwise looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Debugging "restarts: 1/1" on windows/amd64
4 participants