MoqxCache: fix two updateInProgress UB/crash bugs#303
Merged
Conversation
afrind
requested review from
Oxyd,
TimEvens,
akash-a-n,
gmarzot,
michalhosna,
mondain,
peterchave and
suhasHere
as code owners
suhasHere
approved these changes
May 8, 2026
Contributor
suhasHere
left a comment
suhasHere
left a comment
There was a problem hiding this comment.
@suhasHere reviewed 2 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on akash-a-n, gmarzot, michalhosna, mondain, Oxyd, peterchave, and TimEvens).
Branch 3 of updateInProgress() erases the fetchesInProgress entry and sets fetchInProgressIt_=end(). Two distinct bugs follow from this: 1. XCHECK crash (boundary collision): the previous re-key logic would attempt to re-key to maxLocation even when a concurrent fetch had already inserted an entry there. Fix: skip re-key when current >= maxLocation (erase-only), and guard the post-erase path so a second call from endOfFetch() doesn't attempt re-key on end(). 2. UB dereference (double call): fetchInProgressIt_->second.progress was updated before the fetchInProgressIt_ != end() guard, so a second updateInProgress() call (e.g. endOfFetch() after a finFetch=true object) would dereference end(), corrupting the map sentinel. Fix: hoist the guard to cover the progress update, and add a matching guard to the else branch. Add regression tests for both scenarios. The double-call test was verified to abort via a temporary XCHECK before the structural fix. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
afrind
force-pushed
the
fix/moqx-cache-update-in-progress
branch
from
d2a00e6 to
bf020bf
Compare
gmarzot
approved these changes
May 8, 2026
Contributor
gmarzot
left a comment
gmarzot
left a comment
There was a problem hiding this comment.
@gmarzot reviewed 2 files and all commit messages.
Reviewable status:
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Branch 3 of updateInProgress() erases the fetchesInProgress entry and sets fetchInProgressIt_=end(). Two distinct bugs follow from this:
XCHECK crash (boundary collision): the previous re-key logic would attempt to re-key to maxLocation even when a concurrent fetch had already inserted an entry there. Fix: skip re-key when current >= maxLocation (erase-only), and guard the post-erase path so a second call from endOfFetch() doesn't attempt re-key on end().
UB dereference (double call): fetchInProgressIt_->second.progress was updated before the fetchInProgressIt_ != end() guard, so a second updateInProgress() call (e.g. endOfFetch() after a finFetch=true object) would dereference end(), corrupting the map sentinel. Fix: hoist the guard to cover the progress update, and add a matching guard to the else branch.
Add regression tests for both scenarios. The double-call test was verified to abort via a temporary XCHECK before the structural fix.
This change is