net/tshttpproxy: fix incorrect type in Windows implementation, switch to mkwinsyscall, fix memory leak

The definition of winHTTPProxyInfo was using the wrong type (uint16 vs uint32)
for its first field. I fixed that type.

Furthermore, any UTF16 strings returned in that structure must be explicitly
freed. I added code to do this.

Finally, since this is the second time I've seen type safety errors in this code,
I switched the native API calls over to use wrappers generated by mkwinsyscall.
I know that would not have helped prevent the previous two problems, but every
bit helps IMHO.

Updates https://github.com/tailscale/tailscale/issues/4811

Signed-off-by: Aaron Klotz <aaron@tailscale.com>

net/tshttpproxy/mksyscall.go

@@ -0,0 +1,12 @@
+// Copyright (c) 2022 Tailscale Inc & AUTHORS All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package tshttpproxy
+
+//go:generate go run golang.org/x/sys/windows/mkwinsyscall -output zsyscall_windows.go mksyscall.go
+
+//sys globalFree(hglobal winHGlobal) (err error) [failretval==0] = kernel32.GlobalFree
+//sys winHTTPCloseHandle(whi winHTTPInternet) (err error) [failretval==0] = winhttp.WinHttpCloseHandle
+//sys winHTTPGetProxyForURL(whi winHTTPInternet, url *uint16, options *winHTTPAutoProxyOptions, proxyInfo *winHTTPProxyInfo) (err error) [failretval==0] = winhttp.WinHttpGetProxyForUrl
+//sys winHTTPOpen(agent *uint16, accessType uint32, proxy *uint16, proxyBypass *uint16, flags uint32) (whi winHTTPInternet, err error) [failretval==0] = winhttp.WinHttpOpen

net/tshttpproxy/tshttpproxy_windows.go

@@ -26,13 +26,6 @@ import (
 	"tailscale.com/util/cmpver"
 )
 
