Skip to content

fix(corekit): StringField is not lossless: guessed charset, U+FFFD substitution, and a character-count length - #1336

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1293-stringfield-lossless
Oct 7, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1293-stringfield-lossless

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests

  • Followed the coding style — pylint, mypy and isort run on both files: pylint reports the same six findings as main's copy of strings.py and no new one, mypy none, isort clean

  • make test passes, and a test case covers the change — legs below

  • Added a changelog entry — N/A, added centrally after the wave

  • fix — corrects a defect


StringField now packs back exactly the octets it unpacked, while the decoded text is left exactly as it was. Where re-encoding the text would not reproduce its octets — a detected charset other than UTF-8, an octet the charset cannot decode, a lossy errors scheme, a URL quoting that differs — post_process returns a DecodedString, a str subclass carrying those octets, and pre_process packs them verbatim. length=-1 counts encoded octets. I kept the octets rather than recording the detected charset because a field is copied per packet, so a charset stored on unpack never reaches pack.

Decode gained no new way to fail. The fallback stays main's 'replace', so an undecodable octet still reads as U+FFFD, and the docstring says that instead of claiming 'strict' raises — naming idna, measured, as the codec that raises anyway because it supports no error handler. The 'surrogateescape' fallback of my first revision was wrong twice: lone surrogates in the text raise UnicodeEncodeError inside dictdumper for plist and tree, and it is unsupported for utf-16{,-le,-be}/utf-32. Both are now pinned by tests, and plist/tree/json dumps of a capture whose comments are ff fe and caf\xe9 are byte-identical to main's.

DecodedString compares and hashes as its text, deliberately: mixing .raw into __eq__ would make info.comment == 'café' false against a plain literal, which is the comparison callers write. It is exported from pcapkit.corekit.fields.strings; adding it to pcapkit.corekit.fields, pcapkit.corekit and pcapkit.all would touch files this PR does not own, so I am reporting it rather than doing it.

Repro, e974e9 undetected / caf\xe9 as UTF-8 / -1 with 'héllo': before c3a974 / 636166efbfbd / 68c3a96c6c, after e974e9 / 636166e9 / 68c3a96c6c6f. A 24,000-case fuzz over 8 field configurations gives 0 mismatches, 0 exceptions and 0 surrogate-bearing values. Per-frame info over all 23 fixture captures is identical before and after. Tests: 11 of 13 fail on main and 5 fail on my first revision — including the dumper test — and all 13 pass now. Legs: corekit 606 tests / 5 failures, all five pre-existing (test_sentinel_exports_unit, identical on an unpatched tree); protocols/internet 503/0; protocols (rest) 850/0; dumpkit 209/0; tests/project 389 passed, 1 skipped.

Callers (encoding): pcapng CommentOption, IF_Name, IF_Description, IF_OS, IF_Hardware (utf-8); IPv4Record/IPv6Record.resol, NS_DNSNameOption.name (none, so chardet); TLSKeyLog, WireGuardKeyLog (ascii); MH mn_id_selector NAI and ServiceSelectionOption.identifier (none). The five without an encoding should declare encoding='utf-8' — caller work, in files this PR does not own.

Refs #1277. Its repro is repaired by that encoding='utf-8' alone: with it, the NRB record parses to ('a.example', 'b.example') and rebuilds byte-exactly — on main's strings.py too, so this PR is not what fixes it. A name that is not valid UTF-8 stays lossy either way, because IPv4Record.post_process splits resol with str.split, which returns plain str and drops the octets.
Refs #1276. The makers pass length=len(str) explicitly, so this PR does not fix it.
#1241 is unaffected: it goes through Protocol.decode, not StringField.

Closes #1293

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 7, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on e28a6397f: NEEDS CHANGES (Sonnet reviewed; the author ran on Opus). The round trip itself works; two regressions sit beside it.

1. surrogateescape crashes the plist and tree dumpers — blocking. An undecodable octet in a UTF-8 field becomes a lone surrogate, and because surrogateescape re-encodes back to the same bytes, the re-encode check passes and the value stays a plain str rather than a DecodedString. So surrogates reach consumers through the ordinary path. A pcapng whose SHB and IDB comments are ff fe makes extract(format='plist') raise UnicodeEncodeError: surrogates not allowed; main succeeded, writing U+FFFD. tree does the same. A realistic input fails too: the comment caf\xe9 — Latin-1 bytes in a UTF-8 field. json survives, escaping to \udcff. An independent fuzz put a surrogate-bearing value in 11,033 of 60,000 cases, so this is not a corner. Neither the new tests nor any fixture touch it.

