Skip to content
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

Jump to bottom

[dev.fuzz] proposal: default timeout should be disabled #44483

Closed
dsnet opened this issue Feb 21, 2021 · 8 comments
Closed

[dev.fuzz] proposal: default timeout should be disabled #44483

dsnet opened this issue Feb 21, 2021 · 8 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
Backlog

Comments

@dsnet
Copy link
Member

dsnet commented Feb 21, 2021

Running the following flags:

go test -fuzz=MyFunc

eventually crashes with:

*** Test killed with quit: ran too long (11m0s).
exit status 2
FAIL	path/to/my/module	660.015s

I would expect the use of the fuzz flag to implicitly disable the timeout flag (i.e., set it to 0). The fact that it crashes after around 10min matches the default value for timeout, which is 10min. I'm not sure where the additional 60s comes from.
@dsnet
Copy link
Member Author

dsnet commented Feb 22, 2021

The current proposal mentions a possible fuzztime flag. I wonder if it is better to overload the existing timeout flag instead.

\cc @jayconrod @katiehockman

@dsnet dsnet added the fuzz Issues related to native fuzzing support label Feb 22, 2021
@katiehockman
Copy link
Contributor

fuzztime and timeout serve different purposes, so I don't think we should use them interchangably. We should handle the default interaction with timeout better while fuzzing though, so good call on that.

The description for timeout is "If a test binary runs longer than duration d, panic." If someone sets timeout while fuzzing, we don't want to panic when we hit that timeout (which we will if there is no crash). We could overload timeout to mean something different when fuzzing, but I worry that might be confusing. Instead, fuzztime determines how long the fuzzing engine will run for a given target when fuzzing. When it hits that deadline, it won't panic, but will simply stop gracefully with a OK status.

What I'm not sure about is what to do if someone does something like this:
go test -fuzz=FuzzFoo -run=FuzzFoo -timeout=1m -fuzztime=5m

Perhaps timeout only applies to the seed corpus, and if it exceeds that time when running the seed corpus it would crash, otherwise it will fuzz for 5 minutes. We should think on that a bit more.

@ulikunitz
Copy link
Contributor

I suggest that timeout should be also applied to fuzzing but use the default 0 instead 10 minutes. If a timeout value is provided it should be used for testing and fuzzing. This way we can avoid -fuzztime and all confusion resulting from it.

To be honest users will not care for tests on the seed corpus and timeouts on it. For me it was always stuff in a directory. I started often with one simple seed. What was important to me where the crashers, because I needed them to create the test cases for the required fixes.

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 23, 2021
@cagedmantis cagedmantis added this to the Backlog milestone Feb 23, 2021
@AlekSi
Copy link
Contributor

AlekSi commented Apr 22, 2021

I think there needs to be one other timeout: timeout for one fuzz function invocation. This will allow one to find not only crashers, but hangs due to endless loops, large memory consumption, blocking on reading, etc. https://github.com/dvyukov/go-fuzz has this flag:

  -timeout int
    	test timeout, in seconds (default 10)

I think we can redefine what -timeout means when -fuzz is passed.

@AlekSi
Copy link
Contributor

AlekSi commented Apr 22, 2021

To be honest users will not care for tests on the seed corpus and timeouts on it. For me it was always stuff in a directory.

As a user, I respectfully disagree. I used unit tests for filling seed corpus even before dev.fuzz made it much easier. I do care about testing seed corpus and timeouts on it.

@gopherbot
Copy link

Change https://golang.org/cl/313072 mentions this issue: [dev.fuzz] cmd/go/internal/test: don't set default timeout when fuzzing

@katiehockman
Copy link
Contributor

This issue is focused on two main points: 1) the default timeout should be disabled when fuzzing and 2) we should consider a way to specify how long an execution of a fuzz function can run before it should be considered a "crash".

https://golang.org/cl/313072 fixes (1), and I'll track (2) in my list of feature requests/bugs to be filed all at once in a few weeks.

@AlekSi AlekSi mentioned this issue Apr 26, 2021
Closed
gopherbot pushed a commit that referenced this issue May 4, 2021
@katiehockman
[dev.fuzz] cmd/go/internal/test: don't set default timeout when fuzzing
fe8c0e9 
The -timeout flag is not used when the fuzzing engine
is running, but there was another backup alarm that would
stop the test binary after 11 minutes by default. This
change disables that backup alarm when the -fuzz flag is
set.

Note: unfortunately this means that if someone is running
`go test -fuzz` and a test hangs before the fuzzing engine
starts running, then the backup alarm won't trigger and
the test will run ~forever. I don't think there's a way
around this though, since the backup alarm has no way of
knowing what stage of the test execution we're in (ie.
are we running the unit tests, the seed corpus, or is
it fuzzing).

Fixes #44483

Change-Id: I4e212708a739c9cfc2e138440e27f257bb408c7f
Reviewed-on: https://go-review.googlesource.com/c/go/+/313072
Trust: Katie Hockman <katie@golang.org>
Run-TryBot: Katie Hockman <katie@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@katiehockman katiehockman mentioned this issue May 18, 2021
Closed
@katiehockman
Copy link
Contributor

I have filed #46220, so will go ahead and close this one as fixed.

@golang golang locked and limited conversation to collaborators May 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
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
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@AlekSi @cagedmantis @dsnet @ulikunitz @gopherbot @katiehockman