-var (
-	winHTTP            = windows.NewLazySystemDLL("winhttp.dll")
-	httpOpenProc       = winHTTP.NewProc("WinHttpOpen")
-	closeHandleProc    = winHTTP.NewProc("WinHttpCloseHandle")
-	getProxyForUrlProc = winHTTP.NewProc("WinHttpGetProxyForUrl")
-)
-
 func init() {
 	sysProxyFromEnv = proxyFromWinHTTPOrCache
 	sysAuthHeader = sysAuthHeaderWindows
@@ -116,7 +109,7 @@ func proxyFromWinHTTP(ctx context.Context, urlStr string) (proxy *url.URL, err e
 	runtime.LockOSThread()
 	defer runtime.UnlockOSThread()
 
-	whi, err := winHTTPOpen()
+	whi, err := httpOpen()
 	if err != nil {
 		proxyErrorf("winhttp: Open: %v", err)
 		return nil, err
@@ -178,38 +171,25 @@ func getAccessFlag() uint32 {
 	return flag
 }
 
-func winHTTPOpen() (winHTTPInternet, error) {
+func httpOpen() (winHTTPInternet, error) {
-	if err := httpOpenProc.Find(); err != nil {
+	return winHTTPOpen(
-		return 0, err
+		userAgent,
-	}
+		getAccessFlag(),
-	r, _, err := httpOpenProc.Call(
+		nil, /* WINHTTP_NO_PROXY_NAME */
-		uintptr(unsafe.Pointer(userAgent)),
+		nil, /* WINHTTP_NO_PROXY_BYPASS */
-		uintptr(getAccessFlag()),
+		0,
-		0, /* WINHTTP_NO_PROXY_NAME */
+	)
-		0, /* WINHTTP_NO_PROXY_BYPASS */
-		0)
-	if r == 0 {
-		return 0, err
-	}
-	return winHTTPInternet(r), nil
 }
 
 type winHTTPInternet windows.Handle
 
 func (hi winHTTPInternet) Close() error {
-	if err := closeHandleProc.Find(); err != nil {
+	return winHTTPCloseHandle(hi)
-		return err
-	}
-	r, _, err := closeHandleProc.Call(uintptr(hi))
-	if r == 1 {
-		return nil
-	}
-	return err
 }
 
 // WINHTTP_AUTOPROXY_OPTIONS
 // https://docs.microsoft.com/en-us/windows/win32/api/winhttp/ns-winhttp-winhttp_autoproxy_options
-type autoProxyOptions struct {
+type winHTTPAutoProxyOptions struct {
 	DwFlags                uint32
 	DwAutoDetectFlags      uint32
 	AutoConfigUrl          *uint16
@@ -221,30 +201,46 @@ type autoProxyOptions struct {
 // WINHTTP_PROXY_INFO
 // https://docs.microsoft.com/en-us/windows/win32/api/winhttp/ns-winhttp-winhttp_proxy_info
 type winHTTPProxyInfo struct {
-	AccessType  uint16
+	AccessType  uint32
 	Proxy       *uint16
 	ProxyBypass *uint16
 }
 
-var proxyForURLOpts = &autoProxyOptions{
+type winHGlobal windows.Handle
+
+func globalFreeUTF16Ptr(p *uint16) error {
+	return globalFree((winHGlobal)(unsafe.Pointer(p)))
+}
+
+func (pi *winHTTPProxyInfo) free() {
+	if pi.Proxy != nil {
+		globalFreeUTF16Ptr(pi.Proxy)
+		pi.Proxy = nil
+	}
+	if pi.ProxyBypass != nil {
+		globalFreeUTF16Ptr(pi.ProxyBypass)
+		pi.ProxyBypass = nil
+	}
+}
+
+var proxyForURLOpts = &winHTTPAutoProxyOptions{
 	DwFlags:           winHTTP_AUTOPROXY_ALLOW_AUTOCONFIG | winHTTP_AUTOPROXY_AUTO_DETECT,
 	DwAutoDetectFlags: winHTTP_AUTO_DETECT_TYPE_DHCP, // | winHTTP_AUTO_DETECT_TYPE_DNS_A,
 }
 
 func (hi winHTTPInternet) GetProxyForURL(urlStr string) (string, error) {
-	if err := getProxyForUrlProc.Find(); err != nil {
+	var out winHTTPProxyInfo
+	err := winHTTPGetProxyForURL(
+		hi,
+		windows.StringToUTF16Ptr(urlStr),
+		proxyForURLOpts,
+		&out,
+	)
+	if err != nil {
 		return "", err
 	}
-	var out winHTTPProxyInfo
+	defer out.free()
-	r, _, err := getProxyForUrlProc.Call(
-		uintptr(hi),
-		uintptr(unsafe.Pointer(windows.StringToUTF16Ptr(urlStr))),
-		uintptr(unsafe.Pointer(proxyForURLOpts)),
-		uintptr(unsafe.Pointer(&out)))
-	if r == 1 {
 	return windows.UTF16PtrToString(out.Proxy), nil
-	}
-	return "", err
 }
 
 func sysAuthHeaderWindows(u *url.URL) (string, error) {

net/tshttpproxy/zsyscall_windows.go

@@ -0,0 +1,81 @@
+// Code generated by 'go generate'; DO NOT EDIT.
+
+package tshttpproxy
+
+import (
+	"syscall"
+	"unsafe"
+
+	"golang.org/x/sys/windows"
+)
+
+var _ unsafe.Pointer
+
+// Do the interface allocations only once for common
+// Errno values.
+const (
+	errnoERROR_IO_PENDING = 997
+)
+
+var (
+	errERROR_IO_PENDING error = syscall.Errno(errnoERROR_IO_PENDING)
+	errERROR_EINVAL     error = syscall.EINVAL
+)
+
+// errnoErr returns common boxed Errno values, to prevent
+// allocations at runtime.
+func errnoErr(e syscall.Errno) error {
+	switch e {
+	case 0:
+		return errERROR_EINVAL
+	case errnoERROR_IO_PENDING:
+		return errERROR_IO_PENDING
+	}
+	// TODO: add more here, after collecting data on the common
+	// error values see on Windows. (perhaps when running
+	// all.bat?)
+	return e
+}
+
+var (
+	modkernel32 = windows.NewLazySystemDLL("kernel32.dll")
+	modwinhttp  = windows.NewLazySystemDLL("winhttp.dll")
+
+	procGlobalFree            = modkernel32.NewProc("GlobalFree")
+	procWinHttpCloseHandle    = modwinhttp.NewProc("WinHttpCloseHandle")
+	procWinHttpGetProxyForUrl = modwinhttp.NewProc("WinHttpGetProxyForUrl")
+	procWinHttpOpen           = modwinhttp.NewProc("WinHttpOpen")
+)
+
+func globalFree(hglobal winHGlobal) (err error) {
+	r1, _, e1 := syscall.Syscall(procGlobalFree.Addr(), 1, uintptr(hglobal), 0, 0)
+	if r1 == 0 {
+		err = errnoErr(e1)
+	}
+	return
+}
+
+func winHTTPCloseHandle(whi winHTTPInternet) (err error) {
+	r1, _, e1 := syscall.Syscall(procWinHttpCloseHandle.Addr(), 1, uintptr(whi), 0, 0)
+	if r1 == 0 {
+		err = errnoErr(e1)
+	}
+	return
+}
+
+func winHTTPGetProxyForURL(whi winHTTPInternet, url *uint16, options *winHTTPAutoProxyOptions, proxyInfo *winHTTPProxyInfo) (err error) {
+	r1, _, e1 := syscall.Syscall6(procWinHttpGetProxyForUrl.Addr(), 4, uintptr(whi), uintptr(unsafe.Pointer(url)), uintptr(unsafe.Pointer(options)), uintptr(unsafe.Pointer(proxyInfo)), 0, 0)
+	if r1 == 0 {
+		err = errnoErr(e1)
+	}
+	return
+}
+
+func winHTTPOpen(agent *uint16, accessType uint32, proxy *uint16, proxyBypass *uint16, flags uint32) (whi winHTTPInternet, err error) {
+	r0, _, e1 := syscall.Syscall6(procWinHttpOpen.Addr(), 5, uintptr(unsafe.Pointer(agent)), uintptr(accessType), uintptr(unsafe.Pointer(proxy)), uintptr(unsafe.Pointer(proxyBypass)), uintptr(flags), 0)
+	whi = winHTTPInternet(r0)
+	if whi == 0 {
+		err = errnoErr(e1)
+	}
+	return
+}