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

syscall: memory corruption in *bool types generated by mksyscall_windows.go #34364

Closed
zx2c4 opened this issue Sep 18, 2019 · 10 comments

Comments

@zx2c4
Copy link
Contributor

commented Sep 18, 2019

This is an issue to track a memory corruption bug in mksyscall_windows.go and its effects various places.

Windows type PBOOL is a pointer to a 4 byte value, where 0 means false and not-0 means true. That means we should use uint32 when passing pointers, not bool, since Go bools are 1 byte, not 4. Previously, we were passing the 1 byte bool to the 4 byte argument, resulting in 3 null bytes being written somewhere bad. I observed this memory corruption in the wild, and it's not pretty.

x/sys/windows currently has one buggy function. However, lots and lots of projects use Go's mksyscall_windows.go -- it was meant for external consumption -- and it's possible that external projects are affected by this. This issue can be used to indicate the problem to these places.

CC @alexbrainman @ianlancetaylor @bradfitz

@zx2c4

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

Fix history as of writing:

For x/sys/windows, my first attempt was to just get rid of *bool and pass an explicit *uint32, breaking the API: https://go-review.googlesource.com/c/sys/+/195839 .

The first attempt was rejected. Reviewers suggested instead I fix the source of the problem directly, mksyscall_windows.go, which I did: https://go-review.googlesource.com/c/go/+/196122 . Then, I was able to regenerate x/sys/windows using it: https://go-review.googlesource.com/c/sys/+/196123 .

zx2c4-bot pushed a commit to WireGuard/wireguard-windows that referenced this issue Sep 18, 2019
Windows type PBOOL is a pointer to a 4 byte value, where 0 means false
and not-0 means true. That means we should use uint32 here, not bool,
since Go bools can be 1 byte. This commit was re-generated using
mksyscall_windows.go from CL 196122.

Updates: golang/go#34364
Change-Id: I8e83b9a09c0b58d14ac9a7dee316553940ac6ee3
@gopherbot

This comment has been minimized.

Copy link

commented Sep 18, 2019

Change https://golang.org/cl/196123 mentions this issue: windows: do not corrupt stack with larger boolean return value

@gopherbot

This comment has been minimized.

Copy link

commented Sep 18, 2019

Change https://golang.org/cl/196122 mentions this issue: syscall: avoid stack overflow on Windows bool pointers

@FiloSottile FiloSottile changed the title memory corruption in *bool types to windows //sys generated functions syscall: memory corruption in *bool types generated by mksyscall_windows.go Sep 18, 2019
@FiloSottile FiloSottile added this to the Go1.14 milestone Sep 18, 2019
zx2c4-bot pushed a commit to WireGuard/wireguard-windows that referenced this issue Sep 19, 2019
Windows type PBOOL is a pointer to a 4 byte value, where 0 means false
and not-0 means true. That means we should use uint32 here, not bool,
since Go bools can be 1 byte. This commit was re-generated using
mksyscall_windows.go from CL 196122.

Updates: golang/go#34364
Change-Id: I8e83b9a09c0b58d14ac9a7dee316553940ac6ee3
gopherbot pushed a commit that referenced this issue Sep 19, 2019
…arameters

Windows type PBOOL is a pointer to a 4 byte value, where 0 means false
and not-0 means true. That means we should use uint32 here, not bool,
since Go bools can be 1 byte. Since a *bool is never a "real" valid
Windows type, converting on both in and out is probably sufficient,
since *bool shouldn't ever be used as something with significance for
its particular address.

Updates: #34364
Change-Id: I4c1b91cd9a39d91e23dae6f894b9a49f7fba2c0a
Reviewed-on: https://go-review.googlesource.com/c/go/+/196122
Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
gopherbot pushed a commit to golang/sys that referenced this issue Sep 19, 2019
Windows type PBOOL is a pointer to a 4 byte value, where 0 means false
and not-0 means true. That means we should use uint32 here, not bool,
since Go bools can be 1 byte. This commit was re-generated using
mksyscall_windows.go from CL 196122.

Updates: golang/go#34364
Change-Id: I8e83b9a09c0b58d14ac9a7dee316553940ac6ee3
Reviewed-on: https://go-review.googlesource.com/c/sys/+/196123
Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
@zx2c4

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

@gopherbot please backport this to 1.13

@gopherbot

This comment has been minimized.

Copy link

commented Sep 19, 2019

Backport issue(s) opened: #34388 (for 1.13).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot

This comment has been minimized.

Copy link

commented Sep 19, 2019

Change https://golang.org/cl/196417 mentions this issue: [release-branch.go1.13] syscall: avoid memory corruption in mksyscall_windows.go with *bool parameters

@networkimprov

This comment has been minimized.

Copy link

commented Sep 19, 2019

Is 1.12 also affected?

@zx2c4

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

Yes.

@networkimprov

This comment has been minimized.

Copy link

commented Sep 20, 2019

@gopherbot please backport this to 1.12

EDIT: not sure why this didn't work...

@dmitshur

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

not sure why this didn't work...

@networkimprov You can only make one backport request per issue at this time, see issue #25574.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.