Skip to content

fix(interpreter/go): check file ID before symbolizing native frames#1324

Merged
fabled merged 3 commits into
open-telemetry:mainfrom
grafana:kk/cat-symbols-bug-repro
Apr 7, 2026
Merged

fix(interpreter/go): check file ID before symbolizing native frames#1324
fabled merged 3 commits into
open-telemetry:mainfrom
grafana:kk/cat-symbols-bug-repro

Conversation

@korniltsev-grafanista
Copy link
Copy Markdown
Contributor

I accidentally noticed this weird problem when there is a incorrect go frame in the middle of libc.

=== resource: pid=120764 exe=cat path=/usr/bin/cat ===

  profile: time=2026-03-30T00:25:02.327594565Z duration=5.214106076s sampleType=samples/count samples=71

  sample #0  values=[]  timestamps=1
    attrs: thread.name=cat thread.id=120764 cpu.logical_number=1
    stack (15 frames):
      [0] 0xc99575 chacha_permute  [vmlinux kernel]
      [1] 0xc99709 chacha_block_generic  [vmlinux kernel]
      [2] 0xf52cea get_random_bytes_user  [vmlinux kernel]
      [3] 0x81f487 vfs_read  [vmlinux kernel]
      [4] 0x81fff2 ksys_read  [vmlinux kernel]
      [5] 0x15ec9cd do_syscall_64  [vmlinux kernel]
      [6] 0x12e entry_SYSCALL_64_after_hwframe  [vmlinux kernel]
      [7] 0x6ec5d  [libc.so.6 native]
      [8] 0x6ec83 runtime.traceReadCPU /usr/lib/golang/src/runtime/tracecpu.go:142  [libc.so.6 go] <<< o_O
      [9] 0xe873d  [libc.so.6 native]
      [10] 0x26dd  [cat native]
      [11] 0x1b0a  [cat native]
      [12] 0x35b4  [libc.so.6 native]
      [13] 0x3667  [libc.so.6 native]
      [14] 0x2224  [cat native]

From my libc:

pwndbg> x/2i 0x6ec7f
   0x6ec7f <__syscall_cancel+15>:       call   0x6ebe0 <__internal_syscall_cancel>
   0x6ec84 <__syscall_cancel+20>:       pop    rdx

Looking at the go symbolizer it looks like it is symbolizing libc files, which does not seem right.

I managed to dump one of requests for qurious.
otlp-cat-request.pb.bin.zst.zip

Found this on revision 7d44c856bf413930cd0a1b76275ffe4a7a19e1bb

Seeking for someone to proof read my test and the fix.

@korniltsev-grafanista korniltsev-grafanista requested review from a team as code owners April 3, 2026 10:04
Comment thread interpreter/go/go.go Outdated
if !ef.Type().IsInterpType(libpf.Native) {
return interpreter.ErrMismatchInterpreterType
}
if ef.Length() != 2 || host.FileID(ef.Variable(0)) != g.d.fileID {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a comment here to clarify what this check is guarding against? (native frames that do not belong to the Go binary)

Also do we need the ef.Length() != 2 check? Don't all native frames have length 2? Maybe also add a comment to clarify?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment as suggested.

I would prefer something like this though

func (d *goData) canSymbolize(ef libpf.EbpfFrame) bool {
  if !ef.Type().IsInterpType(libpf.Native) {
          return false
  }
  fid := host.FileID(ef.Variable(0))
  return fid == d.fileID
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I added the length check as a I vaguely remembered a panic at grafana parsing frames, but now I see the panic was due to corrupted pointers, the length were fine, so we can probably trust ebpf programs and remove the len check.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment as suggested.

I would prefer something like this though

That's also fine.

Copy link
Copy Markdown
Member

@christos68k christos68k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fabled fabled merged commit 6ab4187 into open-telemetry:main Apr 7, 2026
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants