fix: SkipWhitespace must not report EOF on an all-whitespace read - #570
fix: SkipWhitespace must not report EOF on an all-whitespace read#570Galmanus wants to merge 2 commits into
Conversation
SkipWhitespace::read returned Ok(0) whenever a single delegate read yielded a chunk that was entirely ASCII whitespace (n > 0 but 0 bytes written). Per the Read contract, Ok(0) on a non-empty buffer signals EOF, so the base64 DecoderReader (1024-byte internal buffer) treats a whitespace run of >= 1024 bytes as end-of-input and silently truncates the decoded XDR stream. Loop until at least one non-whitespace byte is produced, or the inner reader genuinely reaches EOF (returns 0). The impl lives in the generator template (header.rs) and is copied verbatim into generated.rs; both are updated identically.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes SkipWhitespace’s Read implementation so runs of whitespace are not misreported as EOF (and therefore don’t silently truncate base64-decoded XDR), and adds a regression test for long whitespace runs.
Changes:
- Update
SkipWhitespace::readto keep reading when a full delegate read contains only whitespace, only returningOk(0)on true inner EOF. - Add a regression test covering long leading and interior whitespace runs around base64.
- Apply the same fix to the checked-in generated output.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| xdr-generator-rust/generator/header.rs | Fix SkipWhitespace::read to avoid returning Ok(0) after filtering whitespace-only reads. |
| tests/tx_base64_skip_whitespace.rs | Add regression test for long whitespace runs that previously caused truncation. |
| src/generated.rs | Mirror the SkipWhitespace::read fix in generated code. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // A whitespace run at least as long as the base64 decoder's internal read | ||
| // buffer (1024 bytes) must not be treated as end-of-input. Before the fix, | ||
| // SkipWhitespace::read returned Ok(0) when a whole delegate read was |
There was a problem hiding this comment.
Good point — reworded in 492d03f so the comment no longer pins the base64 crate's private 1024-byte constant; it now just says the run (2048 bytes) is large enough to fill at least one internal read buffer.
| // SkipWhitespace::read returned Ok(0) when a whole delegate read was | ||
| // whitespace, which the decoder interprets as EOF -> silent truncation. | ||
| let v_bytes = [1u32.to_xdr(Limits::none())?, 2u32.to_xdr(Limits::none())?].concat(); | ||
| let core = base64::engine::general_purpose::STANDARD.encode(&v_bytes); |
There was a problem hiding this comment.
The base64::Engine trait is already explicitly imported at the top of this file (use base64::Engine;, line 3) — that's what resolves the .encode(...) call here, and the pre-existing test_skip_whitespace relies on the same import for its own encode. So method resolution isn't relying on anything non-obvious. Adding use base64::Engine as _; would just duplicate an import that's already present, so leaving as-is.
…comment Copilot review: 1024 is an implementation detail of the base64 crate. Reword to 'a long contiguous whitespace run (2048 bytes here, large enough to fill at least one of the decoder's internal read buffers)' so the test comment doesn't assert a specific private constant.
What
SkipWhitespace::readreturnedOk(0)whenever a single delegate read yielded a chunk that was entirely ASCII whitespace (n > 0but0bytes written after filtering). Per theReadcontract,Ok(0)on a non-empty buffer signals EOF, sobase64::read::DecoderReader(1024-byte internal buffer) treats a whitespace run of ≥ 1024 bytes as end-of-input and silently truncates the decoded XDR stream.The fix loops until at least one non-whitespace byte is produced, or the inner reader genuinely reaches EOF (returns
0).Background
SkipWhitespacewas added to make the base64 read paths tolerate whitespace (the fix for #505). It handles the common case (whitespace every ~64 chars, PEM-style) correctly. This is a follow-up hardening for the edge case of a long contiguous whitespace run.Reproduction (fails before, passes after)
Before this change the assertion fails with
Err(Io(UnexpectedEof, "failed to fill whole buffer"))— the 2048-space run is read as one all-whitespace chunk,SkipWhitespacereturnsOk(0), and the decoder stops before the real data. Interior whitespace runs of the same size truncate mid-stream.Impact
Correctness bug, low real-world frequency: normal PEM-wrapped or hand-pasted base64 breaks lines every ~64 chars, so ≥1024-byte contiguous whitespace runs are uncommon — but when they occur the failure is a silent/short decode, which is the dangerous kind.
Scope
xdr-generator-rust/generator/header.rsand is copied verbatim (viainclude_str!) intosrc/generated.rs; both are updated identically, so the generated output stays in sync with the template.test_skip_whitespace) still passes.Tests
Added
test_skip_whitespace_long_run(leading and interior ≥1024-byte whitespace runs) totests/tx_base64_skip_whitespace.rs.cargo test --features base64 --test tx_base64_skip_whitespaceandcargo clippyare green.