QuakeWang commented on code in PR #364:
URL: https://github.com/apache/paimon-rust/pull/364#discussion_r3396601521
##########
crates/paimon/src/spec/binary_row.rs:
##########
@@ -92,7 +92,29 @@ impl BinaryRow {
});
}
let arity = i32::from_be_bytes([data[0], data[1], data[2], data[3]]);
- Ok(Self::from_bytes(arity, data[4..].to_vec()))
+ if arity < 0 {
+ return Err(crate::Error::UnexpectedError {
+ message: format!("BinaryRow: serialized data has negative
arity: {arity}"),
+ source: None,
+ });
+ }
+ let body = &data[4..];
+ // The body must hold at least the null bitmap and the fixed part
+ // (8 bytes per field); reject truncated input rather than panicking
+ // later when reading the null bitmap or a field. The size is computed
+ // in i64 so an absurd arity in malformed input cannot overflow.
+ let bit_set_width = ((arity as i64 + 63 + Self::HEADER_SIZE_IN_BYTES
as i64) / 64) * 8;
+ let fix_part_size = bit_set_width + 8 * arity as i64;
+ if (body.len() as i64) < fix_part_size {
Review Comment:
Separate from the large-arity overflow case above, this also rejects a
currently valid zero-arity roundtrip. `BinaryRow::new(0).to_serialized_bytes()`
emits only the 4-byte arity prefix because `data` is empty, but this check
requires an 8-byte body for `arity == 0`, so `from_serialized_bytes` no longer
accepts the bytes produced by `to_serialized_bytes`.
Could we special-case `arity == 0` to allow an empty body, or canonicalize
zero-arity rows to serialize with the expected body size? Please also add a
roundtrip test for `BinaryRow::new(0).to_serialized_bytes()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
