From e58e813950a630bd3d867802089773c0db2fcbf5 Mon Sep 17 00:00:00 2001 From: Dominik Honnef Date: Wed, 27 Dec 2023 22:01:19 +0100 Subject: [PATCH] internal/trace/v2: avoid several panics for malformed traces This addresses some panics (out of bounds slice accesses and nil pointer dereferences) when parsing malformed data. These were found via light fuzzing, not by any rigorous means, and more potential panics probably exist. Fixes #64878. Fixes #64879. Change-Id: I4085788ba7dc91fec62e4abd88f50777577db42f Reviewed-on: https://go-review.googlesource.com/c/go/+/552995 Auto-Submit: Michael Knyszek Reviewed-by: Michael Knyszek LUCI-TryBot-Result: Go LUCI Reviewed-by: Dmitri Shuralyov --- src/internal/trace/v2/base.go | 9 +++- src/internal/trace/v2/batchcursor.go | 8 +++- src/internal/trace/v2/order.go | 23 ++++++++- src/internal/trace/v2/reader.go | 3 ++ src/internal/trace/v2/reader_test.go | 47 +++++++++++++++++++ .../testdata/fuzz/FuzzReader/0cb1786dee0f090b | 2 + .../testdata/fuzz/FuzzReader/1e45307d5b2ec36d | 2 + .../testdata/fuzz/FuzzReader/2b05796f9b2fc48d | 2 + .../testdata/fuzz/FuzzReader/2b9be9aebe08d511 | 2 + .../testdata/fuzz/FuzzReader/344331b314da0b08 | 2 + .../testdata/fuzz/FuzzReader/365d7b5b633b3f97 | 2 + .../testdata/fuzz/FuzzReader/56f073e57903588c | 2 + .../testdata/fuzz/FuzzReader/aeb749b6bc317b66 | 2 + .../fuzz/FuzzReader/closing-unknown-region | 2 + .../testdata/fuzz/FuzzReader/d478e18d2d6756b7 | 2 + .../testdata/fuzz/FuzzReader/d91203cd397aa0bc | 2 + .../fuzz/FuzzReader/invalid-proc-state | 2 + .../v2/testdata/fuzz/FuzzReader/large-id | 2 + .../fuzz/FuzzReader/malformed-timestamp | 2 + 19 files changed, 113 insertions(+), 5 deletions(-) create mode 100644 src/internal/trace/v2/testdata/fuzz/FuzzReader/0cb1786dee0f090b create mode 100644 src/internal/trace/v2/testdata/fuzz/FuzzReader/1e45307d5b2ec36d create mode 100644 src/internal/trace/v2/testdata/fuzz/FuzzReader/2b05796f9b2fc48d create mode 100644 src/internal/trace/v2/testdata/fuzz/FuzzReader/2b9be9aebe08d511 create mode 100644 src/internal/trace/v2/testdata/fuzz/FuzzReader/344331b314da0b08 create mode 100644 src/internal/trace/v2/testdata/fuzz/FuzzReader/365d7b5b633b3f97 create mode 100644 src/internal/trace/v2/testdata/fuzz/FuzzReader/56f073e57903588c create mode 100644 src/internal/trace/v2/testdata/fuzz/FuzzReader/aeb749b6bc317b66 create mode 100644 src/internal/trace/v2/testdata/fuzz/FuzzReader/closing-unknown-region create mode 100644 src/internal/trace/v2/testdata/fuzz/FuzzReader/d478e18d2d6756b7 create mode 100644 src/internal/trace/v2/testdata/fuzz/FuzzReader/d91203cd397aa0bc create mode 100644 src/internal/trace/v2/testdata/fuzz/FuzzReader/invalid-proc-state create mode 100644 src/internal/trace/v2/testdata/fuzz/FuzzReader/large-id create mode 100644 src/internal/trace/v2/testdata/fuzz/FuzzReader/malformed-timestamp diff --git a/src/internal/trace/v2/base.go b/src/internal/trace/v2/base.go index e7cee29a88..57e5802902 100644 --- a/src/internal/trace/v2/base.go +++ b/src/internal/trace/v2/base.go @@ -9,6 +9,7 @@ package trace import ( "fmt" + "math" "strings" "internal/trace/v2/event" @@ -123,8 +124,12 @@ func (d *dataTable[EI, E]) compactify() { minID = id } } + if maxID >= math.MaxInt { + // We can't create a slice big enough to hold maxID elements + return + } // We're willing to waste at most 2x memory. - if int(maxID-minID) > 2*len(d.sparse) { + if int(maxID-minID) > max(len(d.sparse), 2*len(d.sparse)) { return } if int(minID) > len(d.sparse) { @@ -146,7 +151,7 @@ func (d *dataTable[EI, E]) get(id EI) (E, bool) { if id == 0 { return *new(E), true } - if int(id) < len(d.dense) { + if uint64(id) < uint64(len(d.dense)) { if d.present[id/8]&(uint8(1)<<(id%8)) != 0 { return d.dense[id], true } diff --git a/src/internal/trace/v2/batchcursor.go b/src/internal/trace/v2/batchcursor.go index fe6275074a..8dc34fd22f 100644 --- a/src/internal/trace/v2/batchcursor.go +++ b/src/internal/trace/v2/batchcursor.go @@ -68,7 +68,7 @@ func readTimedBaseEvent(b []byte, e *baseEvent) (int, timestamp, error) { // Get the event type. typ := event.Type(b[0]) specs := go122.Specs() - if int(typ) > len(specs) { + if int(typ) >= len(specs) { return 0, 0, fmt.Errorf("found invalid event type: %v", typ) } e.typ = typ @@ -82,11 +82,17 @@ func readTimedBaseEvent(b []byte, e *baseEvent) (int, timestamp, error) { // Read timestamp diff. ts, nb := binary.Uvarint(b[n:]) + if nb <= 0 { + return 0, 0, fmt.Errorf("found invalid uvarint for timestamp") + } n += nb // Read the rest of the arguments. for i := 0; i < len(spec.Args)-1; i++ { arg, nb := binary.Uvarint(b[n:]) + if nb <= 0 { + return 0, 0, fmt.Errorf("found invalid uvarint") + } e.args[i] = arg n += nb } diff --git a/src/internal/trace/v2/order.go b/src/internal/trace/v2/order.go index e1abddca6c..2cc7f26d29 100644 --- a/src/internal/trace/v2/order.go +++ b/src/internal/trace/v2/order.go @@ -92,6 +92,9 @@ func (o *ordering) advance(ev *baseEvent, evt *evTable, m ThreadID, gen uint64) case go122.EvProcStatus: pid := ProcID(ev.args[0]) status := go122.ProcStatus(ev.args[1]) + if int(status) >= len(go122ProcStatus2ProcState) { + return curCtx, false, fmt.Errorf("invalid status for proc %d: %d", pid, status) + } oldState := go122ProcStatus2ProcState[status] if s, ok := o.pStates[pid]; ok { if status == go122.ProcSyscallAbandoned && s.status == go122.ProcSyscall { @@ -268,6 +271,10 @@ func (o *ordering) advance(ev *baseEvent, evt *evTable, m ThreadID, gen uint64) gid := GoID(ev.args[0]) mid := ThreadID(ev.args[1]) status := go122.GoStatus(ev.args[2]) + + if int(status) >= len(go122GoStatus2GoState) { + return curCtx, false, fmt.Errorf("invalid status for goroutine %d: %d", gid, status) + } oldState := go122GoStatus2GoState[status] if s, ok := o.gStates[gid]; ok { if s.status != status { @@ -758,7 +765,11 @@ func (o *ordering) advance(ev *baseEvent, evt *evTable, m ThreadID, gen uint64) // ever reference curCtx.P. However, be lenient about this like we are with // GCMarkAssistActive; there's no reason the runtime couldn't change to block // in the middle of a sweep. - if err := o.pStates[pid].activeRange(makeRangeType(typ, 0), gen == o.initialGen); err != nil { + pState, ok := o.pStates[pid] + if !ok { + return curCtx, false, fmt.Errorf("encountered GCSweepActive for unknown proc %d", pid) + } + if err := pState.activeRange(makeRangeType(typ, 0), gen == o.initialGen); err != nil { return curCtx, false, err } return curCtx, true, nil @@ -790,7 +801,11 @@ func (o *ordering) advance(ev *baseEvent, evt *evTable, m ThreadID, gen uint64) // N.B. Like GoStatus, this can happen at any time, because it can // reference a non-running goroutine. Don't check anything about the // current scheduler context. - if err := o.gStates[gid].activeRange(makeRangeType(typ, 0), gen == o.initialGen); err != nil { + gState, ok := o.gStates[gid] + if !ok { + return curCtx, false, fmt.Errorf("uninitialized goroutine %d found during %s", gid, go122.EventString(typ)) + } + if err := gState.activeRange(makeRangeType(typ, 0), gen == o.initialGen); err != nil { return curCtx, false, err } return curCtx, true, nil @@ -917,6 +932,10 @@ func (s *gState) beginRegion(r userRegion) error { // endRegion ends a user region on the goroutine. func (s *gState) endRegion(r userRegion) error { + if len(s.regions) == 0 { + // We do not know about regions that began before tracing started. + return nil + } if next := s.regions[len(s.regions)-1]; next != r { return fmt.Errorf("misuse of region in goroutine %v: region end %v when the inner-most active region start event is %v", s.id, r, next) } diff --git a/src/internal/trace/v2/reader.go b/src/internal/trace/v2/reader.go index 446b2add30..824ca23df3 100644 --- a/src/internal/trace/v2/reader.go +++ b/src/internal/trace/v2/reader.go @@ -157,6 +157,9 @@ func (r *Reader) ReadEvent() (e Event, err error) { } // Try to advance the head of the frontier, which should have the minimum timestamp. // This should be by far the most common case + if len(r.frontier) == 0 { + return Event{}, fmt.Errorf("broken trace: frontier is empty:\n[gen=%d]\n\n%s\n%s\n", r.gen.gen, dumpFrontier(r.frontier), dumpOrdering(&r.order)) + } bc := r.frontier[0] if ctx, ok, err := r.order.advance(&bc.ev, r.gen.evTable, bc.m, r.gen.gen); err != nil { return Event{}, err diff --git a/src/internal/trace/v2/reader_test.go b/src/internal/trace/v2/reader_test.go index 4f00002e37..393e1c80b0 100644 --- a/src/internal/trace/v2/reader_test.go +++ b/src/internal/trace/v2/reader_test.go @@ -46,6 +46,53 @@ func TestReaderGolden(t *testing.T) { } } +func FuzzReader(f *testing.F) { + // Currently disabled because the parser doesn't do much validation and most + // getters can be made to panic. Turn this on once the parser is meant to + // reject invalid traces. + const testGetters = false + + f.Fuzz(func(t *testing.T, b []byte) { + r, err := trace.NewReader(bytes.NewReader(b)) + if err != nil { + return + } + for { + ev, err := r.ReadEvent() + if err != nil { + break + } + + if !testGetters { + continue + } + // Make sure getters don't do anything that panics + switch ev.Kind() { + case trace.EventLabel: + ev.Label() + case trace.EventLog: + ev.Log() + case trace.EventMetric: + ev.Metric() + case trace.EventRangeActive, trace.EventRangeBegin: + ev.Range() + case trace.EventRangeEnd: + ev.Range() + ev.RangeAttributes() + case trace.EventStateTransition: + ev.StateTransition() + case trace.EventRegionBegin, trace.EventRegionEnd: + ev.Region() + case trace.EventTaskBegin, trace.EventTaskEnd: + ev.Task() + case trace.EventSync: + case trace.EventStackSample: + case trace.EventBad: + } + } + }) +} + func testReader(t *testing.T, tr io.Reader, exp *testtrace.Expectation) { r, err := trace.NewReader(tr) if err != nil { diff --git a/src/internal/trace/v2/testdata/fuzz/FuzzReader/0cb1786dee0f090b b/src/internal/trace/v2/testdata/fuzz/FuzzReader/0cb1786dee0f090b new file mode 100644 index 0000000000..326ebe1c6e --- /dev/null +++ b/src/internal/trace/v2/testdata/fuzz/FuzzReader/0cb1786dee0f090b @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("go 1.22 trace\x00\x00\x00\x01\x0100\x85\x00\x190000\x01\x0100\x88\x00\b0000000") \ No newline at end of file diff --git a/src/internal/trace/v2/testdata/fuzz/FuzzReader/1e45307d5b2ec36d b/src/internal/trace/v2/testdata/fuzz/FuzzReader/1e45307d5b2ec36d new file mode 100644 index 0000000000..406af9caa6 --- /dev/null +++ b/src/internal/trace/v2/testdata/fuzz/FuzzReader/1e45307d5b2ec36d @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("go 1.22 trace\x00\x00\x00\x01000\x85\x00\b0001") \ No newline at end of file diff --git a/src/internal/trace/v2/testdata/fuzz/FuzzReader/2b05796f9b2fc48d b/src/internal/trace/v2/testdata/fuzz/FuzzReader/2b05796f9b2fc48d new file mode 100644 index 0000000000..50fdccda6b --- /dev/null +++ b/src/internal/trace/v2/testdata/fuzz/FuzzReader/2b05796f9b2fc48d @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("go 1.22 trace\x00\x00\x00\x01\x0100\x85\x00-0000\x01\x0100\x88\x00\b0000000") \ No newline at end of file diff --git a/src/internal/trace/v2/testdata/fuzz/FuzzReader/2b9be9aebe08d511 b/src/internal/trace/v2/testdata/fuzz/FuzzReader/2b9be9aebe08d511 new file mode 100644 index 0000000000..6bcb99adfc --- /dev/null +++ b/src/internal/trace/v2/testdata/fuzz/FuzzReader/2b9be9aebe08d511 @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("go 1.22 trace\x00\x00\x00\x01\x0100\x85\x00\x0f00\x120\x01\x0100\x88\x00\b0000000") \ No newline at end of file diff --git a/src/internal/trace/v2/testdata/fuzz/FuzzReader/344331b314da0b08 b/src/internal/trace/v2/testdata/fuzz/FuzzReader/344331b314da0b08 new file mode 100644 index 0000000000..de6e4694be --- /dev/null +++ b/src/internal/trace/v2/testdata/fuzz/FuzzReader/344331b314da0b08 @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("go 1.22 trace\x00\x00\x00\x01\x0100\x85\x00\b0000\x01\x01\xff00\xb8\x00\x1900\xff\xff\xff\xff\xff\xff\xff\xff\xff\x01\x04\x1900\xff\xff\xff\xff\xff\xff\xff\xff\xff\x01\x04\x1900\xff\xff\xff\xff\xff\xff\xff\xff\xff\x01\x04\x1901\xff\xff\xff\xff\xff\xff\xff\xff0\x800") \ No newline at end of file diff --git a/src/internal/trace/v2/testdata/fuzz/FuzzReader/365d7b5b633b3f97 b/src/internal/trace/v2/testdata/fuzz/FuzzReader/365d7b5b633b3f97 new file mode 100644 index 0000000000..8dc370f383 --- /dev/null +++ b/src/internal/trace/v2/testdata/fuzz/FuzzReader/365d7b5b633b3f97 @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("go 1.22 trace\x00\x00\x00\x0100\x8c0\x85\x00\b0000") \ No newline at end of file diff --git a/src/internal/trace/v2/testdata/fuzz/FuzzReader/56f073e57903588c b/src/internal/trace/v2/testdata/fuzz/FuzzReader/56f073e57903588c new file mode 100644 index 0000000000..d34fe3f06c --- /dev/null +++ b/src/internal/trace/v2/testdata/fuzz/FuzzReader/56f073e57903588c @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("go 1.22 trace\x00\x00\x00\x01\x0100\x85\x00\x1f0000\x01\x0100\x88\x00\b0000000") \ No newline at end of file diff --git a/src/internal/trace/v2/testdata/fuzz/FuzzReader/aeb749b6bc317b66 b/src/internal/trace/v2/testdata/fuzz/FuzzReader/aeb749b6bc317b66 new file mode 100644 index 0000000000..f93b5a90da --- /dev/null +++ b/src/internal/trace/v2/testdata/fuzz/FuzzReader/aeb749b6bc317b66 @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("go 1.22 trace\x00\x00\x00\x01000\x85\x00\b0000") \ No newline at end of file diff --git a/src/internal/trace/v2/testdata/fuzz/FuzzReader/closing-unknown-region b/src/internal/trace/v2/testdata/fuzz/FuzzReader/closing-unknown-region new file mode 100644 index 0000000000..7433214030 --- /dev/null +++ b/src/internal/trace/v2/testdata/fuzz/FuzzReader/closing-unknown-region @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("go 1.22 trace\x00\x00\x00\x01\x01\xff\xff\xff\xff\xff\xff\xff\xff\xff\x01\x87ߕ\xb4\x99\xb2\x06\x05\b\xa8ֹ\a\x01\x01\xf6\x9f\n\x9fÕ\xb4\x99\xb2\x06\x11\r\xa7\x02\x00\x01\x19\x05\x01\xf6\x9f\n\x02+\x04\x01\x00\x00") \ No newline at end of file diff --git a/src/internal/trace/v2/testdata/fuzz/FuzzReader/d478e18d2d6756b7 b/src/internal/trace/v2/testdata/fuzz/FuzzReader/d478e18d2d6756b7 new file mode 100644 index 0000000000..3e5fda833a --- /dev/null +++ b/src/internal/trace/v2/testdata/fuzz/FuzzReader/d478e18d2d6756b7 @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("go 1.22 trace\x00\x00\x00\x01\x0100\x85\x00\"0000\x01\x0100\x88\x00\b0000000") \ No newline at end of file diff --git a/src/internal/trace/v2/testdata/fuzz/FuzzReader/d91203cd397aa0bc b/src/internal/trace/v2/testdata/fuzz/FuzzReader/d91203cd397aa0bc new file mode 100644 index 0000000000..d24b94ac97 --- /dev/null +++ b/src/internal/trace/v2/testdata/fuzz/FuzzReader/d91203cd397aa0bc @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("go 1.22 trace\x00\x00\x00\x01001\x85\x00\b0000") \ No newline at end of file diff --git a/src/internal/trace/v2/testdata/fuzz/FuzzReader/invalid-proc-state b/src/internal/trace/v2/testdata/fuzz/FuzzReader/invalid-proc-state new file mode 100644 index 0000000000..e5d3258111 --- /dev/null +++ b/src/internal/trace/v2/testdata/fuzz/FuzzReader/invalid-proc-state @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("go 1.22 trace\x00\x00\x00\x01\x01\xff\xff\xff\xff\xff\xff\xff\xff\xff\x01\x87ߕ\xb4\x99\xb2\x06\x05\b\xa8ֹ\a\x01\x01\xff\xff\xff\xff\xff\xff\xff\xff\xff\x01\x94镴\x99\xb2\x06\x05\r\xa7\x02\x00E") \ No newline at end of file diff --git a/src/internal/trace/v2/testdata/fuzz/FuzzReader/large-id b/src/internal/trace/v2/testdata/fuzz/FuzzReader/large-id new file mode 100644 index 0000000000..0fb6273b44 --- /dev/null +++ b/src/internal/trace/v2/testdata/fuzz/FuzzReader/large-id @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("go 1.22 trace\x00\x00\x00\x01\x01\xff\xff\xff\xff\xff\xff\xff\xff\xff\x01\x87ߕ\xb4\x99\xb2\x06\x05\b\xa8ֹ\a\x01\x01\xff\xff\xff\xff\xff\xff\xff\xff\xff\x01\x94镴\x99\xb2\x06\f\x02\x03\xff\xff\xff\xff\xff\xff\xff\x9f\x1d\x00") \ No newline at end of file diff --git a/src/internal/trace/v2/testdata/fuzz/FuzzReader/malformed-timestamp b/src/internal/trace/v2/testdata/fuzz/FuzzReader/malformed-timestamp new file mode 100644 index 0000000000..850ca50f87 --- /dev/null +++ b/src/internal/trace/v2/testdata/fuzz/FuzzReader/malformed-timestamp @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("go 1.22 trace\x00\x00\x00\x01\x01\xff\xff\xff\xff\xff\xff\xff\xff\xff\x01\x87ߕ\xb4\x99\xb2\x06\x05\b\xa8ֹ\a\x01\x01\xfa\x9f\n\xa5ѕ\xb4\x99\xb2\x06\x0e\n\x97\x96\x96\x96\x96\x96\x96\x96\x96\x96\x01\x01\x01") -- 2.48.1