The inflight request counter is unsigned and there are no guards against underflowing
if clearInflightRequest is called too many times.
Details
Details
Without patch:
git checkout master git checkout clear-inflight-underflow avalanche/test/processor_tests.cpp ninja check-avalanche # fails
With patch:
ninja check-avalanche # passes
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- clear-inflight-underflow
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 19090 Build 37928: Build Diff build-without-wallet · build-clang-tidy · build-diff · lint-circular-dependencies · build-debug · build-clang Build 37927: arc lint + arc unit
Event Timeline
| src/avalanche/voterecord.cpp | ||
|---|---|---|
| 105–109 | While the brackets are unnecessary, the linter puts a semicolon on its own line in this case. It looks weird so I picked this instead. Your suggested implementation is incorrect. In the case that count > inflight, you want inflight to be 0. | |
| 110 | The comparison failure can be spurious, but in that case we expect to loop until it's not. Note that registerPoll doesn't return false when compare_exchange_weak fails, but only when the condition on count isn't true. clearInflightRequest doesn't need to return false in a similar way, so having a return value doesn't make sense. | |
Comment Actions
I don't understand what problem this is solving. First, this is using low free gizmo that seem completely unecessary - aren't VoteReccord owned by locks to begin with?
Second, this is true of literally any integer in the codebase. Why is this a problem here specifically?
What problem is any of this solving?
Comment Actions
This was only a concern in relation to changes I suggested in D11474. Since my approach on that is changing, this patch isn't relevant anymore.