-
Notifications
You must be signed in to change notification settings - Fork 1.1k
revoke: race condition accessing CRLSet #1031
Description
The Go memory model allows concurrent reads of variables, but a write cannot be concurrent with any other operation whether read or write. This includes maps.
For example, see the Go 1.6 release notes [emphasis added]:
The runtime has added lightweight, best-effort detection of concurrent misuse of maps. As always, if one goroutine is writing to a map, no other goroutine should be reading or writing the map concurrently. If the runtime detects this condition, it prints a diagnosis and crashes the program. The best way to find out more about the problem is to run the program under the race detector, which will more reliably identify the race and give more detail.
Package revoke declares:
var CRLSet = map[string]*pkix.CertificateList{}
var crlLock = new(sync.Mutex)
Within certIsRevokedCRL, crlLock is used to protect delete(CRLSet, url) and CRLSet[url] = crl (i.e., write operations on CRLSet), but there's no protection on the CRLSet[url] read operation at the top of the function.
Moreover, since crlLock isn't exported, there's no way for users of package revoke to safely access CRLSet unless they know there aren't any concurrent certificate revocation checks happening (e.g., through higher-level application mutexes).
Two possible options (not exhaustive):
-
If you can safely rely on Go 1.9 and can break your API, you can switch to using sync.Map.
-
If backwards compatibility is important, you can at least add locking around the
CRLSet[url]read operation; exportcrlLock(e.g., asCRLLock); and document to users that they need to protect access toCRLSetusingCRLLock.