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 [1.13 backport] #34388

Closed
gopherbot opened this issue Sep 19, 2019 · 41 comments

Comments

@gopherbot
Copy link

commented Sep 19, 2019

@zx2c4 requested issue #34364 to be considered for backport to the next 1.13 minor release.

@gopherbot please backport this to 1.13

@gopherbot

This comment has been minimized.

Copy link
Author

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

@zx2c4

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

Given that lots of external code uses this, and that it's currently causing memory corruption, and that we need to use it for the development of x/sys itself, this seems like a pretty good thing to backport.

@zx2c4

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

Given that lots of external code uses this, and that it's currently causing memory corruption, and that we need to use it for the development of x/sys itself, this seems like a pretty good thing to backport.

That was broken forever. And all Go developers always use current tip. But it is a small change, and can be easily ported.

I am fine with whatever decision is here. Leaving it to others to decide.

Thank you.

Alex

@zx2c4

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

all Go developers always use current tip.

Not me. And probably not a lot of people who run go generate for Windows stuff.

CC @networkimprov

@networkimprov

This comment has been minimized.

Copy link

commented Sep 19, 2019

I've seen issues where folks indicate that they're still using 1.11.

@networkimprov

This comment has been minimized.

Copy link

commented Sep 23, 2019

Can you open a backport issue for 1.12?

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

On CL 196417, @bradfitz commented:

This is a generator program that doesn't affect compiled programs so it doesn't seem urgent to backport, no?

@zx2c4, @networkimprov: are there specific examples of developers using mksyscall_windows.go for things outside of the Go project's own go and sys repos?

@bcmills bcmills modified the milestones: Go1.13.1, Go1.13.2 Sep 25, 2019
@zx2c4

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

WireGuard uses it extensively. Microsoft's go-winio uses it. Basically any non-trivial Windows program requires it to get stuff done, since x/sys doesn't [try to?] cover everything.

@bcmills bcmills removed the WaitingForInfo label Sep 25, 2019
@alexbrainman

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

are there specific examples of developers using mksyscall_windows.go for things outside of the Go project's own go and sys repos?

I use it for my development. But I always run current Go tip.

Alex

@bcmills

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

Basically any non-trivial Windows program requires it to get stuff done, since x/sys doesn't [try to?] cover everything.

Should we be encouraging folks to expand x/sys/windows rather than using mksyscall_windows.go to build their own ad-hoc system call libraries?

Is there some other restructuring we should do in x/sys to reduce duplication of syscall wrappers across projects?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

I never considered that program part of our API. I thought it was an internal detail for the Go repo's own use.

If people are using it, I agree that it should live elsewhere.

@zx2c4

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2019

FWIW, the mksyscalls.go program has been designed to be used externally, and so people do. There's some code in there to try to figure out the calling context. Here's a file from a high profile Microsoft project:

https://github.com/microsoft/go-winio/blob/master/syscall.go

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

Yes, you already mentioned that Wireguard and go-winio uses it. That's not in dispute.

The question is where it should live. I think it should move to x/sys/windows/mkwinsyscall and we could remove the +build ignore.

@zx2c4

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2019

The question is where it should live. I think it should move to x/sys/windows/mkwinsyscall and we could remove the +build ignore.

That sounds like a good plan. I was playing with that last week trying to figure out how I could invoke that from a go generate directive. We'd want to use go run with a package name, I'm guessing? In which case, that file would go into it's own directory in x/sys/windows?

@bcmills

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

We'd want to use go run with a package name, I'm guessing? In which case, that file would go into [its] own directory in x/sys/windows?

Yep.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

When I said "move to x/sys/windows/mkwinsyscall" I meant that would be the directory name (and thus the binary name if people go install it). The filename could be whatever. Either mkwinsyscall.go or mksyscall_windows.go if we wanted to make it easier to find for people used to the old name. But given there are like only a handful of such people, I think just naming the file mkwinsyscall.go is fine too.

@zx2c4

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

And then the go repo itself would use mksyscall.go from its internal vendored copy of x/sys/windows?


So what if for 1.13.2 we do the backport for the bug fix, since people reference this already using $GOROOT.

Then for 1.14 we remove mksyscall.go from its current location and have the go repo use the vendored internal copy. Meanwhile mksyscall.go then moves to x/sys/windows.

