airborne12 opened a new pull request, #69726:
URL: https://github.com/apache/doris/pull/69726
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Three decode primitives of the SNII index format take
numbers read from the file and do arithmetic on them without checking for
overflow.
- The FRQ prelude added the untrusted `sbdir_len` to the header length
before comparing it with the buffer, and summed the super-block lengths into
the window region length unchecked. A crafted length wraps the sum to a small
value, so the bounds check passes while a Slice of a huge length is cut from a
buffer that holds no such bytes, and the block decoder then reads past the end
of it. `memory_required` already compared `sbdir_len` with the remaining bytes
before calling `verify_covered_crc`, but `open` did not. The overflow check now
lives in `verify_covered_crc`, which both call, the super-block sum uses the
file's own `checked_add_u64`, and the final region check compares against the
room left instead of adding to the start offset.
- `decode_varint64` accepted a tenth byte with payload above bit 63. The
extra bits were shifted out, and the encoding decoded to a silently truncated
value. The check sits on the terminating byte, so the loop over the
continuation bytes is unchanged.
- `pfor_decode` and `pfor_skip` added each exception index delta to a 32-bit
running index and tested the sum afterwards, so a delta could wrap the index
back into the block and be accepted. The running index is now 64 bits wide: the
index is below the block size before every add and a delta is at most 2^32 - 1,
so the sum cannot wrap.
Nothing here is reachable from data the writers produce. The checks make a
damaged or crafted index file fail with a corruption status instead of being
decoded into a wrong value or an out-of-bounds read.
### Release note
None
### Compatibility
- No on-disk format change, and no writer is touched. Each check rejects
only input that no writer produces: a varint64 whose tenth byte is above 1, a
PFOR exception index that wraps past the block (the encoder writes strictly
ascending indices below the block size), and an FRQ prelude whose untrusted
lengths overflow the sums built from them. Files written by any released
version decode as before.
- `pfor.cpp`, `varint.cpp` and `frq_prelude.cpp` are identical on master and
on the branch-4.2 tip, so this change backports without conflicts.
### Performance
The three hot decoders were measured offline: the real `encoding/` and
`format/` sources compiled with `-O3`, the old and the new binary run
interleaved on one pinned core (70 rounds, 11 repetitions). Ratios are new /
old.
| metric | min | median |
|---|---|---|
| varint64, ns per value | 0.994 | 0.998 |
| PFOR block, no exceptions | 0.991 | 0.996 |
| PFOR block, 2 exceptions | 0.997 | 1.001 |
| PFOR block, 10 exceptions | 1.008 | 1.012 |
| FRQ prelude open, 64 windows | 1.002 | 0.983 |
| FRQ prelude open, 4096 windows | 1.005 | 1.014 |
Running the same code as both "old" and "new" gives ratios between 0.984 and
1.026 on this machine, so these are noise. A first version of the checks, with
a Status-returning helper called for every exception and a test on every varint
byte, cost 4% to 8% on PFOR blocks with exceptions. The varint check now sits
on the terminating byte only, and the PFOR running index is 64 bits wide, which
keeps the old range check and removes the wraparound.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- Added SniiVarint.TenthByteOverflowFails, SniiPfor.ExceptionIndex*
and SniiFrqPreludeTest.FP10 to FP12; all BE unit tests matching *Snii* pass.
With the three decoders reverted, five of the seven fail (the varint, the PFOR
wraparound and the three prelude cases). The other two pin behavior that must
not change: an index past the block is still rejected and the edge indices are
still accepted.
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason
- Behavior changed:
- [x] No.
- [ ] Yes.
- Does this need documentation?
- [x] No.
- [ ] Yes.
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
