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

common,crypto: move fuzzers out of core #22029

Merged
merged 4 commits into from
Dec 23, 2020
Merged

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Dec 16, 2020

This is one step towards getting coverage reports, a'la google/oss-fuzz#4847 .
A couple of our fuzzers reside within the go-ethereum core codebase. This is fine, except that in order to not include them in regular builds, they've been tagged with // +build gofuzz, to only be included in the fuzzed images.

However, the coverage reports are based on runinng regular go test ... on a testcase which simply feeds the corpus through the Fuzz function. And the Fuzz function is not visible in that case, since the build-tag is not active when not running via go-fuzz.

So a better approach is to move the fuzzers into tests/, and remove the build tag.
We still have some other cases where it's more difficult to move them, since the fuzzing relies on un-exported fields (blake2b), but this is a start.

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman added this to the 1.10.0 milestone Dec 16, 2020
@holiman
Copy link
Contributor Author

holiman commented Dec 17, 2020

This PR also needs google/oss-fuzz#4858 to enable code coverage of fuzzed repos. It does quite a few things.

  1. Move all (except one) fuzzer out of core, in order to get rid of build-flags that prevent running them as regular Tests for coverage
  2. Add a customized version of the compile_go_fuzzer script: since we have the fuzzers in tests/fuzzers, we need to explicitly set to coverpkg to be github.com/ethereum/go-ethereum, otherwise we only get coverage reports on the fuzzers themselves.
  3. Also, the oss-fuzz PR moves the entire go-ethereum installation to the GOPATH, which is also needed for the coverage profile to work. When it uses the coverprofile to locate the sources, it only knows of the GOPATH and the currrent directory, and resolving github.com/ethereum/go-ethereum from /out/src/. In order to find it, it would have to figure out that github.com/ethereum/go-ethereum is actually located in in $OUT/src/go-ethereum. It doesn't, and it panics. So putting everything into the GOPATH seems like the least painful approach.

cc @catenacyber

@catenacyber
Copy link

compile_go_fuzzer accepts build tags as its optional fourth argument

@holiman
Copy link
Contributor Author

holiman commented Dec 17, 2020

compile_go_fuzzer accepts build tags as its optional fourth argument

I'm not sure how that would help.. ? It prepends the input tag with --tag , so I don't think it makes sense to try and sneak the coverpkg in that way, although it might be possible..?

Ah, I guess you mean as a way to help it find the sources, if not in $GOPATH but in $OUT/src ? Yeah, well, we might aswell have everythiing in the gopath, IMO.

@catenacyber
Copy link

go test -tags gofuzz works in other projects

So, you do not have to move the fuzzers into another directory, just add explicitly the tag as an argument to compile_go_fuzzer

@holiman
Copy link
Contributor Author

holiman commented Dec 17, 2020

So, you do not have to move the fuzzers into another directory, just add explicitly the tag as an argument to compile_go_fuzzer

The part about moving the fuzzers is not because of that, it's because If we want them inside the codebase, they should be tagged to not be built "normally". But then go test cannot execute them. So it's nicer to have them in a dedicated tests/fuzzers folder, and omit the built-tags.

And that had some other repercussions, such as having to set the coverpkg

EDIT: Ah, now I get it. Oh, cool! However, we still already have most of them in a separate folder, so it kind of makes sense to have them all there. Plus some fuzzers, e.g. vm/runtime also reach into their parent, vm, so we want to have that in the coverpkg aswell

@catenacyber
Copy link

Thanks @holiman

It seems that we can improve the line in https:/google/oss-fuzz/blob/master/infra/base-images/base-builder/compile_go_fuzzer#L28

fuzzed_package=`pwd | rev | cut -d'/' -f 1 | rev

Because you have the directory named bls12381 even if you have the package named bls

Maybe go list -f '{{.Name}}' looks better

@catenacyber
Copy link

Otherwise, catenacyber/oss-fuzz@fcfceb9 looks fine

Tested with

python infra/helper.py build_fuzzers --sanitizer=coverage go-ethereum
python infra/helper.py coverage --fuzz-target=fuzzBitutilCompress --corpus-dir=/path/to/corpus/ go-ethereum

It works : go test can execute the Fuzz function even with tag gofuzz

@holiman
Copy link
Contributor Author

holiman commented Dec 21, 2020

@catenacyber I'm wondering about two things, first of all -- does that version report coverage for all of go-ethereum, or only the specific package being fuzzed?
And secondly, does it also require a change in the oss-fuzz scripts?

@holiman
Copy link
Contributor Author

holiman commented Dec 23, 2020

Maybe go list -f '{{.Name}}' looks better

I've finally gotten around to checking how that works and what it does :)
In general, I don't think we've been consistent about naming the fuzzing-package the same as the fuzzed package, so not sure if that matters.

I think, in general, we're better off just using coverpkg=github.com/ethereum/go-ethereum/... . That makes it easy and scalable, and we don't have to figure out rules for exactly what is covered by what fuzzer.

@holiman holiman merged commit b9012a0 into ethereum:master Dec 23, 2020
@catenacyber
Copy link

does that version report coverage for all of go-ethereum, or only the specific package being fuzzed?

Only the package being fuzzed.
I think your use of coverpkg is relevant.

does it also require a change in the oss-fuzz scripts?

Yes it required a change in oss-fuzz cf catenacyber/oss-fuzz@fcfceb9

@holiman are you good with your changes ? Or do you need something more ?

@holiman
Copy link
Contributor Author

holiman commented Dec 29, 2020

We have coverage reporting now, it seems to work fine, so I'm good.

If at some point the upstream scripts are changed so coverpkg can be sent along with it, we might switch over to use that instead, but it's not a prio

@catenacyber
Copy link

Ok, I will let you know if coverpkg comes into play

gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Sep 18, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Sep 20, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Sep 21, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Sep 21, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Sep 21, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Sep 22, 2024
gzliudan added a commit to XinFinOrg/XDPoSChain that referenced this pull request Sep 22, 2024
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.

3 participants