@bcmills

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

And then the go repo itself would use mksyscall.go from its internal vendored copy of x/sys/windows?

More precisely, it could vendor a copy of x/sys/windows/mkwinsyscall.

@bcmills

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

So what if for 1.13.2 we do the backport for the bug fix, since people reference this already using $GOROOT.

Then for 1.14 we remove mksyscall.go from its current location and have the go repo use the vendored internal copy. Meanwhile mksyscall.go then moves to x/sys/windows.

We can add the binary to x/sys right now: nobody would need to wait for the 1.14 release for that.

And if we go ahead and add the binary there, external users can switch to that right away, rather than waiting for—and then needing to upgrade to—Go 1.13.2.

@zx2c4

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

We can add the binary to x/sys right now: nobody would need to wait for the 1.14 release for that.

Right.

And if we go ahead and add the binary there, external users can switch to that right away, rather than waiting for—and then needing to upgrade to—Go 1.13.2.

Just seems like expecting people to re-work their go-generate files during a point release is a bit much and we can pretty easily support this, and then go cold-turkey for 1.14. But however you want is fine with me.

I'll play with the x/sys/windows commit for that.

@gopherbot

This comment has been minimized.

Copy link
Author

commented Oct 3, 2019

Change https://golang.org/cl/198637 mentions this issue: windows: import mksyscall.go from go repo

@gopherbot

This comment has been minimized.

Copy link
Author

commented Oct 3, 2019

Change https://golang.org/cl/198541 mentions this issue: syscall: use mksyscall.go from vendored x/sys/windows

gopherbot pushed a commit to golang/sys that referenced this issue Oct 3, 2019
This allows us to modify this file and fix it more fluidly. Users can
invoke it from go generate via:

   go run golang.org/x/sys/windows/mkwinsyscall

This was taken from Go repo commit 6b85fa80.

Updates golang/go#34388

Change-Id: I8dc39eed96b2499ccbde53554b3e16e6c1f6aa98
Reviewed-on: https://go-review.googlesource.com/c/sys/+/198637
Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@zx2c4

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

https://go-review.googlesource.com/c/go/+/198541 is now underway and things are moving the way suggested above.

So at this point I'd recommend one of these options:

  • Backporting the fix to existing 1.13 users for 1.13.2.
  • Removing mksyscall.go from 1.13.2 to force users to immediately switch over to x/sys/windows/mkwinsyscall.
  • Making some big announcement somewhere (mailing list? dunno) telling people to move to x/sys/windows.

Any of these seem appealing?

@bcmills

This comment has been minimized.

Copy link
Member

commented Oct 4, 2019

Are any of those necessary? I've only seen mention of a few affected projects — probably we should start by opening issues (or PRs) against those specific projects.

What fraction of Windows system calls include *bool parameters, and how many of them are already covered by a well-known Go wrapper library?

@zx2c4

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

Most Windows software is not open source, so hard to tell.

@zx2c4

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

Per the CL, seems like @ianlancetaylor disagreed with the plan here. I'll let you guys decide the future of this and will sit this one out.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

I think we should put a copy of mksyscall_windows.go in golang.org/x/sys and encourage people to start using that one. But I don't think we should remove the one from syscall. We should just deprecate it. Since people are apparently using syscall/mksyscall_windows.go, removing it will just break them for no useful purpose.

@bcmills

This comment has been minimized.

Copy link
Member

commented Oct 4, 2019

Over the long term, I would rather we keep a single source of truth for the mksyscall implementation — I don't think it makes sense to maintain it in two different places.

One alternative might be to make the existing mksyscall_windows.go a thin wrapper — for example, we could factor out an implementation package that both it and x/sys/windows/mkwinsyscall could wrap, or we could make mksyscall_windows.go exec go run x/sys/windows/mkwinsyscall as a subprocess.

That said, absent evidence of more external users, I don't think the increase in compatibility is worth the extra indirection. So far we've identified only two (WireGuard and go-winio).

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

Not breaking people unnecessarily means not breaking people unnecessarily. It's not like this program changes frequently: the last substantive change was in 2016. I see no problem with only updating the version in x/sys. If due to some issue people start running into trouble with the syscall version for a problem that is fixed in the x/sys version, we can simply tell them to start using the x/sys version. In other words, exactly what we do for the syscall package in general: the syscall package is frozen, and anybody who needs a bug fix should use the x/sys package instead. Let's extend that frozenness to mksyscall_windows.go.

@bcmills

This comment has been minimized.

Copy link
Member

commented Oct 4, 2019

I see no problem with only updating the version in x/sys. If due to some issue people start running into trouble with the syscall version for a problem that is fixed in the x/sys version, we can simply tell them to start using the x/sys version.

I think that's a fine policy for features, but this example was a bug-fix that did not change the supported feature set and did not manifest as a compile-time error. If we retain the file, we should also backport fixes for run-time bugs.

My concern is that having a source of truth in multiple places will make it too easy to forget to backport bug-fixes, and only adding features in one place will make those backports less likely to merge cleanly over time.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

One of the major features of Go is stability. That means stability of the entire ecosystem. We don't break people unnecessarily, and this breakage is not necessary.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 4, 2019

What about deleting the bulk of $GOROOT/src/syscall/mksyscall_windows.go and turning it into a functioning wrapper that just calls out to go run golang.org/x/sys/windows/mkwinsyscall? It could even log a warning in there without breaking people. Then we both keep things working for people and also reduce duplication.

@zx2c4

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

Hah I was just implementing that. CL posted now.

@gopherbot

This comment has been minimized.

Copy link
Author

commented Oct 4, 2019

Change https://golang.org/cl/198977 mentions this issue: syscall: move mksyscall_windows.go to x/sys/windows/mkwinsyscall

gopherbot pushed a commit that referenced this issue Oct 4, 2019
We replace the existing file with a thin wrapper around its target so
that we don't break anybody's workflow.

Updates #34388

Change-Id: I0d00371c483cb78f4be18fe987df33c79cd40f05
Reviewed-on: https://go-review.googlesource.com/c/go/+/198977
Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@zx2c4

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

With CL 198977 taken care of now, I've just submitted CL 199137 to take care of this actual issue, the 1.13 backport situation, and close out this issue for good.

@gopherbot

This comment has been minimized.

Copy link
Author

commented Oct 4, 2019

Change https://golang.org/cl/199137 mentions this issue: [release-branch.go1.13] syscall: replace mksyscall_windows.go with wrapper to new x/sys home

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Oct 5, 2019

With CL 198977 taken care of now

After I apply CL 198977, I get this warning

WARNING: Please switch from using:
    go run $GOROOT/src/syscall/mksyscall_windows.go
to using:
    go run golang.org/x/sys/windows/mkwinsyscall

when I run 'go generate' command in my $GOROOT/src/syscall.

We need to do something about that. Should we actually follow this warning advise, and adjust our go generate command?

What is the plan?

Thank you.

Alex

@gopherbot

This comment has been minimized.

Copy link
Author

commented Oct 5, 2019

Change https://golang.org/cl/199277 mentions this issue: syscall, internal/syscall/windows, internal/syscall/windows/registry: make go generate use new golang.org/x/sys/windows/mkwinsyscall

gopherbot pushed a commit that referenced this issue Oct 5, 2019
… make go generate use new golang.org/x/sys/windows/mkwinsyscall

Updates #34388

Change-Id: I327a1c1557c47fa6c113c7a1a507a8e7355f9d1a
Reviewed-on: https://go-review.googlesource.com/c/go/+/199277
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
@gopherbot

This comment has been minimized.

Copy link
Author

commented Oct 7, 2019

Change https://golang.org/cl/199538 mentions this issue: [release-branch.go1.13] syscall: replace mksyscall_windows.go with wrapper to new x/sys home

gopherbot pushed a commit that referenced this issue Oct 7, 2019
…apper to new x/sys home

We replace the existing file with a thin wrapper around its target so
that we don't break anybody's workflow.

This combines CL 198977 and CL 199277.

Fixes #34388

Change-Id: I0d00371c483cb78f4be18fe987df33c79cd40f05
Reviewed-on: https://go-review.googlesource.com/c/go/+/199538
Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot

This comment has been minimized.

Copy link
Author

commented Oct 7, 2019

Closed by merging b7b6e8e to release-branch.go1.13.

@gopherbot gopherbot closed this Oct 7, 2019
@katiehockman katiehockman modified the milestones: Go1.13.2, Go1.13.3 Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.