2. The field can now raise on decode. With an explicit utf-16, utf-16-le, utf-16-be, utf-32 or utf-7, bad bytes raise UnicodeDecodeError, because the surrogateescape fallback is unsupported for those codecs; idna gives UnicodeError('Unsupported error handling: surrogateescape'). main's replace fallback did not raise, and the docstring and PR body still claim the field never raises on decode. No in-repo field uses these encodings, so this is a library-user regression — fix it or document it, but the prose cannot stay as it is.

Verified as described: all three #1293 repros fail on main and give e974e9, e974e9, 68c3a96c6c6f here. copy, deepcopy and pickle (default and protocol 5) preserve the type and .raw, including inside a dict. No type(x) is str check exists in pcapkit/. 60,000 fuzz inputs over 84 configurations gave 0 round-trip mismatches. All 23 captures / 1,605 frames produce identical json output to main, and no DecodedString appears — so the fixtures do not exercise the new path at all. The five encoding=-less fields are exactly where the PR says (schema/misc/pcapng.py:1464, :1495, :1533; schema/internet/mh.py:353, :1060), and adding 'utf-8' changes no fixture's parse. #1276 and #1241 are as described; for #1277 the encoding fix alone already repairs the parse (('a.example','b.example')), so the claim that the maker also needs fixing is narrower than stated and still unverified for byte-exact rebuild.

Nits: DecodedString is in strings.__all__ but is not re-exported from pcapkit.corekit.fields, pcapkit.corekit or pcapkit.all, which is odd for something the PR calls a public contract change; and its equality and hashing ignore .raw, so two values with identical text and different bytes compare equal — which matters as a dict key and in Info equality.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 7, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 7, 2026
…bstitution, and a character-count length

- post_process returns a DecodedString -- a str subclass carrying the octets
  it decoded -- whenever re-encoding the text would not reproduce them: a
  detected charset other than UTF-8, an octet the charset cannot decode, a
  lossy errors scheme, or a URL quoting that differs from the original.
  pre_process packs those octets verbatim, so the field round-trips.
- The decoded text is left exactly as it was, U+FFFD and all, which is what
  keeps the dumpers working: a lone surrogate there raises UnicodeEncodeError
  inside dictdumper for plist and tree. The decode fallback therefore stays
  'replace', and the docstring describes that rather than claiming 'strict'
  raises.
- length=-1 resolves to the encoded octet count, not the character count.

Closes #1293
@JarryShaw
JarryShaw force-pushed the fix/1293-stringfield-lossless branch from e28a639 to b1f9072 Compare October 7, 2026 19:30
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 7, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Round-2 verdict on b1f90725a: GOOD TO GO (Sonnet reviewed; author Opus). Neither round-1 failure reproduces.

  • No more surrogates. Going back to main's 'replace' fallback leaves the decoded text identical to main. I fuzzed 28,000 cases with an independent generator over 14 configurations, covering overlong UTF-8, ed a0 80, lone continuation bytes, BOMs, UTF-16/32/UTF-7 fragments and % escapes. That produced 0 surrogate-bearing values, and every case packed back to its original octets, including the 13,722 whose text held U+FFFD.
  • The dumpers match main byte for byte. Four new pcapngs (including ff fe, caf\xe9, ed a0 80 and c0 af) give plist, tree and json output identical to main under diff -r. All 22 captures give identical per-file info hashes.
  • No new way to raise. Across 99+ codecs there are 0 differences from main; idna and undefined raise on both sides.
  • Textual equality is safe in-tree. No pcapkit/ code dict-keys, sets or deduplicates StringField values, and the only cache on this path (utilities/chardet.py) is keyed on input bytes.
  • Test legs: dumpkit 209/0. The 5 corekit failures are test(corekit): test_sentinel_exports_unit fails 5 tests in the corekit unittest leg #1332's.

Two PR-body figures to correct, neither blocking:

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 7, 2026
@JarryShaw
JarryShaw merged commit 0769751 into main Oct 7, 2026
33 of 36 checks passed
@JarryShaw
JarryShaw deleted the fix/1293-stringfield-lossless branch October 7, 2026 20:42
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 7, 2026
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(corekit): StringField is not lossless: guessed charset, U+FFFD substitution, and a character-count length

1 participant