pkg/fuzzer/job.go: triageJob.minimize not starting with cloned program from triageJob.handleCall

[BitsByWill]

Describe the bug
I originally sent this on the mailing list but it didn't seem to make it through moderation.

I'm experimenting with augmenting Syzkaller signal/coverage data with additional feedback metrics based upon static analysis and noticed a quirk about triageJob.minimize and triageJob.handleCall. At

syzkaller/pkg/fuzzer/job.go

Line 169 in 875573a

p := job.p.Clone()

from triageJob.run, the program for handleCall is cloned from job.p, yet minimize still uses the original job.p here

syzkaller/pkg/fuzzer/job.go

Line 354 in 875573a

p, call := prog.Minimize(job.p, call, mode, func(p1 *prog.Prog, call1 int) bool {

.

Shouldn't this use the newly cloned program p? Otherwise, if no minimizations occur from prog/minimization.go, minimize returns the original job.p.

I don't believe this causes any issues currently, but this logic doesn't seem to match the intent of the Clone.

To Reproduce

I noticed this because my experiments added some additional post-processing data to Prog as I assumed uniqueness at this point, and a race condition popped up between two handleCall goroutines that processed programs without successful minimizations (hence the same job.p)

Expected behavior
The intent of clone should be matched and minimize should use the cloned program rather than the Prog pointer from the job struct.

Additional context
Here is a sample fix:

diff --git a/pkg/fuzzer/job.go b/pkg/fuzzer/job.go
index 7b06bc97f..e0c2e9772 100644
--- a/pkg/fuzzer/job.go
+++ b/pkg/fuzzer/job.go
@@ -168,7 +168,7 @@ func (job *triageJob) handleCall(call int, info *triageCall) {

        p := job.p.Clone()
        if job.flags&ProgMinimized == 0 {
-               p, call = job.minimize(call, info)
+               p, call = job.minimize(p, call, info)
                if p == nil {
                        return
                }
@@ -340,7 +340,7 @@ func (job *triageJob) stopDeflake(run, needRuns int, noNewSignal bool) bool {
        return false
 }

-func (job *triageJob) minimize(call int, info *triageCall) (*prog.Prog, int) {
+func (job *triageJob) minimize(p *prog.Prog, call int, info *triageCall) (*prog.Prog, int) {
        job.info.Logf("[call #%d] minimize started", call)
        minimizeAttempts := 3
        if job.fuzzer.Config.Snapshot {
@@ -351,7 +351,7 @@ func (job *triageJob) minimize(call int, info *triageCall) (*prog.Prog, int) {
        if job.fuzzer.Config.PatchTest {
                mode = prog.MinimizeCallsOnly
        }
-       p, call := prog.Minimize(job.p, call, mode, func(p1 *prog.Prog, call1 int) bool {
+       p, call = prog.Minimize(p, call, mode, func(p1 *prog.Prog, call1 int) bool {
                if stop {
                        return false
                }

[a-nogikh]

Sorry, I think I have accidentally pressed the wrong button during the moderation :(

Regarding the issue itself.

In the existing code, the behavior should not cause problems because the original job.p value is already cloned:

syzkaller/pkg/fuzzer/fuzzer.go

Line 153 in d399943

p: req.Prog.Clone(),

And then we Clone() a unique program copy for each job spawned in triageJob.handleCall(). It's also fine to use job.p in minimize() because prog.Minimize() does not change the source program and, for each call relative to which we do the minimization, we anyway start from the same original program.

It will probably be less confusing if we don't do cloning here at all:

syzkaller/pkg/fuzzer/job.go

Line 169 in d399943

p := job.p.Clone()

and just do p := job.p instead.

---

It's not really clear how exactly it broke in your case. Into what method did you add your post-processing code?

[BitsByWill]

I added some post-processing code on coverage data after job.minimize and after this if block:

syzkaller/pkg/fuzzer/job.go

Line 177 in d399943

if !job.fuzzer.Config.NewInputFilter(callName) {

The Clone call is good in processResult from pkg/fuzzer/fuzzer.go. The problem arose because I was augmenting a Prog struct with additional data after the aforementioned point in job.handleCall. Even though there is a Clone call there, minimize starts with the original job.p- if no minimizations occurred, this same job.p is returned. New Prog pointers are returned if successful minimizations occur.

If no minimizations occur, this means that p in job.handleCall is now job.p rather than the cloned p. Then when I add additional data to p, a race condition can occur if at least 2 of them have p set to job.p due to the return value of job.minimize (as all the handleCall calls run in goroutines for a single triageJob.

It will probably be less confusing if we don't do cloning here at all

This makes sense too. I just wasn't sure if the original intent was to run minimize on job.p or the cloned program.

[a-nogikh]

I see. Yes, in that case you'd better do a Clone() call yourself to make sure you modify your own copy.

I've sent #5893 that should make the code less confusing.

[BitsByWill]

Sounds good and thank you for the clarification!