CListMempool has data race(s) #642
Comments
peterbourgon
added
bug
hvanz
added
mempool
and removed
needs-triage
3 tasks
github-merge-queue bot
pushed a commit
that referenced
this issue
Jan 12, 2024
…able (#2021) Addresses #642 --- #### PR checklist - [X] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments
mergify bot
pushed a commit
that referenced
this issue
Jan 12, 2024
…able (#2021) Addresses #642 --- #### PR checklist - [X] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments (cherry picked from commit ce6d2fb) # Conflicts: # mempool/clist_mempool_test.go
mergify bot
pushed a commit
that referenced
this issue
Jan 12, 2024
…able (#2021) Addresses #642 --- #### PR checklist - [X] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments (cherry picked from commit ce6d2fb) # Conflicts: # mempool/clist_mempool.go # mempool/clist_mempool_test.go
3 tasks
|
Closed by #2021 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
mempool.CListMempool.Update writes to the height field under the assumption that the updateMtx field on the CListMempool struct has been locked by its caller. But e.g. mempool.CListMempool.resCbFirstTime reads from the height field without taking that lock. And that method is called by reqResCb via abci/client.ReqRes.SetCallback, neither of which take the lock. This is a data race.
The height field (and others) are commented as "atomic" which suggests they are expected to be accessed with sync/atomic functions like mempoolTx.Height. But that's not the case at the moment. If that's the intent, then they should only be accessed with sync/atomic functions, and do not require the updateMtx to be locked. But this kind of mixing of individually-atomic fields and a mutex that governs (apparently arbitrary) fields in the type is basically impossible to get right :) and probably deserves a refactor.
The text was updated successfully, but these errors were encountered: