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: must not copy tls.Config #12099

Closed
rsc opened this issue Aug 10, 2015 · 7 comments
Closed

net/http: must not copy tls.Config #12099

rsc opened this issue Aug 10, 2015 · 7 comments
Labels
FrozenDueToAge
Milestone
Go1.5

Comments

@rsc
Copy link
Contributor

rsc commented Aug 10, 2015

net/http/transport says:

    cfg := t.TLSClientConfig
    if cfg == nil || cfg.ServerName == "" {
        host := cm.tlsHost()
        if cfg == nil {
            cfg = &tls.Config{ServerName: host}
        } else {
            clone := *cfg // shallow clone
            clone.ServerName = host
            cfg = &clone
        }
    }

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.
@rsc rsc added this to the Go1.5 milestone Aug 10, 2015
@robpike
Copy link
Contributor

robpike commented Aug 11, 2015

While you're there: transport_test.go:2648: possible formatting directive in Error call

@bradfitz
Copy link
Contributor

Got a repro or race failure?

Is the race purely on its copy and not the Transport trying to use the Once I hope?

@rsc
Copy link
Contributor Author

rsc commented Aug 11, 2015

Let's leave the test format issues for a separate change. They are not critical; this one is.
Brad, I sent you more details in email.

@bradfitz
Copy link
Contributor

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.

@rsc
Copy link
Contributor Author

rsc commented Aug 11, 2015

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.

@gopherbot
Copy link

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

@gopherbot
Copy link

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

rsc added a commit that referenced this issue Sep 10, 2015
@rsc @robpike
cmd/vet: diagnose plain assignment in copylock detector
d8384e9 
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>
@golang golang locked and limited conversation to collaborators Aug 22, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.5
Development

No branches or pull requests
4 participants
@bradfitz @rsc @robpike @gopherbot