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: must not copy tls.Config #12099
Comments
While you're there: transport_test.go:2648: possible formatting directive in Error call |
Got a repro or race failure? Is the race purely on its copy and not the Transport trying to use the Once I hope? |
Let's leave the test format issues for a separate change. They are not critical; this one is. |
I managed to reproduce. I'll check corp mail later. No corp or tethering on phone. There's a similar case in ListenAndServeTLS. I have one fix but they probably both need same treatment. And a test to ensure new fields added to tls.Config still get copied in the future. |
We should probably update the vet lock checker to look for sync.Once too. I don't quite understand what it's doing now. It looks like maybe it complains about any field with a Lock method. |
CL https://golang.org/cl/13453 mentions this issue. |
CL https://golang.org/cl/13646 mentions this issue. |
It went out of its way to look for implicit assignments but never checked explicit assignments. This detects the root bug for #12099. Change-Id: I6a6e774cc38749ea8be7cfd58ba6421247b67000 Reviewed-on: https://go-review.googlesource.com/13646 Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com> Reviewed-by: Rob Pike <r@golang.org>
net/http/transport says:
But that line
clone := *cfg
is copying the entire tls.Config, including the sync.Once. That's not okay.In particular if you arrange things right, you can get the copy to be going on at about the same time something else is calling cfg.serverInitOnce.Do, and then the write during the mutex races with the read from the copy, causing the race detector to report a race (and it is a real race, too; the copy will never be unlocked and will cause a deadlock).
Reported by a race detector run in a larger Google program.
Need to fix for Go 1.5.
The text was updated successfully, but these errors were encountered: