Repository navigation
fix: port upstream ClickHouse parser and detection fixes (B3 of #369) - #377
Open
mayankpande88 wants to merge 5 commits into
Open
mayankpande88 wants to merge 5 commits into
mayankpande88 wants to merge 5 commits into
Conversation
12 of 34 tasks
There was a problem hiding this comment.
Code Review
This pull request refactors the ClickHouse protocol parser by replacing the external ch-go dependency with a custom, allocation-free Go parser to prevent OOMs on corrupted payloads, and updates the corresponding eBPF tracer logic in C. The review feedback highlights critical safety issues in the eBPF code, specifically potential out-of-bounds reads in both is_clickhouse_query and is_clickhouse_response due to missing bounds checks against buf_size before bpf_read operations. Additionally, in the Go parser, it was recommended to remove an unnecessary unconditional slice truncation that could corrupt valid trailing characters, as the subsequent UTF-8 validation loop already handles partial runes.
(cherry picked from commit a8a084ff1fccfc091272082e0e79a485f1b475a7) Conflicts were with this fork's own hardening of the ch-go based parser (LimitReader, header checks, recover); upstream's parser replaces it and drops the ch-go dependency. The test function also carries the malformed length case from coroot/coroot-node-agent@2d8fb1b.
(cherry picked from commit b53b38d0dadbf7dcdae9600db960d94088f2f6bc) Conflict: this fork had added an AMQP basic.publish check to is_clickhouse_query. The stricter header validation here rejects AMQP frames anyway, and ClickHouse detection stays limited to ports 9000/8123. ebpf.go is regenerated in a later commit.
… on older kernels (cherry picked from commit 556154b5957118723739b5a3ad8e4740d545627a)
is_clickhouse_query and is_clickhouse_response read fixed offsets (initial_query_id, initial_address, the compression method, the exception code) without checking them against the payload size. The query check could not return a wrong answer, since a final bound rejects anything that read past the end, but the response check decided short reads from bytes that were not part of the payload. Check the size before every read. This diverges from upstream (b53b38d, 556154b).
mayankpande88
force-pushed
the
port/upstream-b3-l7-parsers
branch
from
October 8, 2026 12:20
08d1e10 to
8cc3943
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Third batch of #369: the ClickHouse parser and detection fixes from upstream coroot-node-agent, all cherry-picked with
-x.ch-go. Queries that carry settings were lost: their span had an emptydb.statement, and the empty parse counted as a parse failure. Garbage length prefixes can't turn into large allocations. Thech-godependency is gone.Plus one fix of our own, from review (08d1e10): the ClickHouse detection functions no longer read past the payload. The query check could not return a wrong answer, but the response check decided short reads from bytes that were not part of the payload. Every read is now checked against the payload size first. This diverges from upstream.
ebpf.gois regenerated.Not taken from B3:
L7Statshere creates no metric vector for an unknown protocol, andobserveskips nil vectors.Engineering detail
Conflicts:
ch-goparser: aLimitReader, header checks and arecover. Upstream's parser replaces all of it.basic.publishcheck tois_clickhouse_query. The stricter header validation rejects AMQP frames anyway.is_clickhouse_responsenow takes the read size, so its call intrace_exit_readpassesret.Bounds checks (08d1e10): the e2e below was re-run with them in: same counts, same statement texts. All 40 programs load on 5.10 and 6.1.
CI: gofmt, goimports, vet, golangci-lint,
go test(excluding/containers, including the new ClickHouse parser cases) and the build all pass in a Linux container with Go 1.26.5.Local e2e: I built agent binaries from this branch and from main and ran both side by side on a local Debian 12 VM (kernel 6.1), each sending spans to its own OpenTelemetry collector. A clickhouse-go v2.46 client ran 20 rounds against a local ClickHouse 25.8 on port 9000. Each round sent a plain SELECT, a SELECT with settings (max_execution_time, max_threads) and a query with a syntax error.
db.statementall 20 times.