google / google/gvisor

pkg/lisafs: PReadResp.CheckedUnmarshal panics on malformed NumBytes due to uint32 truncation in bounds check

Open Beginner friendly
#14,424 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
19.3k
Forks
2k
Avg merge
3d 5h
Merged PRs (30d)
264

Description

# [Bug] pkg/lisafs: PReadResp.CheckedUnmarshal panics on malformed NumBytes (uint32 truncation in bounds check)

## Summary
`PReadResp.CheckedUnmarshal` validates `NumBytes` using **uint32-truncated** comparisons but slices `Buf` using the **full uint64** value. Any response frame whose 8-byte NumBytes has nonzero high bits passes validation and then panics with `slice bounds out of range`.

## Affected
`pkg/lisafs/message.go`, `func (r *PReadResp) CheckedUnmarshal` (~line 913):

```go
srcRemain, ok := r.NumBytes.CheckedUnmarshal(src)
if !ok || uint32(r.NumBytes) > uint32(len(srcRemain)) || uint32(r.NumBytes) > uint32(len(r.Buf)) {
return src, false
}
r.Buf = r.Buf[:r.NumBytes] // full uint64 used here
```

`uint32(0x0000000C00000000) == 0`, so all checks pass and the slice panics.

## Repro
```go
func TestReproPReadRespPanic(t *testing.T) {
in := []byte("\x00\x00\x00\x00\x0c\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00")
var m PReadResp
defer func() {
if r := recover(); r != nil {
t.Fatalf("CONFIRMED PANIC: %v", r)
}
}()
m.CheckedUnmarshal(in)
}
```
Observed: `panic: runtime error: slice bounds out of range [:51539607552] with capacity 0`

## Suggested fix
Compare at full width before slicing:
```go
if !ok || r.NumBytes > uint64(len(srcRemain)) || r.NumBytes > uint64(len(r.Buf)) {
return src, false
}
```

## Audit note
A randomized sweep of all other lisafs message `CheckedUnmarshal` implementations (8M structured mutations) found no additional panics; the visually-similar `PWriteReq.CheckedUnmarshal` guard is not affected because its NumBytes is consumed as a 32-bit quantity on this path.

## Impact context
PReadResp is parsed client-side (Sentry) from gofer responses. A malformed frame reaching this parser crashes the Sentry process hosting the sandbox's filesystem view. While gofer responses are host-generated, defensive completeness in "Checked"Unmarshal implementations is what lets the rest of lisafs treat parsing as infallible; this instance breaks that contract.

Contributor guide

Open the contributing guide

Research direction

Start in pkg/lisafs/message.go at PReadResp.CheckedUnmarshal, then run the TestReproPReadRespPanic example from the issue with the malformed NumBytes frame. Ensure malformed high-bit values are rejected without a panic, while valid responses still parse successfully; add or update focused tests near the existing lisafs message tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
operating-systems
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
88/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.