gortiz commented on PR #18953: URL: https://github.com/apache/pinot/pull/18953#issuecomment-4958882684
Reviewed this focusing on the two hand-rolled binary parsers (which parse untrusted stream bytes on the ingestion hot path). Overall this is careful work ā bounds arithmetic is defensive (compare-without-add to avoid overflow, sign-bit `uint64` sizes rejected), UTF-8 uses `CodingErrorAction.REPORT` so corrupt bytes fail rather than silently substitute, canonical numbers are regex-validated before the permissive `parse*`, exact-fill + container-bounded nesting are enforced, and backward compatibility (unset ā `TEXT`, `AUTO` opt-in) is preserved and tested. Nice test coverage too. One blocker, plus a few smaller notes. ## š“ Blocker: unbounded recursion in `SqliteJsonbPayloadParser` ā `StackOverflowError` takes the whole consuming segment to `ERROR` `readElement` ā `readArray`/`readObject` ā `readElement` recurses one frame per nesting level with **no depth cap** (`SqliteJsonbPayloadParser.java:162,206-208,216-237`). Each nesting level costs only ~1ā3 bytes on the wire (a `TYPE_ARRAY`/`TYPE_OBJECT` header plus its size field), so an ordinary stream message of a few KB ā well under Kafka's default 1 MB `max.message.bytes` ā produces thousands of stack frames and throws `StackOverflowError`. The problem is not the throw itself, it's that `StackOverflowError` is an `Error`, not an `Exception`, so it escapes every decode-error guard on the path and is **not** handled as a bad record: - `JSONMessageDecoder.decode` catches only `catch (Exception e)` (`JSONMessageDecoder.java:89`) ā so it's *not* wrapped in the bounded `RuntimeException` this PR documents. - `StreamDataDecoderImpl.decode` also catches only `catch (Exception e)` (`StreamDataDecoderImpl.java:96`) ā so the normal "drop the row / stop-on-decode-error" handling in `RealtimeSegmentDataManager.processStreamEvents` never runs. - The `Error` propagates out of `consumeLoop` into the consumer thread's `catch (Throwable e)` in `RealtimeSegmentDataManager.run()` (line ~1017), which sets `_state = State.ERROR`, drops `LLC_PARTITION_CONSUMING` to 0, and returns. Net effect: **a single malformed `SQLITE_JSONB` message moves the entire consuming segment into `ERROR` state** and stops the partition, instead of dropping one row. Because the offset hasn't advanced past the message, it behaves as a poison pill requiring manual intervention. Every *other* malformed input in this parser correctly throws `IllegalArgumentException` (handled gracefully) ā this recursion path is the one that escalates to segment failure, which contradicts the error-handling contract the rest of the parser upholds. Reachable via pinned `SQLITE_JSONB` (a deeply-nested but otherwise valid, exactly-filling top-level object) and via `AUTO` (same payload passes `matches()`'s exact-fill check). The Jackson-backed Smile/CBOR paths are **not** affected ā Jackson's `StreamReadConstraints` caps nesting depth (~1000) by default. Default `TEXT` is unaffected. Suggested fix: cap recursion depth in `readElement` (mirroring Jackson's ~1000 default) and throw `IllegalArgumentException` past the limit, so it flows through the existing decode-error handling. A deep-nesting regression test should accompany it ā the current tests only exercise shallow nesting (`testSqliteNested`, `testSqliteNestedElementCannotOverrunItsContainer`). ## š” Smaller notes (non-blocking) - **No cap on numeric-token length.** `parseInt` falls back to `new BigInteger(text)` for tokens that overflow `long` (`SqliteJsonbPayloadParser.java:250-257`), and `BigInteger(String)` is ~O(nĀ²). A single huge numeric token (bounded only by the payload size, so up to ~1 MB of digits) is a per-message CPU cost with no analogue in Jackson, which caps number length at ~1000 chars via `StreamReadConstraints`. Worth a length bound for parity. - **A bare `0x0C` decodes to an empty row** under `SQLITE_JSONB`/`AUTO` (`matches()` treats it as an exactly-filling empty OBJECT; `parse()` returns an empty map). Internally consistent and acknowledged by the "AUTO is a heuristic" framing, but under `AUTO` a stray `0x0C` byte becomes an empty record rather than a rejected one ā worth a line in the README's AUTO caveats. - **Trailing bytes after a valid Smile/CBOR object are ignored** (no `FAIL_ON_TRAILING_TOKENS`). This matches the existing text-JSON behavior via `JsonUtils.bytesToMap`, so it's not a regression ā noting only for completeness. The Postgres parser (version-byte strip + text-JSON body, with `matches()` requiring both the version byte and a JSON start char) looks correct to me. -- 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]
