syscall: GetStartupInfo on Windows checks nonexistent return code #31316

apenwarr · 2019-04-07T03:16:04Z

What version of Go are you using (`go version`)?

$ go version
go version go1.12.1 linux/amd64

(cross compiling to Windows amd64)

Does this issue reproduce with the latest release?

The relevant code has not changed lately.

What did you do?

    var si syscall.StartupInfo
    err := syscall.GetStartupInfo(&si)
    if err != nil {
            fatal("GetStartupInfo: %v\n", err)
    }

What did you expect to see?

err should always be nil, because according to https://technet.microsoft.com/en-us/ms683230(v=vs.90), "The Unicode version (GetStartupInfoW) does not fail."

What did you see instead?

Under some circumstances (eg. starting from cmd.exe), GetStartupInfo returns syscall.EINVAL. Under other circumstances (eg. starting from WSL's bash), it succeeds.

The implementation of GetStartupInfo() is:

func GetStartupInfo(startupInfo *StartupInfo) (err error) {
        r1, _, e1 := Syscall(procGetStartupInfoW.Addr(), 1, uintptr(unsafe.Pointer(startupInfo)), 0, 0)
        if r1 == 0 {
                if e1 != 0 {
                        err = errnoErr(e1)
                } else {
                        err = EINVAL
                }
        }
        return
}

So EINVAL should only happen if r1==0 && e1==0. As far as I can tell, GetStartupInfoW() is never supposed to return any value, so checking r1 is not the right thing to do here.

Indeed, if I copy this function but disable the error check, the StartupInfo struct seems to be filled correctly even in the error case. So I think the correct fix is to simply stop checking the return code.

If you want to be paranoid, you could verify that StartupInfo.Cb is set to a nonzero value after the syscall finishes.

The text was updated successfully, but these errors were encountered:

tklauser · 2019-04-08T07:19:45Z

/cc @alexbrainman

alexbrainman · 2019-04-09T09:47:26Z

Yes, I agree. Thank you very much for reporting.

If I would add this function again, I would have it return nothing, like

func GetStartupInfo(startupInfo *StartupInfo)

But I don't think we could change function signature. Can we @ianlancetaylor ?

If we have to keep it as is

func GetStartupInfo(startupInfo *StartupInfo) error

than we need to decide what to return.

We could always return nil.

Alternatively we could return what GetLastError Windows API returns.

I am not sure which approach is better.

Regardless, we will need new test with this change, because GetStartupInfo is not used anywhere in standard library. So we should at least somehow test it.

And we should, probably, copy our solution into golang.org/x/sys/windows package too.

Alex

apenwarr · 2019-04-09T19:34:02Z

GetLastError() is going to just be some random number, and/or what it was before you made the call. It's better to just return nil.

…

On Tue, Apr 9, 2019 at 5:50 AM Alex Brainman ***@***.***> wrote: Yes, I agree. Thank you very much for reporting. If I would add this function again, I would have it return nothing, like func GetStartupInfo(startupInfo *StartupInfo) But I don't think we could change function signature. Can we @ianlancetaylor <https://github.com/ianlancetaylor> ? If we have to keep it as is func GetStartupInfo(startupInfo *StartupInfo) error than we need to decide what to return. We could always return nil. Alternatively we could return what GetLastError Windows API returns. I am not sure which approach is better. Regardless, we will need new test with this change, because GetStartupInfo is not used anywhere in standard library. So we should at least somehow test it. And we should, probably, copy our solution into golang.org/x/sys/windows package too. Alex — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31316 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABQcIgC5To2qH7phaxDPYXcFuBY8f4eks5vfGJYgaJpZM4cgiOP> .

alexbrainman · 2019-04-10T09:58:51Z

GetLastError() is going to just be some random number, and/or what it was before you made the call.

How do you know? Value returned by GetLastError is set during GetStartupInfo implementation.

It's better to just return nil.

Maybe.

Alex

ianlancetaylor · 2019-04-10T12:50:43Z

I don't think we should change the function signature of GetStartupInfo, but it would fine to make it always return nil if that is appropriate.

apenwarr · 2019-04-10T14:11:55Z

It’s standard for windows functions to not change GetLastError state if they don’t return an “error” status value. Hence the pattern of only returning the error code if the status code is an error. Since this function is documented to never fail, it seems clear that it has undefined behaviour around setting GetLastError, and well-documented behaviour of not setting any error code. Also, if it was setting GetLastError to zero on success, I would not have experienced the problem (random error codes returned by the Go function) that led me to file this bug in the first place.

…

On Wed, Apr 10, 2019 at 08:52 Ian Lance Taylor ***@***.***> wrote: I don't think we should change the function signature of GetStartupInfo, but it would fine to make it always return nil if that is appropriate. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31316 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABQcEYvbEc9KpUsa8BltNOa6OeIgeZrks5vfd6ZgaJpZM4cgiOP> .

alexbrainman · 2019-04-12T10:12:25Z

I don't think we should change the function signature of GetStartupInfo, but it would fine to make it always return nil if that is appropriate.

Thank you for replying. SGTM

It’s standard for windows functions to not change GetLastError state if they don’t return an “error” status value.

I never seen any of Microsoft Windows API implementations source code, so I would not really know. But pretty much every Windows API changes GetLastError value. So, if the implementation calls other Windows APIs, that would make GetLastError change. But maybe GetStartupInfo implementation does not call any other APIs.

Anyway, it is not important, because we all agreed to have GetStartupInfo return nil. Would you like to send a change? The change would need a test too.

Alex

gopherbot · 2023-08-16T22:03:02Z

Change https://go.dev/cl/520275 mentions this issue: syscall: don't check non-existent return code in GetStartupInfo

gopherbot · 2023-08-16T22:04:48Z

Change https://go.dev/cl/520295 mentions this issue: windows: don't check non-existent return code in GetStartupInfo

Same as CL 520275 did in package syscall. For golang/go#31316 Change-Id: Ie9d8fed7f40b9e562534d5e91488b4ba1ac44f34 Reviewed-on: https://go-review.googlesource.com/c/sys/+/520295 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@google.com> Auto-Submit: Tobias Klauser <tobias.klauser@gmail.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>

tklauser added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows labels Apr 8, 2019

tklauser changed the title ~~Windows syscall.GetStartupInfo checks nonexistent return code~~ syscall: GetStartupInfo on Windows checks nonexistent return code Apr 8, 2019

laboger mentioned this issue Oct 2, 2020

5 of 5 linux/ppc64 builders are missing #41742

Closed

gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022

seankhliao added this to the Unplanned milestone Aug 20, 2022

tklauser self-assigned this Aug 17, 2023

tklauser added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 17, 2023

dmitshur modified the milestones: Unplanned, Go1.22 Aug 17, 2023

gopherbot closed this as completed in 51fd0cb Aug 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

syscall: GetStartupInfo on Windows checks nonexistent return code #31316

syscall: GetStartupInfo on Windows checks nonexistent return code #31316

apenwarr commented Apr 7, 2019 •

edited

tklauser commented Apr 8, 2019

alexbrainman commented Apr 9, 2019

apenwarr commented Apr 9, 2019 via email

alexbrainman commented Apr 10, 2019

ianlancetaylor commented Apr 10, 2019

apenwarr commented Apr 10, 2019 via email

alexbrainman commented Apr 12, 2019

gopherbot commented Aug 16, 2023

gopherbot commented Aug 16, 2023

syscall: GetStartupInfo on Windows checks nonexistent return code #31316

syscall: GetStartupInfo on Windows checks nonexistent return code #31316

Comments

apenwarr commented Apr 7, 2019 • edited

What version of Go are you using (go version)?

Does this issue reproduce with the latest release?

What did you do?

What did you expect to see?

What did you see instead?

tklauser commented Apr 8, 2019

alexbrainman commented Apr 9, 2019

apenwarr commented Apr 9, 2019 via email

alexbrainman commented Apr 10, 2019

ianlancetaylor commented Apr 10, 2019

apenwarr commented Apr 10, 2019 via email

alexbrainman commented Apr 12, 2019

gopherbot commented Aug 16, 2023

gopherbot commented Aug 16, 2023

apenwarr commented Apr 7, 2019 •

edited

What version of Go are you using (`go version`)?