Validate card counts in CalcAllTablesN to prevent segfault on malformed deals#234
Conversation
…ed deals A deal whose hands do not each hold 13 cards would pass convert_from_pbn and reach the multi-threaded solver in CalcAllTablesN, where it could read out of bounds and crash the process. The fault was intermittent -- it depended on the number of tables in the batch and the heap layout, so a malformed deal solved alone returned RETURN_CARD_COUNT (-14) cleanly while the same deal inside a larger batch segfaulted. Reject malformed deals up front in CalcAllTablesN by checking that every hand holds exactly 13 cards (std::popcount over the four suit bitmasks), returning RETURN_CARD_COUNT before any board is dispatched to the solver. This matches the contract the single-board path already enforces. Adds a regression test that a malformed deal raises both solo and inside a batch.
There was a problem hiding this comment.
Pull request overview
This PR prevents intermittent segfaults in batch table calculation (calc_all_tables_pbn / CalcAllTablesN) when a deal is malformed (a hand does not contain 13 cards) by adding an upfront per-hand card-count validation, and adds a Python regression test to ensure the batch path raises an error instead of crashing.
Changes:
- Add an early card-count check in
CalcAllTablesNusingstd::popcountover the suit bitmasks to returnRETURN_CARD_COUNTbefore dispatching work to the multi-threaded solver. - Add a Python regression test that includes a malformed deal inside a multi-deal batch and asserts that the call raises rather than crashing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
library/src/calc_tables.cpp |
Adds upfront 13-cards-per-hand validation in CalcAllTablesN (requires <bit> for std::popcount). |
python/tests/test_calc_tables_regression.py |
Adds regression coverage ensuring malformed deals in batch raise cleanly instead of crashing. |
| # North holds only 12 cards (empty clubs): A5.QJ953.KQ962.(void) | ||
| malformed = "N:A5.QJ953.KQ962. KJT8.K74..AJ62 632.A6.T74.KQ953 Q974.T82.AJ5.T74" | ||
|
|
||
| # Solo: already rejected by the single-board card-count check. |
| with self.assertRaises(RuntimeError): | ||
| calc_all_tables_pbn([malformed]) |
| with self.assertRaises(RuntimeError): | ||
| calc_all_tables_pbn([valid] * 20 + [malformed]) |
| int cardsInHand = 0; | ||
| for (int s = 0; s < DDS_SUITS; s++) | ||
| cardsInHand += std::popcount(dealsp->deals[m].cards[h][s]); | ||
| if (cardsInHand != 13) |
| if (count * dealsp->no_of_tables > MAXNOOFTABLES * DDS_STRAINS) | ||
| return RETURN_TOO_MANY_TABLES; | ||
|
|
||
| // Each hand must hold exactly 13 cards. A malformed deal otherwise reaches |
There was a problem hiding this comment.
Are we sure that this is what we want? I think all hands have the same number of cards and at most 13 cards is the correct criteria as partial deals are allowed.
There was a problem hiding this comment.
+1
I also prefer to hide this feature behind a flag. While this isn't enormously expensive, I don't want to take the performance hit to verify something I already guaranteed some other way. This is a speed-critical library for many use cases.
Summary
calc_all_tables_pbn/CalcAllTablesNcan segfault when a batch contains a malformed deal — one whose hands don't each hold 13 cards. The same deal solved alone returnsRETURN_CARD_COUNT(-14) cleanly.Mechanism
A malformed deal passes
convert_from_pbnand reaches the multi-threaded solver, which reads out of bounds before the per-board card-count check fires. The crash is intermittent — it depends on the number of tables in the batch and the heap layout (a 32-table batch crashes; the same malformed deal in a 2-table batch returns -14).Fix
Validate up front in
CalcAllTablesNthat every hand holds exactly 13 cards (std::popcountover the four suit bitmasks), returningRETURN_CARD_COUNTbefore any board is dispatched to the solver. This matches the contract the single-board path already enforces.Scope notes:
CalcDDtableN) is unchanged — it already returns -14 without crashing.Testing
//python:alltests pass.