Repository navigation
Fix out-of-bounds write in dou_dizhu when nobody wins the auction - #1602
Closed
codewithfourtix wants to merge 3 commits into
Closed
codewithfourtix wants to merge 3 commits into
codewithfourtix wants to merge 3 commits into
Conversation
If every player passes during the auction no dizhu is chosen, so dizhu_
stays kInvalidPlayer. OriginalDeal() still gave the three left-over cards
to deal[dizhu_] and so wrote before the start of the array. ScoreUp() and
WriteObservationTensor() already guard the no-dizhu case; OriginalDeal()
did not.
deal is returned by value into FormatDeal()'s frame, so the stray writes
land on FormatDeal()'s own locals. Built with -fsanitize=address,undefined,
formatting a terminal all-pass state crashes:
array:211:9: runtime error: addition of unsigned offset to 0x... overflowed
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior array:211:9
==1142==ERROR: AddressSanitizer: SEGV on unknown address
google-deepmind#4 DouDizhuState::FormatDeal() dou_dizhu.cc:174:21
google-deepmind#5 DouDizhuState::ToString() dou_dizhu.cc:85:20
The crash is inside ToString(), which is what apply_action_test_clone in
games_sim_test.py compares, so this may be the segfault reported in issue
google-deepmind#1358 -- but that failure is non-deterministic and macOS-only, and I could
not reproduce it directly to confirm.
Adds a regression test that plays out an all-pass auction and formats the
terminal state. It crashes under sanitizers before this change and passes
after.
Contributor
|
Thanks, this is great! Just a quick heads up that our GitHub Actions CI is currently broken and we'll have to import #1599 and update github (hopefully later today) before we can run tests, so once we do that I'll have to ask you to merge changes from master and push the merge, then I will trigger the tests. |
lanctot
suggested changes
Aug 31, 2026
GamesWithKnownIssues() now returns an empty list and dou_dizhu is back in the general simulation tests. 2000 random sims under -fsanitize=address,undefined are clean with the OriginalDeal() guard in place.
Contributor
Author
Contributor
|
Great! Can you now pull changes from master and push the merge to this branch? It will fix the Github Actions CI. I will trigger tests after that. |
sylvesterkaczmarek
left a comment
Contributor
There was a problem hiding this comment.
Rechecked the current head after the maintainer follow-up. Guarding the leftover-card assignment on a valid dizhu fixes the OOB without changing normal deals, and the all-pass terminal path now has sanitizer-shaped regression coverage. The requested known-issue and sim-test exemptions are also removed. Looks clean to me.
lanctot
approved these changes
Sep 5, 2026
jhtschultz
pushed a commit
that referenced
this pull request
Oct 20, 2026
-- b5108c0 by Ali Zulfiqar <codewithfourtix@gmail.com>: Fix out-of-bounds write in dou_dizhu when nobody wins the auction If every player passes during the auction no dizhu is chosen, so dizhu_ stays kInvalidPlayer. OriginalDeal() still gave the three left-over cards to deal[dizhu_] and so wrote before the start of the array. ScoreUp() and WriteObservationTensor() already guard the no-dizhu case; OriginalDeal() did not. deal is returned by value into FormatDeal()'s frame, so the stray writes land on FormatDeal()'s own locals. Built with -fsanitize=address,undefined, formatting a terminal all-pass state crashes: array:211:9: runtime error: addition of unsigned offset to 0x... overflowed SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior array:211:9 ==1142==ERROR: AddressSanitizer: SEGV on unknown address #4 DouDizhuState::FormatDeal() dou_dizhu.cc:174:21 #5 DouDizhuState::ToString() dou_dizhu.cc:85:20 The crash is inside ToString(), which is what apply_action_test_clone in games_sim_test.py compares, so this may be the segfault reported in issue #1358 -- but that failure is non-deterministic and macOS-only, and I could not reproduce it directly to confirm. Adds a regression test that plays out an all-pass auction and formats the terminal state. It crashes under sanitizers before this change and passes after. -- 5c6ccf8 by Ali Zulfiqar <codewithfourtix@gmail.com>: Remove dou_dizhu from the known-issues and sim-test exemption lists GamesWithKnownIssues() now returns an empty list and dou_dizhu is back in the general simulation tests. 2000 random sims under -fsanitize=address,undefined are clean with the OriginalDeal() guard in place. COPYBARA_INTEGRATE_REVIEW=#1602 from codewithfourtix:fix/dou-dizhu-oob-when-auction-all-pass 0528c61 PiperOrigin-RevId: 1002639184 Change-Id: I45e2ab46054af4064c1d0f6b8674127940394f5a
Member
|
Merged into master in 2a662b8 |
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
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.
If every player passes the auction no dizhu is chosen and dizhu_ stays kInvalidPlayer, but OriginalDeal() still does deal[dizhu_][cards_left_over_[i]]++, writing before the start of the array. ScoreUp() and WriteObservationTensor() already guard this case, this spot was missed.
It's reachable from ToString() on a terminal state — under -fsanitize=address,undefined that's a SEGV in FormatDeal() at dou_dizhu.cc:174, clean with the guard.
Also adds a regression test that plays out an all-pass auction and formats the terminal state. It crashes under sanitizers before the change, passes after.
Discussed in #1358. I couldn't reproduce that issue's segfault directly (macOS/py3.10 only), so I've left GamesWithKnownIssues() alone.