grafana / grafana/pyroscope-jfr-parser

bug: missing continue in T_ALLOC_SAMPLE skip branch causes nil pointer dereference

Open
#89 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
49
Forks
24
PR merge metrics
No merged PRs in 30d

Description

## Description

In `parser/parser.go`, the `T_ALLOC_SAMPLE` case in `ParseEvent` is missing a `continue` statement after the skip branch, unlike every other event type. This means when `bindAllocSample == nil` (i.e. the event type was not found in the JFR metadata), the code skips to the correct position **but then immediately falls through** to call `Parse` with a nil `bind` pointer.

## Code

```go
// parser/parser.go ~line 199
case p.TypeMap.T_ALLOC_SAMPLE:
if p.bindAllocSample == nil {
p.pos = pp + int(size) // skip
// ← missing `continue` here!
}
_, err := p.ObjectAllocationSample.Parse(p.buf[p.pos:], p.bindAllocSample, &p.TypeMap)
```

Compare to every other event type, e.g.:

```go
case p.TypeMap.T_LIVE_OBJECT:
if p.bindLiveObject == nil {
p.pos = pp + int(size) // skip
continue // ← present here
}
```

## Impact

When `jdk.ObjectAllocationSample` is absent from the JFR metadata (which is valid — it's an optional event), `bindAllocSample` is nil and `T_ALLOC_SAMPLE` is set to `-1`. However, if another event type happens to have TypeID `-1`, the nil `bind` will be passed into `ObjectAllocationSample.Parse`, which will likely panic or produce corrupted output.

In practice `T_ALLOC_SAMPLE == -1` means the switch case is never matched under normal conditions (since no real event has type -1), so this bug is latent. It would surface if the JFR event type ID space ever collided with `-1`, or if the default value for unset TypeIDs is changed.

## Fix

Add `continue` after the skip branch:

```go
case p.TypeMap.T_ALLOC_SAMPLE:
if p.bindAllocSample == nil {
p.pos = pp + int(size) // skip
continue
}
_, err := p.ObjectAllocationSample.Parse(p.buf[p.pos:], p.bindAllocSample, &p.TypeMap)
```

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.