airborne12 opened a new pull request, #69793:
URL: https://github.com/apache/doris/pull/69793
### What problem does this PR solve?
Issue Number: None
Related PR: #69726
Problem Summary: Backport #69726 to `branch-4.2`.
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.
#### Branch adaptations
The three decoders (`pfor.cpp`, `varint.cpp`, `frq_prelude.cpp`) are
identical on `branch-4.2` and apply unchanged, and so do `varint_test.cpp` and
`snii_frq_prelude_test.cpp`. `pfor_test.cpp` conflicted only in context:
`branch-4.2` keeps the longer comment block above its encoder tests, so the new
tests are added before it. The commit is the cherry-pick of master `70cdb58e94`.
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- The tests of #69726: SniiVarint.TenthByteOverflowFails,
SniiPfor.ExceptionIndex* and SniiFrqPreludeTest.FP10 to FP12.
- [ ] 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.
### Validation
- The three decoders and the new tests are byte-identical to master's merged
commit `70cdb58e94`. The only difference in `pfor_test.cpp` is comments of
existing tests that already differ between the two branches.
- clang-format 16 on the six changed files.
- A local ASAN run of the BE unit tests on this branch is in progress and
will be added here. The PR's CI runs the same tests.
🤖 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]
