Repository navigation
Recover undercounted discovery_message signals - #25
Conversation
A Minecraft client sending a non-trickle answer with all ICE candidates inline undercounts the length prefixes it writes, so protodef truncates the parsed signal and setRemoteDescription fails. The full payload is present in the checksum-validated datagram, so read the discovery_message data field to the end of the buffer. Scoped to discovery_message and assumes one encapsulated message per datagram; adds synthetic regression fixtures for outer-only, inner-only, valid, and bad-checksum cases.
extremeheat
left a comment
extremeheat
left a comment
There was a problem hiding this comment.
Using this PR as the merge base instead of #24: your live validation and clarification of the two captures resolve the main uncertainty from my earlier review.
Pushed f02af42 via maintainer edits with the useful parts of #24:
- Limit recovery to CONNECTRESPONSE rather than every discovery signal.
- Detect trailing bytes using parser metadata, avoiding decoded UTF-8 byte-count inflation when a prefix cuts through a character.
- Keep appended supported binary discovery frames out of the recovered SDP with the NUL guard; this preserves existing first-packet behavior, not batch support.
- Shorten the implementation comments and move the offset constant to file scope.
- Extend your existing test file rather than add a duplicate suite.
All 38 tests, lint and types pass locally. Four added cases fail on the original patch and pass on this revision. Your captured ASCII answer remains on the recovery path. I have not independently repeated the live-host test of the exact combined revision.
Approved; will merge this revision when its three-platform CI passes, then close #24 as superseded. Thanks for tracking this down.
— OpenAI Astra
|
@Pix3lPirat3 we opened #24 at almost the same time, so we're consolidating on your #25 and keeping your original commit and test fixture. I pushed f02af42 to your branch using maintainer edits: narrower CONNECTRESPONSE-only recovery, byte-based detection via parser metadata, a guard against absorbing appended binary discovery frames, and six additional cases in your existing test file. All 38 tests, lint and types pass locally. Your explanation of the two separate captures resolves the offset discrepancy—thanks. The plan is to merge #25 once CI passes on that revision and close our duplicate #24. No npm release is being made as part of this step. If convenient, please retry the combined revision on your live LAN world; the recovery still covers your reported ASCII answer, but we haven't independently exercised that exact build against a retail host. — OpenAI Astra |
|
/makerelease patch |
|
Confirmed: retested the merged revision (6b03dd3 + f02af42) against my live 1.26.51 client-hosted LAN world. The full 1383-byte CONNECTRESPONSE comes through and the client reaches |
2 participants
Fixes #23. A retail Minecraft client-hosted LAN world can send a complete inline-candidate answer in a secure datagram while undercounting its string length. ProtoDef truncates the SDP trailer and WebRTC rejects the answer.
After checksum validation, recover trailing plaintext only for
discovery_messagesignals beginning withCONNECTRESPONSE. Use the parser's consumed byte count to detect a short payload and read from the schema's data offset (32). Leave other signals and discovery advertisements unchanged. Do not recover data containing NUL bytes: SDP text cannot contain them, while appended supported binary discovery headers do. This preserves the existing first-packet behavior rather than introducing batch parsing; WebRTC still validates the SDP itself.The outer prefix is also short in the capture, but
Read.encapsulatedcurrently does not use its value to bound parsing. The inner pstring is the immediate truncation point.Byte accounting clarified by Pixel: the 1,418/1,384 measurements in #23 came from two different captures (1,418 vs 1,416 plaintext bytes). Data starts at offset 32. The retail client's outer length uses data length +32, whereas our serializer uses +30; recovery depends on neither convention.
Validation:
npm test: lint, types, and all 38 tests pass locally after refinement.Consolidates #24 into this PR, preserving Pixel's original fix, regression fixture and live investigation. Internet-reachable friend-world testing remains separate from this LAN decoder fix.