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: nil panic while doing (*Transport).Clone() #40565

Open
sillydong opened this issue Aug 4, 2020 · 2 comments · May be fixed by #40566
Open

net/http: nil panic while doing (*Transport).Clone() #40565

sillydong opened this issue Aug 4, 2020 · 2 comments · May be fixed by #40566
Milestone

Comments

@sillydong
Copy link

@sillydong sillydong commented Aug 4, 2020

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

$ go version

go version go1.13.14 darwin/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="auto"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/chenzhidong/Library/Caches/go-build"
GOENV="/Users/chenzhidong/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY="github.com/sillydong,git.sillydong.com"
GONOSUMDB="github.com/sillydong,git.sillydong.com"
GOOS="darwin"
GOPATH="/Users/chenzhidong/Src/Go"
GOPRIVATE="github.com/sillydong,git.sillydong.com"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/opt/go/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/opt/go/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
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=/var/folders/jc/wjk1kxrs3vn541l6vf8x0_2c0000gn/T/go-build389228707=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

package client

var (
	DefaultTransport = &http.Transport{
		Proxy: http.ProxyFromEnvironment,
		Dial: (&net.Dialer{
			Timeout:   10 * time.Second,
			KeepAlive: 30 * time.Second,
			DualStack: true,
		}).Dial,
		DialContext: (&net.Dialer{
			Timeout:   10 * time.Second,
			KeepAlive: 30 * time.Second,
			DualStack: true,
		}).DialContext,
		MaxIdleConns:          100,
		IdleConnTimeout:       90 * time.Second,
		TLSHandshakeTimeout:   10 * time.Second,
		ExpectContinueTimeout: 1 * time.Second,
		ResponseHeaderTimeout: 300 * time.Second,
		ForceAttemptHTTP2:     false,
	}

	DefaultTransportInsecureSkipVerify = func() *http.Transport {
		tr := DefaultTransport.Clone()
		tr.TLSClientConfig = &tls.Config{
			InsecureSkipVerify: true,
		}
		return tr
	}()
)

What did you expect to see?

DefaultTransportInsecureSkipVerify should return a *http.Transport with InsecureSkipVerify tls.Config

What did you see instead?

There came a nil panic

panic: runtime error: invalid memory address or nil pointer dereference
--
6 | [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x70bccd]
7 |  
8 | goroutine 1 [running]:
9 | crypto/tls.(*Config).Clone(0x0, 0xc0001d5dc8)
10 | /usr/local/go/src/crypto/tls/common.go:662 +0x6d
11 | net/http.(*Transport).Clone(0xd150e0, 0x60)
12 | /usr/local/go/src/net/http/transport.go:289 +0xbc
13 | pkg.jimu.io/common/client.glob..func1(0xc00016b368)
14 | /project/libs/src/pkg.xxx.io/common/client/transport.go:34 +0x4f
15 | pkg.jimu.io/common/client.init()
16 | /project/libs/src/pkg.xxx.io/common/client/transport.go:39 +0x262
17 | FAIL	pkg.jimu.io/common/auth	0.049s

I checked the Clone function in net/http package

func (t *Transport) Clone() *Transport {
	t.nextProtoOnce.Do(t.onceSetNextProtoDefaults)
	t2 := &Transport{
		Proxy:                  t.Proxy,
		DialContext:            t.DialContext,
		Dial:                   t.Dial,
		DialTLS:                t.DialTLS,
		TLSClientConfig:        t.TLSClientConfig.Clone(),
		TLSHandshakeTimeout:    t.TLSHandshakeTimeout,
		DisableKeepAlives:      t.DisableKeepAlives,
		DisableCompression:     t.DisableCompression,
		MaxIdleConns:           t.MaxIdleConns,
		MaxIdleConnsPerHost:    t.MaxIdleConnsPerHost,
		MaxConnsPerHost:        t.MaxConnsPerHost,
		IdleConnTimeout:        t.IdleConnTimeout,
		ResponseHeaderTimeout:  t.ResponseHeaderTimeout,
		ExpectContinueTimeout:  t.ExpectContinueTimeout,
		ProxyConnectHeader:     t.ProxyConnectHeader.Clone(),
		MaxResponseHeaderBytes: t.MaxResponseHeaderBytes,
		ForceAttemptHTTP2:      t.ForceAttemptHTTP2,
		WriteBufferSize:        t.WriteBufferSize,
		ReadBufferSize:         t.ReadBufferSize,
	}
	if !t.tlsNextProtoWasNil {
		npm := map[string]func(authority string, c *tls.Conn) RoundTripper{}
		for k, v := range t.TLSNextProto {
			npm[k] = v
		}
		t2.TLSNextProto = npm
	}
	return t2
}

when you do Clone() after create the Transport without setting tls.Config, the clone itself directly do the t.TLSClientConfig.Clone() and the t.TLSClientConfig is a nil, and there comes the panic

// Clone returns a shallow clone of c. It is safe to clone a Config that is
// being used concurrently by a TLS client or server.
func (c *Config) Clone() *Config {
	// Running serverInit ensures that it's safe to read
	// SessionTicketsDisabled.
	c.serverInitOnce.Do(func() { c.serverInit(nil) })

	var sessionTicketKeys []ticketKey
	c.mutex.RLock()
	sessionTicketKeys = c.sessionTicketKeys
	c.mutex.RUnlock()

	return &Config{
		Rand:                        c.Rand,
		Time:                        c.Time,
		Certificates:                c.Certificates,
		NameToCertificate:           c.NameToCertificate,
		GetCertificate:              c.GetCertificate,
		GetClientCertificate:        c.GetClientCertificate,
		GetConfigForClient:          c.GetConfigForClient,
		VerifyPeerCertificate:       c.VerifyPeerCertificate,
		RootCAs:                     c.RootCAs,
		NextProtos:                  c.NextProtos,
		ServerName:                  c.ServerName,
		ClientAuth:                  c.ClientAuth,
		ClientCAs:                   c.ClientCAs,
		InsecureSkipVerify:          c.InsecureSkipVerify,
		CipherSuites:                c.CipherSuites,
		PreferServerCipherSuites:    c.PreferServerCipherSuites,
		SessionTicketsDisabled:      c.SessionTicketsDisabled,
		SessionTicketKey:            c.SessionTicketKey,
		ClientSessionCache:          c.ClientSessionCache,
		MinVersion:                  c.MinVersion,
		MaxVersion:                  c.MaxVersion,
		CurvePreferences:            c.CurvePreferences,
		DynamicRecordSizingDisabled: c.DynamicRecordSizingDisabled,
		Renegotiation:               c.Renegotiation,
		KeyLogWriter:                c.KeyLogWriter,
		sessionTicketKeys:           sessionTicketKeys,
	}
}

The Clone function do not check if c is nil.

In comparation, the ProxyConnectHeader.Clone() has a nil check and return nil if h is nil

// Clone returns a copy of h or nil if h is nil.
func (h Header) Clone() Header {
	if h == nil {
		return nil
	}

	// Find total number of values.
	nv := 0
	for _, vv := range h {
		nv += len(vv)
	}
	sv := make([]string, nv) // shared backing array for headers' values
	h2 := make(Header, len(h))
	for k, vv := range h {
		n := copy(sv, vv)
		h2[k] = sv[:n:n]
		sv = sv[n:]
	}
	return h2
}
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 4, 2020

Change https://golang.org/cl/246637 mentions this issue: crypto/tls: returns a copy of c or nil if c is nil while using tls.Config.Clone()

@toothrot toothrot changed the title Fix nil panic while doing http.Transport Clone() net/http: nil panic while doing (*Transport).Clone() Aug 4, 2020
@toothrot toothrot added this to the Backlog milestone Aug 4, 2020
@toothrot
Copy link
Contributor

@toothrot toothrot commented Aug 4, 2020

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.

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