Skip to content

[dev.fuzz] testing: f.Cleanup called before fuzzing is done. #46632

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
klauspost opened this issue Jun 7, 2021 · 3 comments
Closed

[dev.fuzz] testing: f.Cleanup called before fuzzing is done. #46632

klauspost opened this issue Jun 7, 2021 · 3 comments
Labels
FrozenDueToAge fuzz Issues related to native fuzzing support NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@klauspost
Copy link
Contributor

klauspost commented Jun 7, 2021

What version of Go are you using (go version)?

λ gotip version
go version devel go1.17-5542c10fbf Fri Jun 4 03:07:33 2021 +0000 windows/amd64

What operating system and processor architecture are you using (go env)?

go env Output
λ gotip env                                                                                                                                 
set GO111MODULE=                                                                                                                            
set GOARCH=amd64                                                                                                                            
set GOBIN=                                                                                                                                  
set GOCACHE=d:\temp\gocache                                                                                                                 
set GOENV=C:\Users\klaus\AppData\Roaming\go\env                                                                                             
set GOEXE=.exe                                                                                                                              
set GOFLAGS=                                                                                                                                
set GOHOSTARCH=amd64                                                                                                                        
set GOHOSTOS=windows                                                                                                                        
set GOINSECURE=                                                                                                                             
set GOMODCACHE=e:\gopath\pkg\mod                                                                                                            
set GONOPROXY=                                                                                                                              
set GONOSUMDB=                                                                                                                              
set GOOS=windows                                                                                                                            
set GOPATH=e:\gopath                                                                                                                        
set GOPRIVATE=                                                                                                                              
set GOPROXY=https://goproxy.io                                                                                                              
set GOROOT=C:\Users\klaus\sdk\gotip                                                                                                         
set GOSUMDB=sum.golang.org                                                                                                                  
set GOTMPDIR=                                                                                                                               
set GOTOOLDIR=C:\Users\klaus\sdk\gotip\pkg\tool\windows_amd64                                                                               
set GOVCS=                                                                                                                                  
set GOVERSION=devel go1.17-5542c10fbf Fri Jun 4 03:07:33 2021 +0000                                                                         
set GCCGO=gccgo                                                                                                                             
set AR=ar                                                                                                                                   
set CC=gcc                                                                                                                                  
set CXX=g++                                                                                                                                 
set CGO_ENABLED=1                                                                                                                           
set GOMOD=NUL                                                                                                                               
set CGO_CFLAGS=-g -O2                                                                                                                       
set CGO_CPPFLAGS=                                                                                                                           
set CGO_CXXFLAGS=-g -O2                                                                                                                     
set CGO_FFLAGS=-g -O2                                                                                                                       
set CGO_LDFLAGS=-g -O2                                                                                                                      
set PKG_CONFIG=pkg-config                                                                                                                   
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=d:\temp\wintemp\go-build102784969=/tmp/go-build -gno-record-gcc-switches

What did you do?

Run fuzzer:

func FuzzCleanup(f *testing.F) {
	var mu sync.Mutex
	var cleaned bool
	f.Cleanup(func() {
		mu.Lock()
		cleaned = true
		mu.Unlock()
	})
	f.Add([]byte("abc"))
	f.Fuzz(func(t *testing.T, data []byte) {
		time.Sleep(time.Second)
		mu.Lock()
		defer mu.Unlock()
		if cleaned {
			panic("Cleanup called before fuzzing finished.")
		}
	})
}

What did you expect to see?

f.Cleanup() is called when all fuzzers have finished, giving no output.

I think this is a reasonable expectation. Otherwise the usefulness is pretty reduced of f.Cleanup, or at least annoying to use, having to add a sync.Waitgroup.

What did you see instead?

Output: https://gist.github.com/klauspost/fcbfa313d9dfab5b927fb7b80ad75775

@katiehockman katiehockman changed the title [dev.fuzz] f.Cleanup called before fuzzing is done. [dev.fuzz] testing: f.Cleanup called before fuzzing is done. Jun 7, 2021
@katiehockman katiehockman added fuzz Issues related to native fuzzing support NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 7, 2021
@katiehockman katiehockman added this to the Unreleased milestone Jun 7, 2021
@katiehockman
Copy link
Contributor

/cc @jayconrod

@jayconrod
Copy link
Contributor

This looks like a bug. I couldn't reproduce it at all on macOS, but I think I know why it's happening.

There are a couple places in internal/fuzz where we read from pipes. We don't want those reads to block after the context is cancelled (which happens after a timeout or after the user presses ^C). So the actual read is done in a goroutine, and the parent function returns when ctx.Done() is closed or the goroutine sends to a chan, indicating it's finished.

In one of these places, we call the fuzz function after the read completes successfully. If the context is cancelled while the fuzz function is running, we might start cleaning up while the fuzz function continues to run.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/329920 mentions this issue: [dev.fuzz] internal/fuzz: fix race in worker RPC logic

gopherbot pushed a commit that referenced this issue Jun 23, 2021
We want worker RPCs to return as soon as the context is cancelled,
which happens if the user presses ^C, we hit the time limit, or
another worker discovers a crasher. RPCs typically block when reading
pipes: the server waits for call arguments from the client, and the
client waits for results from the server.

Since io.Reader.Read doesn't accept a context.Context and reads on
pipe file descriptors are difficult to reliably unblock, we've done
this by calling Read in a goroutine, and returning from the parent
function when ctx.Done() is closed, even if the underlying goroutine
isn't finished.

In workerServer.serve, we also called the fuzz function in the same
goroutine. This resulted in a bug: serve could return while the fuzz
function was still running. The fuzz function could observe side
effects from cleanup functions registered with F.Cleanup.

This change refactors read cancellation logic into contextReader. Only
the underlying Read is done in a goroutine. workerServe.serve won't
return while the fuzz function is running.

Fixes #46632

Change-Id: Id1ed31f6521155c7c8e76dd52a2d70aa93cab201
Reviewed-on: https://go-review.googlesource.com/c/go/+/329920
Trust: Jay Conrod <[email protected]>
Trust: Katie Hockman <[email protected]>
Run-TryBot: Jay Conrod <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Katie Hockman <[email protected]>
@golang golang locked and limited conversation to collaborators Jun 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge fuzz Issues related to native fuzzing support NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Status: No status
Development

No branches or pull requests

4 participants