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

[dblohm7]

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.

[ianlancetaylor]

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

[mknyszek]

CC @alexbrainman via https://dev.golang.org/owners

[alexbrainman]

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

Thank you.

Alex

[dblohm7]

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]

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]

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]

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