Doris-Breakwater commented on issue #69330: URL: https://github.com/apache/doris/issues/69330#issuecomment-6286227376
Breakwater-GitHub-Analysis-Slot: slot_65aa06b4cd52 ### Assessment The reported out-of-bounds read is supported by source inspection and warrants prompt attention because successful UTF-8 validation would not establish that the returned keys are correct. The upstream origin of the inconsistent path/depth pair is still unconfirmed. This issue currently has no labels; suggested triage areas are VARIANT, storage reads, and correctness. I inspected the local upstream `4.1.4` tag (`4accb51bc1f27d84980668f3a2eaf0ac96fb3cba`) and available master snapshot (`57e8aa36ebac600037852c1c564bc0a5648cad60`). This is a read-only source investigation, without an end-to-end reproduction or ASan run. The precise deployed FE/BE commit IDs remain necessary to match the running binaries. ### Verified code behavior - In [`ObjectEmitter`](https://github.com/apache/doris/blob/4accb51bc1f27d84980668f3a2eaf0ac96fb3cba/be/src/storage/segment/variant/v2/variant_assembler.cpp), `open_suffix` iterates by the supplied depth. Once `offset > path.size`, the debug-only assertion in `component_end` does not protect release builds, and unsigned subtraction makes the `memchr` length wrap. The subsequent key slice can therefore include unrelated memory. This defect is present in both inspected snapshots. `common_prefix_depth` already bounds both offsets in its loop in these snapshots. - The two merge sources derive depth differently. Materialized values borrow raw bytes from the assembler-owned `MaterializedSlot::relative_path.get_path()` but take depth from `get_parts().size()`. Sparse/doc values derive depth from the same raw slice they pass onward; an empty non-direct slice gives depth **1**, not 6. Thus an empty path with depth 6 particularly warrants checking the materialized path/parts invariant, while corruption elsewhere remains possible. - Both `next_batch` and `read_by_rowids` use the shared [`process_read`](https://github.com/apache/doris/blob/4accb51bc1f27d84980668f3a2eaf0ac96fb3cba/be/src/storage/segment/variant/hierarchical_data_iterator.h). It reads all streams, assembles synchronously, and clears physical columns **after** successful assembly. Materialized path metadata is separately owned by the assembler. This does not establish that rowid-batch cleanup cleared a materialized path. A full scan can also request a different subtree and construct different relative-path metadata. ### Additional concrete lead, not a confirmed trigger [`PathInData::operator=` / `build_parts`](https://github.com/apache/doris/blob/4accb51bc1f27d84980668f3a2eaf0ac96fb3cba/be/src/util/json/path_in_data.cpp) contain an independently identifiable invariant hazard: assigning a default empty `PathInData` to an existing nonempty instance replaces the raw string with an empty string, then `build_parts` returns early for empty source parts without clearing the destination's old parts. That leaves empty raw bytes with a nonzero part count. `build_materialized_slots` sorts records containing `PathInData`, so assignments involving logical-root relative paths deserve targeted examination. I have **not** established that this assignment occurs in the affected query or causes its failure; please verify it before attributing the incident to it. ### Suggested next steps and missing evidence 1. Please open the proposed guard PR. Reject an inconsistent split with an explicit error propagated through assembly; simply stopping the loop could publish an incomplete object. Check bounds before pointer arithmetic/subtraction and preserve atomic output publication. An empty string alone is not invalid: the logical root has depth 0, while an empty object key can legitimately have depth 1. Cover trailing/adjacent empty components as well. 2. Capture the first mismatch at its producer: whether `materialized` is non-null, slot/batch index, requested subtree, `is_direct_subtree_value`, raw length, depth, and original versus relative path component counts. Compare metadata immediately after slot creation/sorting, before assembly, and at `set_materialized`/`set_cell`. Use bounded, sanitized diagnostics; do not post arbitrary heap contents. 3. Supply exact FE/BE build hashes, sanitized DDL/table VARIANT properties and load history establishing V1 storage, the failing SQL and `EXPLAIN`, the successful comparison query, and a minimized data/load sequence if available. Include the first ASan report and symbolized stack when reproducible. Existing instrumentation is valuable, but it does not yet prove which object first became inconsistent or independently establish that every affected persisted path is valid. 4. Add targeted regression coverage for nonempty-to-empty `PathInData` assignment and mixed root/descendant slot sorting, then repeated `read_by_rowids` batches over V1 materialized+sparse data with V2 output. Compare identical projections and row IDs against sequential reads, including depth ≥6, missing/null values, empty keys, and keys containing `/`. Assert exact keys/values as well as absence of errors. The bounds defect can be addressed independently, but closing this issue should also require identifying the first path/parts inconsistency and a regression that exercises its producer. No verified workaround or fixed release is established by this investigation. -- 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]
