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

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

Comments

Projects
None yet
4 participants
@rsc
Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2015

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

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 11, 2015

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Aug 11, 2015

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link

commented Aug 11, 2015

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

@gopherbot

This comment has been minimized.

Copy link

commented Aug 16, 2015

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

@bradfitz bradfitz closed this in d931716 Aug 18, 2015

rsc added a commit that referenced this issue Sep 10, 2015

cmd/vet: diagnose plain assignment in copylock detector
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.
You can’t perform that action at this time.