You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "KHARSHAVARDHAN-eng (via GitHub)" <gi...@apache.org> on 2026/09/07 04:10:30 UTC
[PR] fix(ipc): validate block lengths before allocation [arrow-rs]
KHARSHAVARDHAN-eng opened a new pull request, #11012:
URL: https://github.com/apache/arrow-rs/pull/11012
Closes #10921
### Summary
- Validate IPC footer block offsets and lengths against the file size before allocating the block buffer.
- Use checked integer conversions and arithmetic for block offsets and lengths.
- Add a regression test covering oversized, negative, out-of-bounds, and overflowing block metadata.
### Validation
- `cargo fmt --check`
- `cargo clippy -p arrow-ipc --all-targets -- -D warnings`
- `cargo test -p arrow-ipc`
- `git diff --check`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(ipc): validate block lengths before allocation [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #11012:
URL: https://github.com/apache/arrow-rs/pull/11012#issuecomment-5707577512
run benchmark ipc_reader
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(ipc): validate block lengths before allocation [arrow-rs]
Posted by "adriangbot (via GitHub)" <gi...@apache.org>.
adriangbot commented on PR #11012:
URL: https://github.com/apache/arrow-rs/pull/11012#issuecomment-5707635687
🤖 Arrow criterion benchmark completed (GKE) | [trigger](https://github.com/apache/arrow-rs/pull/11012#issuecomment-5707577512)
**Instance:** `c4a-highmem-16` (12 vCPU / 65 GiB)
Comparing fix-ipc-block-length-allocation (a4921f116f71f5e22461d73030654214b9f662b3) to b06b9c1 (merge-base) [diff](https://github.com/apache/arrow-rs/compare/b06b9c12c17dca4f575c9c3e5a091fdb9412bab7..a4921f116f71f5e22461d73030654214b9f662b3)
<details><summary>Run configuration</summary>
```yaml
run benchmark ipc_reader
```
</details>
<details><summary>CPU Details (lscpu)</summary>
```
Architecture: aarch64
CPU op-mode(s): 64-bit
Byte Order: Little Endian
CPU(s): 16
On-line CPU(s) list: 0-15
Vendor ID: ARM
Model name: Neoverse-V2
Model: 1
Thread(s) per core: 1
Core(s) per cluster: 16
Socket(s): -
Cluster(s): 1
Stepping: r0p1
BogoMIPS: 2000.00
Flags: fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 sm3 sm4 asimddp sha512 sve asimdfhm dit uscat ilrcpc flagm sb paca pacg dcpodp sve2 sveaes svepmull svebitperm svesha3 svesm4 flagm2 frint svei8mm svebf16 i8mm bf16 dgh rng bti
L1d cache: 1 MiB (16 instances)
L1i cache: 1 MiB (16 instances)
L2 cache: 32 MiB (16 instances)
L3 cache: 80 MiB (1 instance)
NUMA node(s): 1
NUMA node0 CPU(s): 0-15
Vulnerability Gather data sampling: Not affected
Vulnerability Indirect target selection: Not affected
Vulnerability Itlb multihit: Not affected
Vulnerability L1tf: Not affected
Vulnerability Mds: Not affected
Vulnerability Meltdown: Not affected
Vulnerability Mmio stale data: Not affected
Vulnerability Reg file data sampling: Not affected
Vulnerability Retbleed: Not affected
Vulnerability Spec rstack overflow: Not affected
Vulnerability Spec store bypass: Mitigation; Speculative Store Bypass disabled via prctl
Vulnerability Spectre v1: Mitigation; __user pointer sanitization
Vulnerability Spectre v2: Mitigation; CSV2, BHB
Vulnerability Srbds: Not affected
Vulnerability Tsa: Not affected
Vulnerability Tsx async abort: Not affected
Vulnerability Vmscape: Not affected
```
</details>
<details><summary>Details</summary>
<p>
```
group fix-ipc-block-length-allocation main
----- ------------------------------- ----
arrow_ipc_reader/FileReader/no_validation/read_10 1.00 122.0±12.52µs ? ?/sec 1.07 130.7±16.88µs ? ?/sec
arrow_ipc_reader/FileReader/no_validation/read_10/mmap 1.00 58.2±0.92µs ? ?/sec 1.01 58.6±0.89µs ? ?/sec
arrow_ipc_reader/FileReader/read_10 1.00 414.0±45.48µs ? ?/sec 1.07 442.6±60.75µs ? ?/sec
arrow_ipc_reader/FileReader/read_10/mmap 1.01 491.9±35.86µs ? ?/sec 1.00 486.2±38.95µs ? ?/sec
arrow_ipc_reader/StreamReader/no_validation/read_10 1.00 115.7±7.91µs ? ?/sec 1.03 119.5±10.42µs ? ?/sec
arrow_ipc_reader/StreamReader/no_validation/read_10/zstd 1.00 2.5±0.02ms ? ?/sec 1.00 2.4±0.02ms ? ?/sec
arrow_ipc_reader/StreamReader/read_10 1.02 419.0±29.99µs ? ?/sec 1.00 409.7±41.04µs ? ?/sec
arrow_ipc_reader/StreamReader/read_10/zstd 1.01 2.8±0.02ms ? ?/sec 1.00 2.7±0.01ms ? ?/sec
```
</p>
</details>
<details><summary>Resource Usage</summary>
**base (merge-base)**
| Metric | Value |
|--------|-------|
| Wall time | 85.0s |
| Peak memory | 15.9 MiB |
| Avg memory | 11.7 MiB |
| CPU user | 71.4s |
| CPU sys | 9.4s |
| Peak spill | 0 B |
**branch**
| Metric | Value |
|--------|-------|
| Wall time | 85.0s |
| Peak memory | 15.1 MiB |
| Avg memory | 11.0 MiB |
| CPU user | 69.3s |
| CPU sys | 9.7s |
| Peak spill | 0 B |
</details>
---
[File an issue](https://github.com/adriangb/datafusion-benchmarking/issues) against this benchmark runner
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(ipc): validate block lengths before allocation [arrow-rs]
Posted by "adriangbot (via GitHub)" <gi...@apache.org>.
adriangbot commented on PR #11012:
URL: https://github.com/apache/arrow-rs/pull/11012#issuecomment-5707607052
🤖 Arrow criterion benchmark running (GKE) | [trigger](https://github.com/apache/arrow-rs/pull/11012#issuecomment-5707577512)
**Instance:** `c4a-highmem-16` (12 vCPU / 65 GiB) | `Linux bench-c5707577512-2399-42cr9 6.12.94+ #1 SMP Tue Aug 4 08:44:15 UTC 2026 aarch64 GNU/Linux`
<details><summary>CPU Details (lscpu)</summary>
```
Architecture: aarch64
CPU op-mode(s): 64-bit
Byte Order: Little Endian
CPU(s): 16
On-line CPU(s) list: 0-15
Vendor ID: ARM
Model name: Neoverse-V2
Model: 1
Thread(s) per core: 1
Core(s) per cluster: 16
Socket(s): -
Cluster(s): 1
Stepping: r0p1
BogoMIPS: 2000.00
Flags: fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 sm3 sm4 asimddp sha512 sve asimdfhm dit uscat ilrcpc flagm sb paca pacg dcpodp sve2 sveaes svepmull svebitperm svesha3 svesm4 flagm2 frint svei8mm svebf16 i8mm bf16 dgh rng bti
L1d cache: 1 MiB (16 instances)
L1i cache: 1 MiB (16 instances)
L2 cache: 32 MiB (16 instances)
L3 cache: 80 MiB (1 instance)
NUMA node(s): 1
NUMA node0 CPU(s): 0-15
Vulnerability Gather data sampling: Not affected
Vulnerability Indirect target selection: Not affected
Vulnerability Itlb multihit: Not affected
Vulnerability L1tf: Not affected
Vulnerability Mds: Not affected
Vulnerability Meltdown: Not affected
Vulnerability Mmio stale data: Not affected
Vulnerability Reg file data sampling: Not affected
Vulnerability Retbleed: Not affected
Vulnerability Spec rstack overflow: Not affected
Vulnerability Spec store bypass: Mitigation; Speculative Store Bypass disabled via prctl
Vulnerability Spectre v1: Mitigation; __user pointer sanitization
Vulnerability Spectre v2: Mitigation; CSV2, BHB
Vulnerability Srbds: Not affected
Vulnerability Tsa: Not affected
Vulnerability Tsx async abort: Not affected
Vulnerability Vmscape: Not affected
```
</details>
Comparing fix-ipc-block-length-allocation (a4921f116f71f5e22461d73030654214b9f662b3) to b06b9c1 (merge-base) [diff](https://github.com/apache/arrow-rs/compare/b06b9c12c17dca4f575c9c3e5a091fdb9412bab7..a4921f116f71f5e22461d73030654214b9f662b3)
<details><summary>Run configuration</summary>
```yaml
run benchmark ipc_reader
```
</details>
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench ipc_reader
Results will be posted here when complete
---
[File an issue](https://github.com/adriangb/datafusion-benchmarking/issues) against this benchmark runner
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(ipc): validate block lengths before allocation [arrow-rs]
Posted by "alamb (via GitHub) via github" <gi...@arrow.apache.org>.
alamb commented on PR #11012:
URL: https://github.com/apache/arrow-rs/pull/11012#issuecomment-6533213819
This PR looks abandoned and I don't see any evidence of human engagement. I am going to close it for now; Please reopen and respond to reviewer comments if you want to proceed with this
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(ipc): validate block lengths before allocation [arrow-rs]
Posted by "KHARSHAVARDHAN-eng (via GitHub)" <gi...@apache.org>.
KHARSHAVARDHAN-eng commented on PR #11012:
URL: https://github.com/apache/arrow-rs/pull/11012#issuecomment-5598139552
Thanks for the review! I’ve addressed Jeffrey’s suggestion by calculating the file length once and passing it through to read_block, avoiding repeated end-of-file seeks. The changes have been verified with formatting, clippy, and the full arrow-ipc test suite. Ready for review.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(ipc): validate block lengths before allocation [arrow-rs]
Posted by "flyovers-bot (via GitHub)" <gi...@apache.org>.
flyovers-bot commented on PR #11012:
URL: https://github.com/apache/arrow-rs/pull/11012#issuecomment-5577221099
<!-- flyovers:flyover -->
[](https://flyovers.dev/flyover/a7977e43-27a5-4886-a371-d18258fdf7f8)
**[Watch the flyover →](https://flyovers.dev/flyover/a7977e43-27a5-4886-a371-d18258fdf7f8)** · 4:14 · 6 chapters · 1 files
An animated walkthrough of this diff: context, the problem, the change, implementation care, code walkthrough, and impact.
<sub>b94fc60 · updated automatically by flyovers</sub>
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(ipc): validate block lengths before allocation [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #11012:
URL: https://github.com/apache/arrow-rs/pull/11012#discussion_r3954353532
##########
arrow-ipc/src/reader.rs:
##########
@@ -947,11 +945,37 @@ fn get_dictionary_values(
/// Read the data for a given block
fn read_block<R: Read + Seek>(mut reader: R, block: &Block) -> Result<Buffer, ArrowError> {
- reader.seek(SeekFrom::Start(block.offset() as u64))?;
- let body_len = block.bodyLength().to_usize().unwrap();
- let metadata_len = block.metaDataLength().to_usize().unwrap();
- let total_len = body_len.checked_add(metadata_len).unwrap();
+ let offset = u64::try_from(block.offset())
+ .map_err(|_| ArrowError::ParseError(format!("Invalid block offset: {}", block.offset())))?;
+ let body_len = usize::try_from(block.bodyLength()).map_err(|_| {
+ ArrowError::ParseError(format!("Invalid block body length: {}", block.bodyLength()))
+ })?;
+ let metadata_len = usize::try_from(block.metaDataLength()).map_err(|_| {
+ ArrowError::ParseError(format!(
+ "Invalid block metadata length: {}",
+ block.metaDataLength()
+ ))
+ })?;
+ let total_len = body_len.checked_add(metadata_len).ok_or_else(|| {
+ ArrowError::ParseError("Block body length + metadata length overflowed usize".to_string())
+ })?;
+ let file_len = reader.seek(SeekFrom::End(0))?;
Review Comment:
we probably should pass in the file length to the function, instead of re-seeking for every block read
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(ipc): validate block lengths before allocation [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #11012:
URL: https://github.com/apache/arrow-rs/pull/11012#discussion_r4032588970
##########
arrow-ipc/src/reader.rs:
##########
@@ -4264,4 +4299,68 @@ mod tests {
"unexpected error: {err}"
);
}
+
+ fn file_with_declared_block(body_length: i64, meta_length: i32, offset: i64) -> Vec<u8> {
+ let mut fbb = flatbuffers::FlatBufferBuilder::new();
+
+ let schema = crate::SchemaBuilder::new(&mut fbb).finish();
+ let block = crate::Block::new(offset, meta_length, body_length);
+ let blocks = fbb.create_vector(&[block]);
+
+ let mut footer = crate::FooterBuilder::new(&mut fbb);
+ footer.add_version(crate::MetadataVersion::V5);
+ footer.add_schema(schema);
+ footer.add_recordBatches(blocks);
+ let root = footer.finish();
+ fbb.finish(root, None);
+
+ let footer_data = fbb.finished_data();
+ let footer_len = footer_data.len() as i32;
+
+ let mut file = Vec::new();
+ file.extend_from_slice(&crate::ARROW_MAGIC);
+ file.extend_from_slice(&[0u8; 100]);
+ file.extend_from_slice(footer_data);
+ file.extend_from_slice(&footer_len.to_le_bytes());
+ file.extend_from_slice(&crate::ARROW_MAGIC);
+ file
+ }
+
+ /// A block's declared offset, metadata length, and body length are read from the footer,
+ /// so they cannot be trusted. Declaring implausible lengths or offsets that extend beyond
+ /// the file length must be rejected with an ArrowError before attempting any buffer allocation.
+ #[test]
+ fn test_file_reader_rejects_implausible_block_length() {
+ for (body_length, meta_length, offset) in [
+ (i64::MAX, 10, 8),
+ (1 << 50, 10, 8),
+ (-1, 10, 8),
+ (10, -1, 8),
+ (10, 10, -1),
+ (1000, 100, 8),
+ (100, 100, i64::MAX - 50),
+ ] {
+ let file = file_with_declared_block(body_length, meta_length, offset);
+ let mut reader = match FileReaderBuilder::new().build(std::io::Cursor::new(file)) {
+ Ok(reader) => reader,
+ Err(err) => {
+ assert!(
+ matches!(err, ArrowError::ParseError(_)),
+ "expected ParseError for block ({body_length}, {meta_length}, {offset}): {err}"
+ );
+ continue;
Review Comment:
we shouldnt have this approach in tests; it makes it unclear which test case here is expected to pass or fail this specific assertion. can we split the cases up to be clear which should successfully build a reader and which should fail
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(ipc): validate block lengths before allocation [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on code in PR #11012:
URL: https://github.com/apache/arrow-rs/pull/11012#discussion_r3946912516
##########
arrow-ipc/src/reader.rs:
##########
@@ -947,11 +945,37 @@ fn get_dictionary_values(
/// Read the data for a given block
fn read_block<R: Read + Seek>(mut reader: R, block: &Block) -> Result<Buffer, ArrowError> {
- reader.seek(SeekFrom::Start(block.offset() as u64))?;
- let body_len = block.bodyLength().to_usize().unwrap();
- let metadata_len = block.metaDataLength().to_usize().unwrap();
- let total_len = body_len.checked_add(metadata_len).unwrap();
+ let offset = u64::try_from(block.offset())
Review Comment:
nit: might be worth placing all the parsing Errors into a anon function that takes in a string. either way otherwise this makes sense 👍
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(ipc): validate block lengths before allocation [arrow-rs]
Posted by "alamb (via GitHub) via github" <gi...@arrow.apache.org>.
alamb closed pull request #11012: fix(ipc): validate block lengths before allocation
URL: https://github.com/apache/arrow-rs/pull/11012
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org