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

Jump to bottom

net/http: racing write to t.ProxyConnectHeader in dialConn when proxy URL includes auth credentials #36431

Closed
newmanifold opened this issue Jan 7, 2020 · 9 comments
Closed

net/http: racing write to t.ProxyConnectHeader in dialConn when proxy URL includes auth credentials #36431

newmanifold opened this issue Jan 7, 2020 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.14

Comments

@newmanifold
Copy link

newmanifold commented Jan 7, 2020 •
edited by bcmills

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

$ go version
go version go1.13.5 linux/amd64

Does this issue reproduce with the latest release?

1.13.5, yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/nuts/.cache/go-build"
GOENV="/home/nuts/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/nuts/golang-test/go-src"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/nuts/golang-test/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/nuts/golang-test/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build701551313=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Ran this code. although it uses elazarl/goproxy package, behavour can be reproduced without this package.

package main

import (
	"net/http"
	"net/url"
	"time"
	"sync"
	"io/ioutil"
	"github.com/elazarl/goproxy"
	"fmt"
)

func ProxyFunc(req *http.Request) (*url.URL, error) {
	return url.Parse(req.RemoteAddr)
}

func prepareTransport() *http.Transport {
	return &http.Transport{
		Proxy: ProxyFunc,
		//DisableKeepAlives: true,
		ProxyConnectHeader: http.Header{
			"User-Agent": { "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:71.0) Gecko/20100101 Firefox/71.0" },
			"Proxy-Connection": {"keep-alive"},
			"Connection": {"keep-alive"},
		},
	}
}

func sendRequest(client *http.Client, proxy *url.URL, u *url.URL) {
	for {
		req := http.Request{
			URL: u,
			RemoteAddr: proxy.String(),
			Method: "GET",
		}

		if res, err := client.Do(&req); err == nil {
			defer res.Body.Close()
			_, _ = ioutil.ReadAll(res.Body)

		}
	}
}

func spawnProxies(port string) {
	proxy := goproxy.NewProxyHttpServer()
	proxy.Verbose = false
	fmt.Println(http.ListenAndServe(":" + port, proxy))
}

func main() {
	numProxies := 4
	client := http.Client{
		Transport: prepareTransport(),
		Timeout: time.Second * time.Duration(5),
	}

	u, _ := url.Parse("https://httpbin.org/")

	proxyPorts := make([]string, 0, numProxies)

	for i := 0; i < numProxies; i++ {
		port := fmt.Sprint("800", i)
		proxyPorts = append(proxyPorts, port)
		go spawnProxies(port)
	}

	var wg sync.WaitGroup

	wg.Add(numProxies)

	for _, port := range proxyPorts {
		proxy, _ := url.Parse("http://test:test@127.0.0.1:" + port)
		go func() {
			defer wg.Done()
			sendRequest(&client, proxy, u)
		}()
	}

	wg.Wait()
}

What did you expect to see?

Above code to run without data race warning.

What did you see instead?

==================
WARNING: DATA RACE
Write at 0x00c00008b530 by goroutine 22:
  runtime.mapassign_faststr()
      /home/nuts/golang-test/go/src/runtime/map_faststr.go:202 +0x0
  net/http.(*Transport).dialConn()
      /home/nuts/golang-test/go/src/net/textproto/header.go:22 +0x1d4f
  net/http.(*Transport).dialConnFor()
      /home/nuts/golang-test/go/src/net/http/transport.go:1308 +0x14f

Previous write at 0x00c00008b530 by goroutine 21:
  runtime.mapassign_faststr()
      /home/nuts/golang-test/go/src/runtime/map_faststr.go:202 +0x0
  net/http.(*Transport).dialConn()
      /home/nuts/golang-test/go/src/net/textproto/header.go:22 +0x1d4f
  net/http.(*Transport).dialConnFor()
      /home/nuts/golang-test/go/src/net/http/transport.go:1308 +0x14f

