fix(node): bound the emitted Arrow IPC stream length at the producer - #2787
fix(node): bound the emitted Arrow IPC stream length at the producer#2787phil-opp wants to merge 1 commit into
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
🤖 Automated review by Claude — this is a fully automated review with no human in the loop; treat findings as suggestions to verify, not as authority. No issues found. The fix correctly guards the emitted stream length rather than the input Generated by Claude Code |
|
🤖 Automated review by Claude — fully automated, no human in the loop; it may contain mistakes. The bound logic itself looks correct — it bounds the actual emitted Relative to current
The PR also currently shows as conflicting with Generated by Claude Code |
874cc7e to
cca4a51
Compare
|
🤖 Automated review by Claude — this is a fully automated review, with no human in the loop verifying the findings below. Re-reviewed after the rebase onto
The bound arithmetic itself is correct, but the producer-side guard #2586 asked for is already provided on the live send path by #2588. Unless there's an intended case #2588 doesn't cover, this looks safe to close. Generated by Claude Code |
The Arrow IPC encode guards bounded the *input* length (`data_len` in `uint8_layout`, the body offset in the fast-path builder) against `MAX_IPC_BYTES`, but the emitted stream is ~1.125x larger (validity bitmap, 64-byte alignment padding, schema + record-batch message blocks). Every receiver bounds the *whole stream* against the same `MAX_IPC_BYTES`, so a payload whose `data_len` sits just under the cap could encode and be sent, then be rejected by all receivers — a silent drop on the zenoh path. Guard the resulting stream length instead, failing loudly at the producer: - `uint8_layout` now checks the computed `total` (covers the `send_output_raw` UInt8 construct-in-place path, incl. the Python bindings). - `send_output_array` runs `check_ipc_size` on the fast-path length and on the fallback stream before allocating, so anything the producer emits is decodable by every receiver. `check_ipc_size` is now shared (producer + decode guard); its doc notes the dual role. Fixes #2586 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C79MriEbXiurvZQQ3QoCkU
cca4a51 to
9c7e141
Compare
Re-checked after the rebase that moved the two
The Generated by Claude Code |
|
Closing as already fixed. The producer-side bound landed via #2588: The branch is also no longer rebaseable as-is: |
Summary
Fixes #2586.
The Arrow IPC data plane (#2366) guarded the input length against
MAX_IPC_BYTESat encode time, but the emitted IPC stream is ~1.125× larger (validity bitmapdata_len/8, 64-byte alignment padding, and the schema + record-batch message blocks). Every receiver bounds the whole stream against the sameMAX_IPC_BYTES. So a payload whose input length sits just under the cap (roughly(227.5 MiB, 256 MiB]forUInt8) could pass the encode guard and be sent, then be rejected by all receivers — a silent drop on the zenoh receive path.Changes
Guard the resulting stream length instead, so anything the producer accepts is decodable by every receiver (or it fails loudly at the producer):
uint8_layout(ipc_encode.rs): now bounds the computedtotalstream length, not justdata_len. Covers thesend_output_rawUInt8 construct-in-place path — including the Python bindings (apis/python/node/.../sample_handler.rs) andsend_output_bytes(mod.rs), both of which go throughuint8_ipc_len/encode_uint8_ipc_header. The pre-existingdata_len > MAX_IPC_BYTEScheck is kept as an overflow-safety bound beforetotalis computed.send_output_array(mod.rs): runscheck_ipc_sizeon the fast-path length and on the fallback stream length before allocating the sample, so the general array path (both the zero-copy fast path and the official-writer fallback) also fails loudly rather than emitting an undecodable stream.check_ipc_sizeis promoted topub(crate)and its doc updated to note the dual producer/decode role.Tests
Added
uint8_ipc_len_rejects_oversized_stream:data_len == MAX_IPC_BYTESclears a naivedata_lencheck but the stream overhead pushes the emitted stream over the cap, so it must be rejected; a small payload still encodes to an in-bounds stream. Fullipc_encodesuite (33 tests) passes;cargo fmt --checkandcargo clippy -p dora-node-api -- -D warningsare clean.Notes
The receiver-side guards are unchanged — this only tightens the producer side so the encode and decode limits agree.
🤖 Generated with Claude Code
Generated by Claude Code