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.
When the
RepositoriesService.DownloadReleaseAssetmethod is called, thehttp.Clientpointed to bygithub.Clientis modified in order to temporarily overridehttp.Client.CheckRedirectfor the span of a single request. Async.Mutexis used on thegithub.Clientstruct to prevent multiple concurrent modifications by the same instance ofgithub.Client- however, if multiple instances refer to the samehttp.Clientand each call this method, it becomes unsafe.The intent may be that each
github.Clientreceives its ownhttp.Clientto mitigate this type of issue, but the fact thatgithub.NewClientspecifically defaults to usinghttp.DefaultClientwhen one is not provided suggests otherwise. This not only affects multiple concurrentgithub.Clientinstances, it will also (potentially) impact any other code in the program making use ofhttp.DefaultClientwith no indication.This behavior presented itself in a case where multiple clients were created to download release assets from different sources. The underlying
http.Clientpassed toNewClientwasnilin the case that authentication was not required, which led to the case above withhttp.DefaultClientbeing 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 repoArelease asset1instead returned a URL for downloading repoB's release asset2), leading to data corruption.Additionally, I did notice this method which performs a similar modification of the underlying
http.Clientwithout locking the client's mutex.