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

x/sys/windows: error codes for HRESULTs should be int32, not Handle #48736

Open
dblohm7 opened this issue Oct 1, 2021 · 7 comments
Open

x/sys/windows: error codes for HRESULTs should be int32, not Handle #48736

dblohm7 opened this issue Oct 1, 2021 · 7 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone

Comments

@dblohm7
Copy link

dblohm7 commented Oct 1, 2021

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

$ go version
go version go1.17.1 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/$USER/.cache/go-build"
GOENV="/home/$USER/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/$USER/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/$USER/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17.1"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/$USER/src/tailscale/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build84692255=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Tried to check the return code of a Windows syscall, known to be an HRESULT.

What did you expect to see?

I expect to be able to do things that I can typically do with HRESULTs in C++, like check if they are negative (equivalent to the Windows SDK's FAILED macro), or compare my hresult value to a known HRESULT error code without the compiler complaining about type mismatches.

In Microsoft's Windows SDK for C and C++, HRESULTs are typedef'd as LONG, which on Windows is always an int32. Further details as to why HRESULTs are not actually handles are outlined by Raymond Chen on his blog.

What did you see instead?

Compile errors due to type mismatches, because error codes for HRESULTs are defined as having type Handle, which in turn is defined as uintptr. Error codes for HRESULTs should be int32.

@gopherbot gopherbot added this to the Unreleased milestone Oct 1, 2021
@ianlancetaylor
Copy link
Contributor

I don't see any references to HRESULT in x/sys/windows. Can you show us your code?

@mknyszek mknyszek changed the title x/sys/windows: Error codes for HRESULTs should be int32, not Handle x/sys/windows: error codes for HRESULTs should be int32, not Handle Oct 4, 2021
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 4, 2021
@mknyszek
Copy link
Contributor

mknyszek commented Oct 4, 2021

@mknyszek mknyszek added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Oct 4, 2021
@alexbrainman
Copy link
Member

What @ianlancetaylor said. @dblohm7 please, show us the code.

Thank you.

Alex

@dblohm7
Copy link
Author

dblohm7 commented Oct 5, 2021

package main

import (
	"fmt"

	// Using lxn/win because its COM functions expose raw HRESULTs
	"github.com/lxn/win"
	"golang.org/x/sys/windows"
)

func main() {
	hr := win.OleInitialize()
	switch hr {
	case windows.S_FALSE:
		fmt.Print("Previously initialized in this apartment")
	case windows.RPC_E_CHANGED_MODE:
		fmt.Print("Different concurrency model")
	default:
	}
}

fails to compile with:

./test_windows.go:13:2: invalid case "golang.org/x/sys/windows".S_FALSE in switch on hr (mismatched types "golang.org/x/sys/windows".Handle and win.HRESULT)
./test_windows.go:15:2: invalid case "golang.org/x/sys/windows".RPC_E_CHANGED_MODE in switch on hr (mismatched types "golang.org/x/sys/windows".Handle and win.HRESULT)

Notice that S_FALSE and RPC_E_CHANGED_MODE are among those constants that are for HRESULTs. Because they are typed as windows.Handle, they mismatch with the correct integer representation of an HRESULT, which is equivalent to int32. These constants should get a similar treatment to the windows.NTStatus error codes, IMHO.

@alexbrainman
Copy link
Member

Notice that S_FALSE and RPC_E_CHANGED_MODE are among those constants that are for HRESULTs. Because they are typed as windows.Handle, they mismatch with the correct integer representation of an HRESULT, which is equivalent to int32.

Thanks for explaining @dblohm7. I agree, S_FALSE and RPC_E_CHANGED_MODE consts should be int32, and not windows.Handle type.

These consts type is HRESULT. According to https://docs.microsoft.com/en-us/windows/win32/winprog/windows-data-types HRESULT is LONG, and LONG is int32.

Also if I google, I find

https://docs.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.vsconstants.rpc_e_changed_mode?view=visualstudiosdk-2019

https://docs.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.vsconstants.s_false?view=visualstudiosdk-2019

both say Int32.

S_FALSE and RPC_E_CHANGED_MODE were introduced by https://go-review.googlesource.com/c/sys/+/170918 so leaving for @zx2c4 to decide what to do here.

Thank you.

Alex

@hajimehoshi
Copy link
Member

hajimehoshi commented Jun 1, 2022

On 64bit machines, this code might not work as expected:

r, _, e := procDwmFlush.Call()
if windows.Handle(r) != windows.S_OK {
    return fmt.Errorf("DwmFlush failed: %w", e)
}
return nil

since, IIUC, windows.Handle is 64bit while the original HRESULT is 32bit. Actually we found that DwmFlush returned a non-zero 64bit value on Wine but all the lower 32bits were 0. The error message was "DwmFlush failed: Success". So, we had to cast r to uint32 once and then cast it to windows.Handle in order to compare with windows.S_OK correctly.

EDIT: NVM, DwmFlush's returning value in that environment was non-zero value, but GetLastError returned a zero value. So I should have not used GetLastError.

@hajimehoshi
Copy link
Member

hajimehoshi commented Jun 2, 2022

So I suggest these:

  • Add a new type HRESULT as uint32 or int32
  • Redefine S_OK and other error types with HRESULT instead of Handle

hajimehoshi added a commit to hajimehoshi/ebiten that referenced this issue Jun 2, 2022
The error value are defined as windows.Handle (64bit) in
golang.org/x/sys/windows, but an actual type should be HRESULT (32bit).
This causes an issue that a returning value from a Windows API was
recognized as a non-zero error value though the value was not an error,
when the value's lower 32-bits are all zero.

See also golang/go#48736 (comment)

Updates #2113
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
@seankhliao seankhliao removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Dec 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Projects
Status: Triage Backlog
Development

No branches or pull requests

8 participants