Goroutine 22 (running) created at:
  net/http.(*Transport).queueForDial()
      /home/nuts/golang-test/go/src/net/http/transport.go:1277 +0x68a
  net/http.(*Transport).getConn()
      /home/nuts/golang-test/go/src/net/http/transport.go:1231 +0x785
  net/http.(*Transport).roundTrip()
      /home/nuts/golang-test/go/src/net/http/transport.go:522 +0x83d
  net/http.(*Transport).RoundTrip()
      /home/nuts/golang-test/go/src/net/http/roundtrip.go:17 +0x42
  net/http.send()
      /home/nuts/golang-test/go/src/net/http/client.go:250 +0x6a8
  net/http.(*Client).send()
      /home/nuts/golang-test/go/src/net/http/client.go:174 +0x1ca
  net/http.(*Client).do()
      /home/nuts/golang-test/go/src/net/http/client.go:641 +0x2cc
  main.sendRequest()
      /home/nuts/golang-test/go/src/net/http/client.go:509 +0x11f
  main.main.func1()
      /home/nuts/golang-test/t.go:74 +0x80

Goroutine 21 (running) created at:
  net/http.(*Transport).queueForDial()
      /home/nuts/golang-test/go/src/net/http/transport.go:1277 +0x68a
  net/http.(*Transport).getConn()
      /home/nuts/golang-test/go/src/net/http/transport.go:1231 +0x785
  net/http.(*Transport).roundTrip()
      /home/nuts/golang-test/go/src/net/http/transport.go:522 +0x83d
  net/http.(*Transport).RoundTrip()
      /home/nuts/golang-test/go/src/net/http/roundtrip.go:17 +0x42
  net/http.send()
      /home/nuts/golang-test/go/src/net/http/client.go:250 +0x6a8
  net/http.(*Client).send()
      /home/nuts/golang-test/go/src/net/http/client.go:174 +0x1ca
  net/http.(*Client).do()
      /home/nuts/golang-test/go/src/net/http/client.go:641 +0x2cc
  main.sendRequest()
      /home/nuts/golang-test/go/src/net/http/client.go:509 +0x11f
  main.main.func1()
      /home/nuts/golang-test/t.go:74 +0x80
==================
==================
WARNING: DATA RACE
Write at 0x00c0000b84f0 by goroutine 22:
  net/http.(*Transport).dialConn()
      /home/nuts/golang-test/go/src/net/textproto/header.go:22 +0x1d67
  net/http.(*Transport).dialConnFor()
      /home/nuts/golang-test/go/src/net/http/transport.go:1308 +0x14f

Previous write at 0x00c0000b84f0 by goroutine 21:
  net/http.(*Transport).dialConn()
      /home/nuts/golang-test/go/src/net/textproto/header.go:22 +0x1d67
  net/http.(*Transport).dialConnFor()
      /home/nuts/golang-test/go/src/net/http/transport.go:1308 +0x14f

Goroutine 22 (running) created at:
  net/http.(*Transport).queueForDial()
      /home/nuts/golang-test/go/src/net/http/transport.go:1277 +0x68a
  net/http.(*Transport).getConn()
      /home/nuts/golang-test/go/src/net/http/transport.go:1231 +0x785
  net/http.(*Transport).roundTrip()
      /home/nuts/golang-test/go/src/net/http/transport.go:522 +0x83d
  net/http.(*Transport).RoundTrip()
      /home/nuts/golang-test/go/src/net/http/roundtrip.go:17 +0x42
  net/http.send()
      /home/nuts/golang-test/go/src/net/http/client.go:250 +0x6a8
  net/http.(*Client).send()
      /home/nuts/golang-test/go/src/net/http/client.go:174 +0x1ca
  net/http.(*Client).do()
      /home/nuts/golang-test/go/src/net/http/client.go:641 +0x2cc
  main.sendRequest()
      /home/nuts/golang-test/go/src/net/http/client.go:509 +0x11f
  main.main.func1()
      /home/nuts/golang-test/t.go:74 +0x80

Goroutine 21 (running) created at:
  net/http.(*Transport).queueForDial()
      /home/nuts/golang-test/go/src/net/http/transport.go:1277 +0x68a
  net/http.(*Transport).getConn()
      /home/nuts/golang-test/go/src/net/http/transport.go:1231 +0x785
  net/http.(*Transport).roundTrip()
      /home/nuts/golang-test/go/src/net/http/transport.go:522 +0x83d
  net/http.(*Transport).RoundTrip()
      /home/nuts/golang-test/go/src/net/http/roundtrip.go:17 +0x42
  net/http.send()
      /home/nuts/golang-test/go/src/net/http/client.go:250 +0x6a8
  net/http.(*Client).send()
      /home/nuts/golang-test/go/src/net/http/client.go:174 +0x1ca
  net/http.(*Client).do()
      /home/nuts/golang-test/go/src/net/http/client.go:641 +0x2cc
  main.sendRequest()
      /home/nuts/golang-test/go/src/net/http/client.go:509 +0x11f
  main.main.func1()
      /home/nuts/golang-test/t.go:74 +0x80
@bcmills bcmills changed the title Data Race on http transport.go net/http: data race in dialConn when dialing proxied connections Jan 7, 2020
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 7, 2020
@bcmills
Copy link
Contributor

bcmills commented Jan 7, 2020

Thanks for the report. I can reproduce this with both go1.13.5 and gotip using the provided program.

Since this bug is already present in 1.13, I'm tentatively milestoning it for 1.15, but if the fix is small it might make 1.14 (and might be a backport candidate for a patch release either way).

CC @bradfitz

@bcmills bcmills added this to the Go1.15 milestone Jan 7, 2020
@bcmills
Copy link
Contributor

bcmills commented Jan 7, 2020 •
edited

It is also worth noting that the stack traces in the race report seem to be missing at least one inlined stack frame. That is probably a compiler and/or runtime bug.

CC @dvyukov @randall77 @aclements

[Update: this is #19749.]

@bcmills bcmills self-assigned this Jan 7, 2020
@bcmills
Copy link
Contributor

bcmills commented Jan 7, 2020

Here is a race report with inlining disabled, using a current gotip (after go1.14beta1).

==================
WARNING: DATA RACE
Write at 0x00c0001324f0 by goroutine 17:
  net/textproto.MIMEHeader.Set()
      /usr/local/google/home/bcmills/go/src/net/textproto/header.go:22 +0xe3
  net/http.Header.Set()
      /usr/local/google/home/bcmills/go/src/net/http/header.go:37 +0x60
  net/http.(*Transport).dialConn()
      /usr/local/google/home/bcmills/go/src/net/http/transport.go:1569 +0x1828
  net/http.(*Transport).dialConnFor()
      /usr/local/google/home/bcmills/go/src/net/http/transport.go:1357 +0x14f

Previous write at 0x00c0001324f0 by goroutine 15:
  net/textproto.MIMEHeader.Set()
      /usr/local/google/home/bcmills/go/src/net/textproto/header.go:22 +0xe3
  net/http.Header.Set()
      /usr/local/google/home/bcmills/go/src/net/http/header.go:37 +0x60
  net/http.(*Transport).dialConn()
      /usr/local/google/home/bcmills/go/src/net/http/transport.go:1569 +0x1828
  net/http.(*Transport).dialConnFor()
      /usr/local/google/home/bcmills/go/src/net/http/transport.go:1357 +0x14f

Goroutine 17 (running) created at:
  net/http.(*Transport).queueForDial()
      /usr/local/google/home/bcmills/go/src/net/http/transport.go:1326 +0x592
  net/http.(*Transport).getConn()
      /usr/local/google/home/bcmills/go/src/net/http/transport.go:1280 +0x6c0
  net/http.(*Transport).roundTrip()
      /usr/local/google/home/bcmills/go/src/net/http/transport.go:544 +0x7b4
  net/http.(*Transport).RoundTrip()
      /usr/local/google/home/bcmills/go/src/net/http/roundtrip.go:17 +0x42
  net/http.send()
      /usr/local/google/home/bcmills/go/src/net/http/client.go:252 +0x24c
  net/http.(*Client).send()
      /usr/local/google/home/bcmills/go/src/net/http/client.go:176 +0x196
  net/http.(*Client).do()
      /usr/local/google/home/bcmills/go/src/net/http/client.go:692 +0x2eb
  net/http.(*Client).Do()
      /usr/local/google/home/bcmills/go/src/net/http/client.go:560 +0x42
  main.sendRequest()
      /tmp/tmp.jpcyEnuDdq/36431/main.go:37 +0x134
  main.main.func1()
      /tmp/tmp.jpcyEnuDdq/36431/main.go:76 +0x89

Goroutine 15 (running) created at:
  net/http.(*Transport).queueForDial()
      /usr/local/google/home/bcmills/go/src/net/http/transport.go:1326 +0x592
  net/http.(*Transport).getConn()
      /usr/local/google/home/bcmills/go/src/net/http/transport.go:1280 +0x6c0
  net/http.(*Transport).roundTrip()
      /usr/local/google/home/bcmills/go/src/net/http/transport.go:544 +0x7b4
  net/http.(*Transport).RoundTrip()
      /usr/local/google/home/bcmills/go/src/net/http/roundtrip.go:17 +0x42
  net/http.send()
      /usr/local/google/home/bcmills/go/src/net/http/client.go:252 +0x24c
  net/http.(*Client).send()
      /usr/local/google/home/bcmills/go/src/net/http/client.go:176 +0x196
  net/http.(*Client).do()
      /usr/local/google/home/bcmills/go/src/net/http/client.go:692 +0x2eb
  net/http.(*Client).Do()
      /usr/local/google/home/bcmills/go/src/net/http/client.go:560 +0x42
  main.sendRequest()
      /tmp/tmp.jpcyEnuDdq/36431/main.go:37 +0x134
  main.main.func1()
      /tmp/tmp.jpcyEnuDdq/36431/main.go:76 +0x89
==================

@bcmills bcmills mentioned this issue Jan 7, 2020
Closed
@bcmills bcmills changed the title net/http: data race in dialConn when dialing proxied connections net/http: racing write to t.ProxyConnectHeader in dialConn Jan 7, 2020
@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 7, 2020
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 7, 2020
@bcmills bcmills changed the title net/http: racing write to t.ProxyConnectHeader in dialConn net/http: racing write to t.ProxyConnectHeader in dialConn when proxy URL includes auth credentials Jan 7, 2020
@gopherbot
Copy link

Change https://golang.org/cl/213638 mentions this issue: net/http: avoid writing to Transport.ProxyConnectHeader

@bcmills
Copy link
Contributor

bcmills commented Jan 7, 2020

@gopherbot, please backport to Go 1.13 and 1.12.

This is a data race in net/http, and the fix is straightforward.

This was referenced Jan 7, 2020
Closed
Closed
@gopherbot
Copy link

Backport issue(s) opened: #36433 (for 1.12), #36434 (for 1.13).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@bcmills bcmills modified the milestones: Go1.15, Go1.14 Jan 7, 2020
@bcmills
Copy link
Contributor

bcmills commented Jan 7, 2020

This should be fixed in the next 1.14 build (beta2 or rc1).

@gopherbot
Copy link

Change https://golang.org/cl/213657 mentions this issue: [release-branch.go1.13] net/http: avoid writing to Transport.ProxyConnectHeader

@gopherbot
Copy link

Change https://golang.org/cl/213677 mentions this issue: [release-branch.go1.12] net/http: avoid writing to Transport.ProxyConnectHeader

gopherbot pushed a commit that referenced this issue Jan 7, 2020
[release-branch.go1.13] net/http: avoid writing to Transport.ProxyCon…
367da16 
…nectHeader

Previously, we accidentally wrote the Proxy-Authorization header for
the initial CONNECT request to the shared ProxyConnectHeader map when
it was non-nil.

Updates #36431
Fixes #36434

Change-Id: I5cb414f391dddf8c23d85427eb6973f14c949025
Reviewed-on: https://go-review.googlesource.com/c/go/+/213638
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit 249c85d)
Reviewed-on: https://go-review.googlesource.com/c/go/+/213657
gopherbot pushed a commit that referenced this issue Jan 7, 2020
[release-branch.go1.12] net/http: avoid writing to Transport.ProxyCon…
c5af2aa 
…nectHeader

Previously, we accidentally wrote the Proxy-Authorization header for
the initial CONNECT request to the shared ProxyConnectHeader map when
it was non-nil.

Updates #36431
Fixes #36433

Change-Id: I5cb414f391dddf8c23d85427eb6973f14c949025
Reviewed-on: https://go-review.googlesource.com/c/go/+/213638
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit 249c85d)
Reviewed-on: https://go-review.googlesource.com/c/go/+/213677
@golang golang locked and limited conversation to collaborators Jan 6, 2021
@rsc rsc unassigned bcmills Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.14
Development

No branches or pull requests
3 participants
@bcmills @gopherbot @newmanifold