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

Add golang 1.14+ cmd/compile's native libfuzzer instrumentation support. #3633

Merged
merged 4 commits into from
Apr 14, 2020

Conversation

inferno-chromium
Copy link
Collaborator

Slightly modified version of https://github.com/mdempsky/go114-fuzz-build
also uses template from gen_go_fuzz.go

@inferno-chromium
Copy link
Collaborator Author

@lukasz-milewski @mdempsky - can you help to review this. Should this go in an official go module ? This just modifies https://github.com/mdempsky/go114-fuzz-build slightly and uses fuzz template from internal gen_go_fuzz, any other changes you think we should do here before we migrate projects from @dvyukov's https://github.com/dvyukov/go-fuzz

@TravisBuddy
Copy link

Hey @inferno-chromium,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 13ff54f0-7d8e-11ea-9491-7112ce4ca887

@mdempsky
Copy link

Should this go in an official go module ?

By official Go module, do you mean like a module maintained by the Go project? Or do you mean like in a separate, standalone Go module?

I would think you can just use github.com/mdempsky/go114-fuzz-build as-is.

This just modifies https://github.com/mdempsky/go114-fuzz-build slightly and uses fuzz template from internal gen_go_fuzz, any other changes you think we should do here before we migrate projects from @dvyukov's https://github.com/dvyukov/go-fuzz

What was the motivation for these changes?

@inferno-chromium
Copy link
Collaborator Author

Should this go in an official go module ?

By official Go module, do you mean like a module maintained by the Go project? Or do you mean like in a separate, standalone Go module?

Yes an official go module maintained by Go project.

I would think you can just use github.com/mdempsky/go114-fuzz-build as-is.

Prefer to avoid user repo, prefer to use github.com/google, we will just put code in OSS-Fuzz repo.

This just modifies https://github.com/mdempsky/go114-fuzz-build slightly and uses fuzz template from internal gen_go_fuzz, any other changes you think we should do here before we migrate projects from @dvyukov's https://github.com/dvyukov/go-fuzz

What was the motivation for these changes?

Makes it easier to make minor changes, e.g. adds LLVMFuzzerInitialize, removes unneeded command line switches, makes output file mandatory.

@TravisBuddy
Copy link

Hey @inferno-chromium,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 076822d0-7db1-11ea-b89f-d9b5d34487e5

@mdempsky
Copy link

e.g. adds LLVMFuzzerInitialize,

Does adding this have any effect if the function doesn't do anything?

The //exported functions should use C.xxx types in their parameter / return types to guarantee C ABI compatibility.

@inferno-chromium
Copy link
Collaborator Author

e.g. adds LLVMFuzzerInitialize,

Does adding this have any effect if the function doesn't do anything?

Yes right, in future, we can add support for it. but no reason to keep it now. removed.

The //exported functions should use C.xxx types in their parameter / return types to guarantee C ABI compatibility.

Ok done, switched to your version.

@TravisBuddy
Copy link

Hey @inferno-chromium,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 1c8a4e00-7db5-11ea-b89f-d9b5d34487e5

Copy link
Collaborator

@oliverchang oliverchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but not a fan of branching/duplicating code. Can we just clone https://github.com/mdempsky/go114-fuzz-build directly? If these changes are needed, can we just contribute them to that repo instead?

@inferno-chromium
Copy link
Collaborator Author

LGTM, but not a fan of branching/duplicating code. Can we just clone https://github.com/mdempsky/go114-fuzz-build directly? If these changes are needed, can we just contribute them to that repo instead?

Sure, removed from our repo and will ask @lukasz-milewski to upstream in an official repo. Till then better to not duplicate stuff, so removed my implementation.

@inferno-chromium inferno-chromium merged commit 20f5e05 into master Apr 14, 2020
@TravisBuddy
Copy link

Hey @inferno-chromium,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: b01b1ef0-7ded-11ea-a704-7741d9774a61

@mdempsky
Copy link

For what it's worth, I'm the upstream maintainer of the Go compiler's libfuzzer support, and I intend to maintain mdempsky/go114-fuzz-build until there's official cmd/go integration. But I expect the latter will still be a while.

@inferno-chromium
Copy link
Collaborator Author

For what it's worth, I'm the upstream maintainer of the Go compiler's libfuzzer support, and I intend to maintain mdempsky/go114-fuzz-build until there's official cmd/go integration. But I expect the latter will still be a while.

Sure that sounds good, we will follow mdempsky/go114-fuzz-build then.

Copy link
Contributor

@Dor1s Dor1s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants