matanper opened a new issue, #69330: URL: https://github.com/apache/doris/issues/69330
### Search before asking - [x] I had searched in the [issues](https://github.com/apache/doris/issues?q=is%3Aissue) and found no similar issues. ### Version 4.1.4 (cloud mode, arm64, `enable_variant_v2=true`). master is the same: `be/src/storage/segment/variant/v2/variant_assembler.cpp:177-186` (`ObjectEmitter::component_end`) and `:220-232` (`ObjectEmitter::open_suffix`); neither file has changed since #66858. ### Description When a VARIANT column stored in V1 layout (subcolumns + sparse paths) is read as V2, `VariantAssembler` can emit object keys built from process memory. Queries then fail with ``` [INVALID_ARGUMENT]Variant metadata key is not valid UTF-8 ``` from `VariantMetadataBuilder::register_key`, or, when the garbage happens to be valid UTF-8, return wrong keys silently. Path: `BetaRowsetReader::_next_batch` → `SegmentIterator::_read_columns_by_rowids` → `HierarchicalDataIterator::read_by_rowids` → `_assemble_variant_v2` → `VariantAssembler::assemble` → `append_visible_merge_values` → `ObjectEmitter::prepare` → `open_suffix` → `add_key` → `register_key`. `open_suffix` splits `path` into `depth` components and trusts `depth`: ```cpp for (size_t part = 0; part < depth; ++part) { const size_t end = component_end(path, offset); ... offset = end + 1; } ``` `component_end` guards `offset <= path.size` only with a `DCHECK`. If `depth` is larger than the number of components in `path`, after the last component `offset == path.size + 1`, so `path.size - offset` wraps and `memchr` scans past the buffer until it finds a `.`. The returned `end` is far beyond the path and `add_key` copies that range. We instrumented the assembler and caught it on the late-materialization path (`read_by_rowids`, after predicate filtering) only; a full scan of the same tablets (`CAST(v AS STRING)`) never hits it. Every hit looked like: ``` part=5 depth=6 common_depth=0 offset=52 end=141 path_size=0 ``` i.e. a `MergeValue` whose `raw_path` is empty while its `depth` is 6. The emitted 89-byte key was the NUL-terminated tail of an unrelated `std::string`, then a 64-byte `ColumnString` object (its vtable pointer resolves into the reading BE's own `doris_be` image and is not mapped on other BEs), then the start of the next heap string. The stored subcolumn and sparse paths themselves are valid (we checked them at read time). So the data on disk is fine; the bug is the depth/path mismatch on the `read_by_rowids` path plus the unchecked split. ### Solution 1. Make the split safe regardless of input: in `open_suffix` / `common_prefix_depth`, stop when `offset > path.size` (or derive the component count from `path` and treat a mismatch with `depth` as `Status::Corruption`), and turn the `DCHECK_LE(offset, path.size)` in `component_end` into a real check. This prevents the out-of-bounds read. 2. Fix the cause: on the `read_by_rowids` path a `MergeValue` reaches the emitter with `raw_path` empty but `depth` set. Either the raw path `StringRef` handed to `set_materialized` / `set_cell` points at a buffer that was cleared for the rowid-gathered batch, or the slot's path is not populated before assembly when columns are read by rowids rather than by `next_batch`. We have a patch for (1) and are working on (2); we can open a PR. ### How to Reproduce? 1. Table with a VARIANT column loaded with nested objects, deep enough that some paths are stored as sparse (exceed `variant_max_subcolumns_count`), with keys up to depth ≥ 6 and some keys containing `/` (Kubernetes label style, e.g. `metadata.labels.kubernetes.io/pod-name`). FE `enable_variant_v2=true`. 2. Query that filters on several `v['a']['b']...` subpaths (so the other columns are late-materialized via `read_by_rowids`) and returns a whole object subtree, e.g. `SELECT CAST(v['a']['b']['c'] AS STRING) FROM t WHERE v['x'] = ... AND v['y'] LIKE ... ORDER BY ts LIMIT n`. 3. The query fails with `Variant metadata key is not valid UTF-8` (or returns corrupted keys). The same tablets read with a plain full scan return correct data. It is data- and plan-dependent; we see it on a few tablets of large tables, not in a small synthetic test yet. ### Anything Else? An AddressSanitizer build should flag the over-read in `component_end` directly once the depth/path mismatch occurs. ### Are you willing to submit PR? - [x] Yes I am willing to submit a PR! ### Code of Conduct - [x] I agree to follow this project's [Code of Conduct](https://www.apache.org/foundation/policies/conduct) -- 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]
