When I disable crasher minimization with this patch to the toolchain

diff --git a/src/internal/fuzz/worker.go b/src/internal/fuzz/worker.go
index e3029bcd66..fca250077d 100644
--- a/src/internal/fuzz/worker.go
+++ b/src/internal/fuzz/worker.go
@@ -206,14 +206,8 @@ func (w *worker) coordinate(ctx context.Context) error {
 
                case crasher := <-w.coordinator.minimizeC:
                        // Received input to minimize from coordinator.
-                       minRes, err := w.minimize(ctx, crasher)
-                       if err != nil {
-                               // Failed to minimize. Send back the original crash.
-                               fmt.Fprintln(w.coordinator.opts.Log, err)
-                               minRes = crasher
-                               minRes.minimized = true
-                       }
-                       w.coordinator.resultC <- minRes
+                       crasher.minimized = true
+                       w.coordinator.resultC <- crasher
                }
        }
 }

the problem goes away.

Interestingly, it's always the penultimate byte of the corpus entry which is incorrect and is always equal to the last byte.

The bug seems to be caused by assigning a byte slice to the collection of minimized inputs which can be modified after assignment by minimizeBytes. This should be a copy of the data rather than a reference. This patch fixes the problem for me:

diff --git a/src/internal/fuzz/worker.go b/src/internal/fuzz/worker.go
index e3029bcd66..bab0f07036 100644
--- a/src/internal/fuzz/worker.go
+++ b/src/internal/fuzz/worker.go
@@ -787,7 +787,7 @@ func (ws *workerServer) minimizeInput(ctx context.Context, vals []interface{}, c
                case []byte:
                        switch prev.(type) {
                        case []byte:
-                               vals[valI] = c
+                               vals[valI] = append([]byte{}, c...)
                        case string:
                                vals[valI] = string(c)
                        default:

Change https://golang.org/cl/340630 mentions this issue: [dev.fuzz] internal/fuzz: avoid corruption of minimized crashers

/cc @golang/fuzzing

I can reproduce this, and confirm that your fix stops the issue from happening, but I'm not actually seeing exactly why this bug is occurring in the first place. In theory, I can see how assigning vals[valI] the way that it does would allow it to be modified later on, but I also don't see anywhere in the code where it is actually modifying it.

I'd like to figure that out before we merge the fix.

Actually I think I know what is going on. I think the fix is still a reasonable one, but here is my theory for how the memory is actually changing later on:

My suspicion is that https://github.com/golang/go/blob/dev.fuzz/src/internal/fuzz/minimize.go#L38 is the root of how this is happening. We use this same tmp slice every time, pointing to the same slots in memory. We set candidate to tmp every iteration of the loop. Let's say that one iteration finds a smaller input that works: vals[valI] will be set to candidate which is actually just pointing to tmp. Even though we create a new candidate in the next iteration of the loop, we are still setting candidate to the same tmp that we used before, thus the same memory that vals[valI] is currently pointing to. This next iteration, we change candidate, which changes tmp, which changes vals[valI], and there's our issue.

@katiehockman I think it's possible to corrupt candidates in the minimization process in the function under test even with the proposed fix. There are no guarantees that the function will not modify the contents of a byte slice. For example,

// +build gofuzzbeta

package unicode

import (
	"fmt"
	"strings"
	"testing"
)

func FuzzBeta(f *testing.F) {
	f.Fuzz(func(t *testing.T, data []byte) {
		s := string(data)
		// trash the input data
		for i := range data {
			data[i] = 'z'
		}
		valid := strings.ToValidUTF8(s, "")
		if len(valid) == 0 {
			return
		}

		lower := strings.ToLower(valid)
		upper := strings.ToUpper(lower)

		roundtrip := strings.ToLower(upper)

		if roundtrip != lower {
			panic(fmt.Sprintf("%x: %v (%x): %v (%x) != %v (%x), upper = %v (%x)", data, valid, []byte(valid), roundtrip, []byte(roundtrip), lower, []byte(lower), upper, []byte(upper)))
		}
	})
}

will crash but the input recorded in the corpus does not cause a crash due the write

		// trash the input data
		for i := range data {
			data[i] = 'z'
		}

I have a fix for this. Should I add it to #47591 / golang.org/cl/340630?

@stevenjohnstone That's a good point. I tested it locally and it definitely had some strange behavior.

It might be worth arguing though that this isn't a case that we actually have to make work, at least not right now. It seems very reasonable that we can inform developers that changing the bytes that are sent into the f.Fuzz function can cause the fuzzing engine to misbehave. They shouldn't really be doing that anyway, because it can mess with the efficiency of the fuzzing engine even if it didn't cause other problems. I would hold off on that for now.

Change https://golang.org/cl/348610 mentions this issue: [dev.fuzz] internal/fuzz: avoid incorrect bytes modification during minimization

@katiehockman I'm inclined to agree that it's likely to be a niche problem. Unfortunately, we aren't always in control of the code we fuzz. Even having the source, it may not be straightforward to tell if a function modifies its arguments. My use case for fuzzing is often to do due diligence on third-party dependencies and I've seen bugs where the author forgets that a byte slice references shared data and not a copy like a string.

The first indication you'd get of this problem would be the fuzzer finding a crasher and then losing it after minimization fails. Perhaps a mitigation to this and any future minimization bugs is to keep the original crash and a minimized version?

PS: probably a good thing to add to a fuzzing guide that the test could do

f.Fuzz(t *testing.T, b []byte) {
   tmp := append([]byte, b...)
   functionUnderTest(tmp)
   if !bytes.Equal(tmp, b) {
        t.Fatalf("input modified %v != %v", tmp, b)
   }
})

This should be fixed now. LMK if you run into this again.