net/http, crypto/tls: cloneTLSConfig is out of sync with tls.Config

[tombergan]

I added a new tls.Config field in Issue #14376
https://tip.golang.org/src/crypto/tls/common.go#L384

but didn't know I needed to update cloneTLSConfig
https://tip.golang.org/src/net/http/transport.go#L2014

Is there a reason to not add a Clone method to tls.Config? Cloning that struct from another package seems brittle. transport.go has a comment about a race between transport.go and tls.Server -- that race seems more simply resolved if tls.Server clones the config before mutating it internally. (There's also no comment on tls.Server saying that it will mutate the provided config, which makes the behavior surprising.)

/cc @bradfitz

[bradfitz]

We could add a Clone method to TLSConfig, but where do we draw the line at adding clone methods to all types? I don't know.

But let's fix up cloneTLSConfig for now. I thought it had a test for testing that all fields are cloned, but maybe I'm remembering a different clone test.

We can keep this bug open for thinking of a better solution (and tests) in Go 1.8.

[gopherbot]

CL https://golang.org/cl/23283 mentions this issue.

[bradfitz]

See also @ianlancetaylor's https://go-review.googlesource.com/24287 (which adds a private *tls.Config.clone method)

[bdarnell]

A tls.Config.Clone method is needed for more than the standard library. Several projects have their own copies of this method, which will need to be updated for Go 1.7 (and again for 1.8 if #16363 goes in).

GRPC recently stopped doing a naive copy of the tls.Config struct to avoid the incorrect copying of a lock, which resulted in bugs due to mutating caller-supplied memory.

[bradfitz]

@tombergan, @bdarnell, yeah, I'm sold at this point. We'll add one early in Go 1.8.