-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Syzkaller targets are broken with native go114-fuzz-build #3639
Comments
* Switch OSS projects to use native go-fuzz. * Fix go-json-iterator breakage, put source in package search dir. * Revert syzkaller change, track bug in #3639
/cc @dvyukov |
@mdempsky go-fuzz does not instrument os/exec. If the native instrumentation does, it probably shouldn't (at least the child part). |
Oops. Yeah, the compiler avoids instrumenting package runtime, but will instrument whatever else it's told to, including package syscall and the forkExec functions. Adding I know you mentioned os/exec, but I think that's fine to instrument if syscall is suppressed? |
Fuzz instrumentation isn't safe for the child process within syscall.ForkExec. Reported as google/oss-fuzz#3639.
You may find the full list that go-fuzz does not instrument in go-fuzz-build. I don't remember anything additional on top of what may be in the comments there... |
Thanks. It looks like go-fuzz-build doesn't explicitly avoid instrumenting syscall, but avoids it as a side effect of it being a dependency of github.com/dvyukov/go-fuzz/go-fuzz-dep, which the comments say it avoids because it would lead to import cycles: https://github.com/dvyukov/go-fuzz/blob/be3528f3a81351d8a438aed216130e1e7da39f7c/go-fuzz-build/main.go#L552 Since go114-fuzz-build doesn't need a Go support library (other than the Go runtime itself), the import cycle concern didn't apply, so I didn't try replicating that logic beyond avoiding instrumenting package runtime. So that explains why I didn't realize package syscall (and fork in particular) would be a sore point. |
@inferno-chromium how many executions did this need to crash? I wonder why bad build check didn't catch it. |
bad build check did catch it instantly, that is why i excluded this from original commit. |
The text was updated successfully, but these errors were encountered: