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

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.

CC @alexbrainman

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

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

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

Can you open a backport issue for 1.12?

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?

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.

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

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?

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.

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

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.

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?

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.

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.

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.

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.

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.

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.

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

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

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?

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?

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

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.

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.

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).

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.

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.

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.

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.

Hah I was just implementing that. CL posted now.

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

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.

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

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

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

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

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