Don't mutate registered tls configs

[rebeccachin]

Signed-off-by: Dave Protasowski [email protected]

Description

tls.Configs in the registry are no longer mutated. They are cloned. This will fix the issue #536

Checklist

• Code compiles correctly
• Created tests which fail without the change (if possible)
• All tests passing
• Added myself / the copyright holder to the AUTHORS file

[coveralls]

Coverage decreased (-0.2%) to 74.358% when pulling 6898fad on pivotal-rebecca-chin:master into 382e13d on go-sql-driver:master.

[julienschmidt]

This unfortunately breaks support for all Go versions prior to Go 1.8, as https://golang.org/pkg/crypto/tls/#Config.Clone seems to be available only in Go 1.8+

[julienschmidt]

Please keep this list sorted too (P < S)

[coveralls]

Coverage increased (+0.9%) to 75.439% when pulling 077c278 on pivotal-rebecca-chin:master into 382e13d on go-sql-driver:master.

[coveralls]

Coverage increased (+0.9%) to 75.439% when pulling 60eff1f on pivotal-rebecca-chin:master into 382e13d on go-sql-driver:master.

[julienschmidt]

The amount of clutter all these files introduce is unacceptable in my opinion.

Can we maybe use something as simple as the following to create a shallow copy or do we get some problems with unexported fields (pointers) that also get copied?
Basically we can guarantee that the original struct is never used, thus it shouldn't be initialized with some internal state, right?

func cloneTLSConfig(cfg *tls.Config) *tls.Config {
	clone := *cfg
	return &clone
}

[julienschmidt]

To clarify, with Go 1.8+ we should use the Clone function, but is it possible the we break something with shallow copies pre Go 1.8?

[julienschmidt]

A shallow copy seems to be sufficient: https://play.golang.org/p/d6h9AvM6kI (compare with unexported fields in https://github.com/golang/go/blob/release-branch.go1.7/src/crypto/tls/common.go#L390)

My suggestion would be to create 2 versions of the clone function: One for Go 1.8+ in utils_go18.go where the new Clone function is used and a legacy version in utils_legacy.go where a simple shallow copy is used.

[dprotaso]

Take a look at this example: https://play.golang.org/p/dqYUYAJEfk

It illustrates when you copy the struct you are also copying the state of the mutex. Hence if the mutex is locked, when the struct is being copied, the program will deadlock.

This was our motivation for not using the raw struct copy (ie. clone := *cfg).

[julienschmidt]

But why should the mutex on the original struct ever be locked? We just use it to clone it

[dprotaso]

But why should the mutex on the original struct ever be locked?

That mutex is being used here by this exported method. Thus it could be called at any point.
https://github.com/golang/go/blob/release-branch.go1.7/src/crypto/tls/common.go#L499

We're flexible and can simplify the implementation as you suggested - but be aware a deadlock could occur.

[julienschmidt]

I think adding a note to the doc of RegisterTLSConfig that the TLS config is exclusively owned by the driver after registering it should be sufficient.

[coveralls]

Coverage increased (+0.08%) to 74.658% when pulling 7187728 on pivotal-rebecca-chin:master into 382e13d on go-sql-driver:master.

[dprotaso]

FYI: govet failed for go1.7 due to the sync copies - we'll add an additional util for go1.7

[coveralls]

Coverage increased (+0.3%) to 74.89% when pulling 6e1e05f on pivotal-rebecca-chin:master into 382e13d on go-sql-driver:master.

[julienschmidt]

The stdlib actually contained the same workaround until the addition of Config.Clone: golang/go@d24f446#diff-4d34e6d5bd560e4ac6fd23ee5d9bda5fL119

But I think it isn't worth trying to silence go vet with some kind of workaround... so another special case for Go 1.7 😒

[julienschmidt]

Please add the following copyright header to the new files:

// Go MySQL Driver - A MySQL-Driver for Go's database/sql package
//
// Copyright 2017 The Go-MySQL-Driver Authors. All rights reserved.
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// You can obtain one at http://mozilla.org/MPL/2.0/.

[coveralls]

Coverage increased (+0.3%) to 74.89% when pulling ef7511c on pivotal-rebecca-chin:master into 382e13d on go-sql-driver:master.

[julienschmidt]

Please move them to the top, i.e. above the build tags (like https://github.com/go-sql-driver/mysql/pull/603/files)

[coveralls]

Coverage increased (+0.3%) to 74.89% when pulling 2c63450 on pivotal-rebecca-chin:master into 382e13d on go-sql-driver:master.

[julienschmidt]

Thanks for contributing!