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

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
Assignees
Labels
Milestone

Comments

@newmanifold
Copy link

@newmanifold newmanifold commented Jan 7, 2020

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

This comment has been minimized.

Copy link
Member

@bcmills 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

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jan 7, 2020

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

This comment has been minimized.

Copy link
Member

@bcmills 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 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 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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 7, 2020

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

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills 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.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 7, 2020

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
@gopherbot gopherbot closed this in 249c85d Jan 7, 2020
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jan 7, 2020

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 7, 2020

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 7, 2020

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
…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
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.