Skip to content

test(tcp): expect the short-read padding on the tail, repairing main (#604) (#621) - #627

Merged
JarryShaw merged 2 commits into
mainfrom
fix/tcp-truncated-option-tail-padding
Sep 22, 2026
Merged

JarryShaw merged 2 commits into
mainfrom
fix/tcp-truncated-option-tail-padding

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

What this is

main is currently red, and this repairs it. Test-only — pcapkit/** is untouched.

#621 changed pcapkit/corekit/fields/field.py:527 from rjust to ljust, so a short field
read is padded on the tail rather than the head. That fix is correct and is not touched
here. But tests/protocols/transport/test_tcp_udp_unit.py::TCPUDPUnitTests::test_a_truncated_option_still_parses_its_declared_length
still pinned the old head-padded behaviour, so both of its subtests now fail on main.

#621 deliberately left that file alone because #612 owned it at the time. #612 has since
merged, the file became unowned, and #621 merged at 03:28:24Z — two minutes before its
cross-review's NEEDS CHANGES verdict landed at 03:30:20Z, which is what named this file. So
this repairs main after #621 merged ahead of its own cross-review verdict. Context: #604
(the defect #621 fixed) and #621 (the PR whose merge exposed this). Not Fixes #604#604
is already closed by #621.

The failure, measured on unmodified main

$ pytest -q tests/protocols/transport/test_tcp_udp_unit.py
SUBFAILED(declared_length=12) tests/protocols/transport/test_tcp_udp_unit.py::TCPUDPUnitTests::test_a_truncated_option_still_parses_its_declared_length
SUBFAILED(declared_length=32) tests/protocols/transport/test_tcp_udp_unit.py::TCPUDPUnitTests::test_a_truncated_option_still_parses_its_declared_length
2 failed, 17 passed, 2 warnings in 16.80s
exit 1

with the assertion detail confirming the cause is purely the padding side and not a semantic
regression — the option's declared length, the parsed options list and the
bytes(proto.__header__) round-trip all still hold:

AssertionError: b'\xaa\xbb\xcc\xdd\xee\xff\x00\x00\x00\x00'   (actual, tail-padded — correct now)
             != b'\x00\x00\x00\x00\xaa\xbb\xcc\xdd\xee\xff'   (expected, pins the old rjust)

What changed

Three sites, all inside that one test:

  • :1415 (the assertion) — b'\x00' * zeroes + trailing becomes
    trailing + b'\x00' * zeroes, with a comment saying why the real octets come first and
    noting that the inputs distinguish the two orders.
  • :1374 (prose) — FieldBase.unpack "left-pads the short read … a data value of
    four zero octets followed by the six real ones" becomes "pads the short read … the six real
    octets followed by four zero ones", plus a sentence saying the zeros go on the tail, where
    the octets never read would have been, and that before FieldBase.unpack pads a short read with rjust regardless of byte order, silently corrupting little-endian values #604 they were placed at the front.
  • :1386 (prose) — "32 yields 24 zero octets where 12 yields 4" becomes "24 trailing
    zero octets".

Inverting the prose as well as the assertion is the point rather than tidiness: a docstring
describing head-padding above an assertion checking tail-padding is exactly how this test came
to pin the wrong behaviour, and how the stale expectation survived #604's review. The wording
now matches the sibling IPv4UnitTests.test_a_truncated_option_still_parses_its_declared_length,
which #621 already corrected.

Nothing else in the 1511-line file describes or depends on the padding side — checked by
grepping for rjust/ljust/pad/left/front/leading/zero octets/short read and
reading every hit. The only other near-miss, the # the truncated HMAC (8) + the nonce (4)
comment at :1021, is RFC 8684 MP_JOIN field arithmetic, unrelated. Consistent with that, the
other 17 tests in the file passed on unmodified main.

Do (12, 4) and (32, 24) actually discriminate tail- from head-padding?

Yes, and it is measured rather than argued. trailing = aabbccddeeff is non-zero and
zeroes is non-zero in both pairs, and b'\x00' * n + t == t + b'\x00' * n only when t is
all zeros or n == 0 — neither holds here. The proof is the two runs in this PR: the
head-padded expectation fails both subtests on the current library, and the tail-padded one
passes both. A test that held under either order would have stayed green across #621.

The pair also still pins what it was added to pin: 4 vs 24 shows the pad width tracking
length - 2 and scaling with the declared length rather than being a fixed 4. Worth stating
explicitly given this file's history — a sibling test was once written with inputs that were
all multiples of 8 and so could not tell a floor from a ceiling from a quarter, a pattern that
has now bitten this codebase four times.

Verification

After, on this branch:

$ pytest -q tests/protocols/transport/test_tcp_udp_unit.py
17 passed, 2 warnings, 2 subtests passed in 16.29s
exit 0

The two files #621 itself updated are undisturbed, matching the figure #621's reviewer
measured:

$ pytest -q tests/corekit/test_fields_field.py tests/protocols/internet/test_ipv4_unit.py
61 passed, 20 warnings, 164 subtests passed in 62.03s
exit 0

and all three together on the exact commit being proposed:

78 passed, 22 warnings, 166 subtests passed in 79.01s
exit 0

Every exit code above was read from a file written by pytest.main()'s return value, not from
a pipeline's $?. Measured against
/local/home/jarryx/GitHub/PyPCAPKit/.claude/worktrees/agent-a50e24f7802968df3/pcapkit/__init__.py
(version 1.5.0b4) — the venv's __editable__.pypcapkit-1.4.1.post2.pth installs a
MetaPathFinder whose MAPPING hard-codes the main checkout, and a meta-path finder beats
PYTHONPATH, so the runner drops that finder and asserts pcapkit.__file__ resolves inside
this worktree before collecting. git status was clean for every run quoted.

The full suite was deliberately not run: coverage run -m pytest tests/ reached 41.4 GB
RSS on this host today and was killed. Every run above is scoped to the named files.

Changelog

Bullet added to docs/source/changelog/1.5.0.rst; CHANGELOG.md regenerated with
python util/changelog_md.py and --check exits 0. Rebased onto origin/main after #619
landed its own changelog bullet — both bullets are kept, mine last, and the regeneration after
the rebase was a no-op, so the .md was never hand-edited.

* `TCPUDPUnitTests.test_a_truncated_option_still_parses_its_declared_length`
  still expected `b'\x00' * zeroes + trailing`, the head-padded short read
  that #604 removed. Both subtests failed on that assertion once #621 landed,
  so `main` was red. The expectation is now `trailing + b'\x00' * zeroes`.
* The docstring above it said `FieldBase.unpack` "left-pads" the short read
  and described the value as four zero octets followed by the six real ones.
  Both are inverted, so the prose no longer contradicts its own assertion.

Test-only; `pcapkit/` is untouched. The file goes 2 failed / 17 passed / exit 1
to 17 passed / 2 subtests passed / exit 0, and the two files #621 updated stay
at 61 passed / 164 subtests / exit 0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test Pull requests that add or correct tests (test: subject prefix)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant