revoke: race condition accessing CRLSet
- Dominant language
- Go
- Stars
- 9.5k
- Forks
- 1.2k
- PR merge metrics
- No merged PRs in 30d
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](https://golang.org/doc/go1.6#runtime) [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):
1. If you can safely rely on Go 1.9 and can break your API, you can switch to using [sync.Map](https://golang.org/pkg/sync/#Map).
2. If backwards compatibility is important, you can at least add locking around the `CRLSet[url]` read operation; export `crlLock` (e.g., as `CRLLock`); and document to users that they need to protect access to `CRLSet` using `CRLLock`.
Contributor guide
Assessment
This issue has not been assessed yet.