pkg/query: Fix nil panic in flamegraph table + top#6337
Open
AlJohri wants to merge 1 commit intoparca-dev:mainfrom
Open
pkg/query: Fix nil panic in flamegraph table + top#6337AlJohri wants to merge 1 commit intoparca-dev:mainfrom
AlJohri wants to merge 1 commit intoparca-dev:mainfrom
Conversation
AddProfileLocation (flamegraph_table.go) and GenerateTopTable (top.go) both assume every Location.Line has a non-nil Function and panic with `runtime error: invalid memory address or nil pointer dereference` when a caller passes in an unsymbolized line. The same bug existed in pkg/query/flamegraph.go and was fixed in PR parca-dev#3892 ("Fix nil panic when building flat flamegraph", dd07a12) by guarding with `if line.Function != nil`. The analogous call sites in flamegraph_table.go and top.go were missed by that patch. This commit adds the same guard in both: - AddProfileLocation: skip lines whose Function is nil - GenerateTopTable: only populate node.Meta.Function when present Reproducible on main (HEAD 43cd07b) against profile data ingested via OTLP from the OTel eBPF profiler where some locations are unresolved. Triggers REPORT_TYPE_FLAMEGRAPH_TABLE and REPORT_TYPE_TOP queries to crash the server with the stack: panic: runtime error: invalid memory address or nil pointer dereference github.com/parca-dev/parca/pkg/query.(*tableConverter).AddFunction(0xc00024e680, 0x0) pkg/query/flamegraph_table.go:312 github.com/parca-dev/parca/pkg/query.(*tableConverter).AddProfileLocation(...) pkg/query/flamegraph_table.go:284
|
|
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
2 participants
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
GenerateFlamegraphTable(flamegraph_table.go) andGenerateTopTable(top.go) both assume that everylocation.Lines[i].Functionis non-nil and panic withwhen a caller passes in an unsymbolized line (Line with no Function reference), which crashes the whole server process.
The same class of bug existed in
pkg/query/flamegraph.goand was fixed in #3892 ("Fix nil panic when building flat flamegraph", dd07a1214a) by guarding withif line.Function != nil. The analogous call sites inflamegraph_table.goandtop.gowere missed by that patch and remain onmaintoday.How to reproduce
I hit this running parca v0.27.1 as the backend for the OpenTelemetry eBPF profiler. Profiles arrive via OTLP; a subset of locations are unsymbolized (no uploaded debuginfo for that build-id yet, or no match in kallsyms), producing Line records with nil
Function.Any of these then crash parca-server:
REPORT_TYPE_PPROFandREPORT_TYPE_FLAMEGRAPH_ARROWgo through different code paths and are unaffected.Verification
Built a patched image from this branch (based on the v0.27.1 tag), deployed to our cluster, re-ran the same queries that were crashing:
REPORT_TYPE_TOP— returns 142 nodes, 127 with resolved function names, no crash.REPORT_TYPE_FLAMEGRAPH_TABLE— returns valid tree (~297 KB payload), parca UI renders the flame graph correctly.Existing
flamegraph_flat_test.go/top_test.gocontinue to pass; nil-Line case isn't covered by existing tests — happy to add targeted cases here if that'd help.