From 4dbdb19c2658e458325e30afec0e80b818b13b22 Mon Sep 17 00:00:00 2001 From: Aaron Klotz Date: Wed, 27 Jul 2022 13:47:28 -0600 Subject: [PATCH] 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 --- net/tshttpproxy/mksyscall.go | 12 ++++ net/tshttpproxy/tshttpproxy_windows.go | 84 ++++++++++++-------------- net/tshttpproxy/zsyscall_windows.go | 81 +++++++++++++++++++++++++ 3 files changed, 133 insertions(+), 44 deletions(-) create mode 100644 net/tshttpproxy/mksyscall.go create mode 100644 net/tshttpproxy/zsyscall_windows.go diff --git a/net/tshttpproxy/mksyscall.go b/net/tshttpproxy/mksyscall.go new file mode 100644 index 000000000..2989748e5 --- /dev/null +++ b/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 diff --git a/net/tshttpproxy/tshttpproxy_windows.go b/net/tshttpproxy/tshttpproxy_windows.go index d647f153b..651970ae9 100644 --- a/net/tshttpproxy/tshttpproxy_windows.go +++ b/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) { - if err := httpOpenProc.Find(); err != nil { - return 0, err - } - r, _, err := httpOpenProc.Call( - uintptr(unsafe.Pointer(userAgent)), - uintptr(getAccessFlag()), - 0, /* WINHTTP_NO_PROXY_NAME */ - 0, /* WINHTTP_NO_PROXY_BYPASS */ - 0) - if r == 0 { - return 0, err - } - return winHTTPInternet(r), nil +func httpOpen() (winHTTPInternet, error) { + return winHTTPOpen( + userAgent, + getAccessFlag(), + nil, /* WINHTTP_NO_PROXY_NAME */ + nil, /* WINHTTP_NO_PROXY_BYPASS */ + 0, + ) } type winHTTPInternet windows.Handle func (hi winHTTPInternet) Close() error { - if err := closeHandleProc.Find(); err != nil { - return err - } - r, _, err := closeHandleProc.Call(uintptr(hi)) - if r == 1 { - return nil - } - return err + return winHTTPCloseHandle(hi) } // 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 { - return "", err - } var out winHTTPProxyInfo - 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 + err := winHTTPGetProxyForURL( + hi, + windows.StringToUTF16Ptr(urlStr), + proxyForURLOpts, + &out, + ) + if err != nil { + return "", err } - return "", err + defer out.free() + return windows.UTF16PtrToString(out.Proxy), nil } func sysAuthHeaderWindows(u *url.URL) (string, error) { diff --git a/net/tshttpproxy/zsyscall_windows.go b/net/tshttpproxy/zsyscall_windows.go new file mode 100644 index 000000000..c07e9ee03 --- /dev/null +++ b/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 +}