tests: close two IP conflict coverage gaps from #482's review - #486
Merged
Merged
Conversation
- #482's reviewer named two untested branches of the new overlap-conflict logic, neither a suspected bug: no test had two BUFIDs genuinely in flight at once, and no test drove one fragment's comparison pass through two non-adjacent conflict runs. - Add test_conflict_stays_scoped_to_its_own_bufid: interleaves two datagrams' fragments, conflicts one, and asserts the other's `conflict` stays empty. This is the higher-value gap -- #478's original six boundary cases all shared one ACK bucket, which is exactly the shape that let a cross-bucket regression through undetected. - Add test_single_write_produces_two_conflict_runs_separated_by_a_match: one fragment write disagrees at octets 8-15 and 24-31 but agrees at 16-23, exercising _detect_conflicts' break-on-match-and-resume path. - Verified each test by temporarily breaking the behaviour it targets (shared conflict list across BUFIDs; break after the first conflict run) and confirming the test fails, then restored the source. Build/test: tests/foundation/reassembly/test_ip.py 16 passed; tests/foundation/reassembly/ 65 passed; tests/foundation/ 202 passed, 11 skipped.
Owner
Author
|
Landed directly on Closing by hand because GitHub does not mark a PR merged when its commits arrive via a push rather than the merge button, even though they are genuinely on Before pushing I confirmed the change really is test-only and that it passes, rather than taking the branch's word for it:
|
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.
Summary
PR #482 (closing #477) made IP fragment reassembly record overlap conflicts instead of
silently losing data, adding
_detect_conflictsand aconflictfield onBufferandDatagraminpcapkit/foundation/reassembly/ip.py. Its reviewer approved the change butnamed two genuine coverage gaps in the new logic -- neither pointing at a suspected bug,
both untested branches of otherwise-verified behaviour. This PR closes both, test-only.
Gap 1 -- no test had two BUFIDs genuinely in flight at once.
conflictis meant to bestrictly per-
Buffer; the reviewer judged the risk low becauseBuffer.conflictiscreated fresh per BUFID, but "created fresh" is an argument, not a test. This class of gap
already burned this programme once: reassembly: resolve conflicting TCP overlaps first-write-wins, per RFC 9293 #478's original six boundary cases all used a single
ACK bucket, which is exactly why they could not catch a cross-bucket data-loss regression
that was genuinely present in the code.
test_conflict_stays_scoped_to_its_own_bufidinterleaves two datagrams' fragments -- neither completes before the other's next fragment
arrives -- conflicts one, and asserts the other's
conflictstays empty.Gap 2 -- no test exercised a conflict sandwiched between two matching regions inside one
write. The inner loop of
_detect_conflictsbreaks a conflict run on a match and resumesdetection afterwards, but nothing drove a single fragment's comparison pass through two
separate, non-adjacent conflict runs.
test_single_write_produces_two_conflict_runs_separated_by_a_matchconstructs one fragmentwrite that disagrees with the buffer at octets 8-15 and 24-31 but agrees at 16-23, and
asserts both runs appear with the correct absolute inclusive bounds.
Both tests go through the public reassembly entry point and
Datagram, not the private_detect_conflictsroutine directly.No defect was found in
pcapkit/while writing these -- both gaps were genuinely untestedbranches of correct logic, not bugs. No source under
pcapkit/is touched by this PR.Regression proof
Each test was verified by temporarily breaking the exact behaviour it targets, confirming
the test failed, then restoring the source (
git checkout -- pcapkit/foundation/reassembly/ip.py):Bufferthe same sharedconflictlist instead of a fresh one perBUFID -> the clean datagram's
conflictpicked up the conflicting datagram's(0, 7)entry, failing the assertion that it stays
()._detect_conflicts' outer loop after the first conflict run insteadof resuming -> only
(8, 15)was returned, missing(24, 31), failing the two-runassertion.
Test plan
pytest tests/foundation/reassembly/test_ip.py-- 16 passedpytest tests/foundation/reassembly/-- 65 passedpytest tests/foundation/-- 202 passed, 11 skippedpass again against the restored source
Refs: #477, #482