Unsafe concurrent modification of http.DefaultClient

[ktravis]

When the RepositoriesService.DownloadReleaseAsset method is called, the http.Client pointed to by github.Client is modified in order to temporarily override http.Client.CheckRedirect for the span of a single request. A sync.Mutex is used on the github.Client struct to prevent multiple concurrent modifications by the same instance of github.Client - however, if multiple instances refer to the same http.Client and each call this method, it becomes unsafe.

The intent may be that each github.Client receives its own http.Client to mitigate this type of issue, but the fact that github.NewClient specifically defaults to using http.DefaultClient when one is not provided suggests otherwise. This not only affects multiple concurrent github.Client instances, it will also (potentially) impact any other code in the program making use of http.DefaultClient with no indication.

This behavior presented itself in a case where multiple clients were created to download release assets from different sources. The underlying http.Client passed to NewClient was nil in the case that authentication was not required, which led to the case above with http.DefaultClient being shared by multiple github clients. The ultimate effect was that redirect URLs returned by different method calls were mismatched (i.e. the request to download repo A release asset 1 instead returned a URL for downloading repo B's release asset 2), leading to data corruption.

Additionally, I did notice this method which performs a similar modification of the underlying http.Client without locking the client's mutex.

[gmlewis]

So in cases like the one described, wouldn't the correct (and simplest) solution be to explicitly create a new http.Client for each asset downloader (instead of passing in nil and therefore be given the http.DefaultClient)?

If not, how do you propose that this issue be solved?

[ktravis]

Ideally these methods would not need to modify the underlying client for each request in order to achieve the desired behavior. At the very least I think the docs need to be explicit about the modifying behavior - currently passing nil to github.NewClient will make it unsafe for concurrent use with other clients, or with any other code making use of http.DefaultClient. The fact that the problem manifests the way it does with mismatched responses is also a bit more insidious than a simple panic (which would be more straightforward to diagnose).

It might make sense to make a new http.Client in the case that the user passes in nil. It could copy the default transport, since only the CheckRedirect needs to change.