Doris-Breakwater commented on issue #69859: URL: https://github.com/apache/doris/issues/69859#issuecomment-6503411411
Source inspection confirms an unbounded-deserialization defect worth fixing. However, the proposed direct replacement with `Roaring::readSafe(buf, size)` needs a format-compatibility correction, and the fix needs to cover additional CLucene call sites. I checked the repository's `4.1.4` tag (`ad35a140c7fd`), current master (`2a4191c1dc94`), and their pinned CLucene dependency (`08d45e541fef`). This is a read-only code investigation; I have not reproduced a BE crash or run a patched build. The issue currently has no labels; BE/storage/inverted-index ownership looks appropriate. **Verified code findings** - The null-bitmap reader allocates and fills a buffer of known length, then calls `Roaring::read(..., false)` without that length. Its local handler catches `CLuceneError`, not standard-library exceptions. The BKD visitor likewise discards `doc_id.size()` when decoding. Both calls exist in [4.1.4's reader](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/storage/index/inverted/inverted_index_reader.cpp#L157) and [master's CLucene reader](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/be/src/storage/index/inverted/clucene/clucene_index_reader.cpp#L157). A decoded null-bitmap cache hit returns before parsing, which matters when constructing a reproducer. - These files contain **non-portable** Roaring serialization: the [null-bitmap writer](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/storage/index/inverted/inverted_index_writer.cpp#L567) and [BKD bitmap writer](https://github.com/apache/doris-thirdparty/blob/08d45e541fef1e48dc4ab00d2d5fde9569b49ee3/src/core/CLucene/util/bkd/docids_writer.cpp#L9) both use `write(..., false)`. Doris pins [CRoaring 2.1.2](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/thirdparty/vars.sh#L319). In that version, [`Roaring::readSafe`](https://github.com/RoaringBitmap/CRoaring/blob/v2.1.2/cpp/roaring.hh#L642) is portable-only. The matching bounded native-format API already exists: [`roaring::api::roaring_bitmap_deserialize_safe(buf, size)`](https://github.com/RoaringBitmap/CRoaring/blob/v2.1.2/src/roaring.c#L1566). Check its result before constructing the owning C++ `Roaring` object. `frozenView` is also not a compatible replacement: it expects frozen-format bytes, alignment, and a backing buffer that outlives the view. [Frozen-format contract](https://github.com/RoaringBitmap/CRoaring/blob/v2.1.2/include/roaring/roaring.h#L664). - The two reported calls are not the complete scope. The pinned dependency has additional unbounded reads in [`docids_writer.cpp`](https://github.com/apache/doris-thirdparty/blob/08d45e541fef1e48dc4ab00d2d5fde9569b49ee3/src/core/CLucene/util/bkd/docids_writer.cpp#L62), including the bitmap/low-cardinality visitor paths. Compaction separately decodes null bitmaps in [`IndexWriter::indexCompaction`](https://github.com/apache/doris-thirdparty/blob/08d45e541fef1e48dc4ab00d2d5fde9569b49ee3/src/core/CLucene/index/IndexWriter.cpp#L1242); it does not reuse the Doris query reader. Its local `catch (CLuceneError&)` closes the input and continues, so merely converting a decode failure into that exception could swallow the error. Compaction failure propagation must be checked explicitly. - [SNII](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/be/src/storage/index/inverted/snii/format/null_bitmap.cpp#L208) writes portable bytes and uses bounded decoding after framing/length checks. That explains its different API choice; it does not justify changing the legacy reader's expected encoding. **Recommended PR direction** 1. Use a bounded decoder compatible with existing native-format bytes across the Doris and CLucene paths above. Preserve old-file compatibility. Reject invalid lengths before allocation or narrowing conversions; some dependency BKD readers allocate `char buf[size]` from an on-disk length before decoding. 2. Report malformed input as a corruption status, preserve input/directory cleanup, and propagate failure through query and compaction boundaries. Contain standard exceptions appropriately, but distinguish allocation/resource failures from proven corrupt data. Catching exceptions alone cannot prevent an out-of-bounds read or catch SIGSEGV. 3. Validate decoded structure and relevant document-ID bounds before bitmap operations or compaction remapping. Bounded parsing is not complete semantic validation: CRoaring provides [`roaring_bitmap_internal_validate`](https://github.com/RoaringBitmap/CRoaring/blob/v2.1.2/include/roaring/roaring.h#L859), and compaction subsequently indexes `_trans_vec[i][srcDocId]` in [`mergeNullBitmap`](https://github.com/apache/doris-thirdparty/blob/08d45e541fef1e48dc4ab00d2d5fde9569b49ee3/src/core/CLucene/index/IndexWriter.cpp#L2043). A syntactically valid bitmap can still contain an out-of-range document ID. 4. Add compatibility tests for empty, sparse and run-optimized existing bitmaps, plus malformed/truncated payloads, invalid sizes and invalid document IDs. Exercise null predicates, BKD visitor paths and index compaction under ASAN. Verify an error reaches the caller, the BE remains alive, and corrupt data is neither cached nor silently treated as an empty bitmap. **Evidence still needed** - Exact deployed BE build/commit and symbolized crash stack, with the fatal/exception log. These distinguish an out-of-bounds read, an uncaught exception and a failure before deserialization. - Minimal table/index DDL, inserts and query, index storage format, and the precise byte mutation/truncation procedure or a sanitized corrupt bitmap sample. Include byte length and whether the decoded bitmap cache was missed. A physical truncation can fail during `readBytes` before reaching Roaring, so the exact mutation matters. - If cache corruption is suspected, compare the affected cached byte range with the same remote object/range. Cache damage, an incorrect slice and remote-file corruption remain hypotheses; the issue does not establish which occurred. Remote reread/eviction should be a separate, evidence-driven follow-up and cannot repair a corrupt remote copy. The source establishes the unsafe decoding mechanism and incomplete local exception handling. It does not yet establish the reported production crash's exact trigger or guarantee that bounded parsing alone handles every kind of corrupt bitmap. Breakwater-GitHub-Analysis-Slot: slot_37842081e10a -- 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]
