syscall: GetStartupInfo on Windows checks nonexistent return code

[apenwarr]

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.

[tklauser]

/cc @alexbrainman

[alexbrainman]

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]

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]

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]

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]

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]

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]

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

[gopherbot]

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