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

Golang coverage report test #3142

Merged
merged 4 commits into from
Nov 19, 2020
Merged

Golang coverage report test #3142

merged 4 commits into from
Nov 19, 2020

Conversation

catenacyber
Copy link
Contributor

This is a draft, not to be merged yet, following the discussion in #2817

The result now is that
python infra/helper.py build_fuzzers --sanitizer coverage gonids
builds a coverage report named build/out/gonids/covfuzzparserule.html

There is still a big list of points to address :

  • We need to have the corpus directory when running python infra/helper.py build_fuzzers. For now, the directory is hardcoded to $OUT/corpus/$fuzzer/ . Is this possible and the right thing to do ?
  • There is no need to run python infra/helper.py coverage gonids because this relies on go testing which needs the source code. I guess that there is somewhere where we should add a test like if we already have some html report, no need to launch this...
  • should ossfuzz_test.go be part of the docker image (and not related to this specific project) ? I think so
    (sorry if I do not have the right wording about Docker)
  • What about the naming of the file for the coverage report ? Is it ok ?
  • The modifications to build.sh are meant to be generic. Should this be left to be customized by individual projects ? Or should there be a helper script compile_golang with this compile_fuzzer function ?

I think that the alternative solution to have a coverage report while not using go test, is to modify go-fuzz-build, and that means a bigger work.
Since there is LLVMFuzzerInitialize but no such function for closing, the best shot would be to modify go-fuzz LLVMFuzzerTestOneInput https://github.com/dvyukov/go-fuzz/blob/master/go-fuzz-build/main.go#L876 so that coverage info gets streamed after processing each input, and I did not find out yet to access this information.
Then a later processing could transform this raw coverage profiles into some nice html report.

PS : There is the bug #3107 which seems to prevent gonids project to have a fresh generated corpus

@catenacyber
Copy link
Contributor Author

Happy new year and ping @Dor1s

@Dor1s
Copy link
Contributor

Dor1s commented Jan 6, 2020

@catenacyber we can land this, but our infra will not be running it in the meantime anyway. Please let us know if you want this landed

@catenacyber
Copy link
Contributor Author

Thanks, this is only a draft for the moment, not to be landed right away.
I would rather like to know how we can make your infra run this.

Can you answer the questions in my bullets list ? such as about the naming of the file for the coverage report ?
Or am I too far and missing something else ?

@Dor1s
Copy link
Contributor

Dor1s commented Jan 28, 2020

  • We need to have the corpus directory when running python infra/helper.py build_fuzzers. For now, the directory is hardcoded to $OUT/corpus/$fuzzer/ . Is this possible and the right thing to do ?
  • There is no need to run python infra/helper.py coverage gonids because this relies on go testing which needs the source code. I guess that there is somewhere where we should add a test like if we already have some html report, no need to launch this...

We would need to copy the Go sources into $OUT by extending this command:

COPY_SOURCES_CMD="cp -rL --parents $SRC $WORK /usr/include $OUT"

That would let us to have sources in the base-runner image, and then we would use coverage command to actually generate the report, instead of doing that in build_fuzzers.

  • should ossfuzz_test.go be part of the docker image (and not related to this specific project) ? I think so
    (sorry if I do not have the right wording about Docker)

Yes, it's fine to have it in the image.

  • What about the naming of the file for the coverage report ? Is it ok ?

What name do you mean?

  • The modifications to build.sh are meant to be generic. Should this be left to be customized by individual projects ? Or should there be a helper script compile_golang with this compile_fuzzer function ?

Yeah, we definitely have to generalize it somewhere. With #3297 being addressed, it should be fine to have something like compile_go that would take care of common Go specifics.

@catenacyber
Copy link
Contributor Author

catenacyber commented Feb 1, 2020

Thanks for your review, I will rework this PR...

What about the naming of the file for the coverage report ? Is it ok ?

What name do you mean?

I meant the name of the generated html file report :
https://github.com/google/oss-fuzz/pull/3142/files#diff-3116338d1d486a71c32a97ec981199deR32

@Dor1s
Copy link
Contributor

Dor1s commented Feb 3, 2020

The doesn't really matter, as we'll likely just add a dummy index.html that would redirect to the actual report, e.g. for C/C++ coverage we have:

$ curl https://storage.googleapis.com/oss-fuzz-coverage/libxml2/reports/20200203/linux/index.html

    <!DOCTYPE html>
    <html>
      <head>
        <!-- HTML meta refresh URL redirection -->
        <meta http-equiv="refresh" content="0; url=directory_view_index.html">
      </head>
    </html>

Just because the underlying coverage implementation might change, but the index.html file should always be present and should send the user to the right page

@catenacyber
Copy link
Contributor Author

catenacyber commented Feb 14, 2020

With this latest commit, we are getting close I think :

I modified base-runner and base-builder so, I needed to run

docker build -t gcr.io/oss-fuzz-base/base-builder infra/base-images/base-builder
docker build -t gcr.io/oss-fuzz-base/base-runner infra/base-images/base-runner

To get going.

After downloading the corpus to build/corpus/gonids/fuzz_parserule/
The documented commands work :

export PROJECT_NAME=gonids
python infra/helper.py build_fuzzers --sanitizer=coverage $PROJECT_NAME
python infra/helper.py coverage --no-corpus-download $PROJECT_NAME

ie, I get the html report named as build/out/gonids/report/index.html

What is left to be done ?

With #3297 being addressed, it should be fine to have something like compile_go that would take care of common Go specifics.

The compile_fuzzer function is only used by this gonids project, and should be used by all golang projects

The big remaining question is I am not sure how to deal with multiple fuzz targets...

What do you think @Dor1s ?

@@ -15,6 +15,7 @@
################################################################################

FROM gcr.io/oss-fuzz-base/base-clang
RUN apt-get update
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why I needed this but docker build -t gcr.io/oss-fuzz-base/base-builder infra/base-images/base-builder did not work without it

@TravisBuddy
Copy link

Hey @catenacyber,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 9bda4f50-4f13-11ea-b39d-337b6d42f374

@@ -63,3 +64,15 @@ ENV MSAN_OPTIONS="print_stats=1:strip_path_prefix=/workspace/:symbolize=1"
ENV UBSAN_OPTIONS="print_stacktrace=1:print_summary=1:silence_unsigned_overflow=1:strip_path_prefix=/workspace/:symbolize=1"
ENV FUZZER_ARGS="-rss_limit_mb=2560 -timeout=25"
ENV AFL_FUZZER_ARGS="-m none"

# Download and install the latest stable Go.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from base-builder

Maybe we can reuse the binaries already present in /out/root/go...

Copy link
Contributor

Choose a reason for hiding this comment

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

so we need golang to be installed in the base-runner image, as it's being used for generating a coverage report?

Copy link
Contributor

Choose a reason for hiding this comment

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

@oliverchang as we also install Go in the base-runner, would you be ok with us doing that once in the base-image instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Leaving it here is probably fine.

We plan to diverge images here and migrate runners to be based on ClusterFuzz images instead, so we'd have to add it back here anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

ack, thanks Oliver!

# Run each fuzz target, generate raw coverage dumps.
for fuzz_target in $FUZZ_TARGETS; do
# Test if fuzz target is a golang one.
grep "go test -v -coverprofile" $fuzz_target > /dev/null 2>&1 && export GO_TARGET="true"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can have a better way to know if we are dealing with a golang project...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we now have a language attribute in every project.yaml file, see

def _get_project_language(project_name):
and how it's used

export FUZZ_CORPUS_DIR="/corpus/${fuzz_target}/"
#export GOPATH="./root/go/"
sh $fuzz_target
go tool cover -html=fuzz.cov -o $REPORT_ROOT_DIR/cov"${fuzz_target,,}".html
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could keep the different coverage files by naming them ${fuzz_target}.cov

if [[ $GO_TARGET == "true" ]]; then
echo "End for go targets"
#TODO merge all fuzz targets reports cf https://github.com/wadey/gocovmerge
exit 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I get it right, the logic for C projects is to run the fuzz targets, and then merge coverage profiles.
We can do this for golang, it seems there is a project that does it with 111 lines

Or we can, from the start, have the command go test -v -coverprofile running all the fuzz targets

@Dor1s
Copy link
Contributor

Dor1s commented Feb 14, 2020

Thanks a lot for working on this! I'll take another look some time later

@catenacyber
Copy link
Contributor Author

I tested the profile merging from multiple targets for go-dns project and it works :-)

I used https://github.com/wadey/gocovmerge
Is it ok ? ie no license issues ?

By the way, it seems the corpus downloads are broken
https://oss-fuzz.com/fuzzer-stats?project=gonids&fuzzer=libFuzzer_gonids_fuzz_parserule&job=libfuzzer_asan_gonids&group_by=by-day
Does not show corpus for March 9 and 10 (it does for the previous days)
And the link gets me to the error "Authorization required"

@TravisBuddy
Copy link

Hey @catenacyber,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 6e79cb70-646d-11ea-b857-47bc0e3c428e

@Dor1s
Copy link
Contributor

Dor1s commented Mar 12, 2020

@catenacyber regarding corpus, I think corpus pruning task was somewhat broken last two days (//cc @mbarbella-chromium @mpherman2), that's why. It should recover today and the corpora links should be available again.

Regarding coverage -- really appreciate your contributions, but can't commit to a thorough review just yet.

@catenacyber
Copy link
Contributor Author

Hi @Dor1s
Friendly ping on this :-)
Do you know when you will have time to review it ?

@Dor1s
Copy link
Contributor

Dor1s commented Apr 12, 2020

@catenacyber thanks for another ping. This is not forgotten, but I can't give you any ETA, as I don't think it's worth investing into finalizing and maintaining this until we get more projects integrated (i.e. more potential users).

When Go coverage becomes more needed, we'll have to update code coverage job, ClusterFuzz side of things, etc -- this won't be done with a single PR :)

@catenacyber
Copy link
Contributor Author

By the way (not sure where to post this), any thoughts on improving golang standard library fuzzing cf golang/go#37499 ?
cc @inferno-chromium

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.

@catenacyber good news: we decided that we should get this change landed so that other OSS-Fuzz users could generate coverage reports for Go projects locally. We don't have a concrete plan for when we'll add the infrastructure support for generating coverage reports for non-C/C++ projects, but at least for the local use case it seems worthy to do now.

May I ask you please to rebase your change on the newest tip of tree and test if it's still working?

@@ -105,6 +106,7 @@ COPY compile compile_afl compile_dataflow compile_libfuzzer compile_honggfuzz \
precompile_honggfuzz srcmap write_labels.py /usr/local/bin/

COPY detect_repo.py /opt/cifuzz/
COPY ossfuzz_test.go /root/go/
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this scrip tin the base-builder image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this somewhere.
It is meant to be generic for all projects. (but it can be in each project)
So, it is related to compile_fuzzer being generic or not

What do you want for now ? specific by projet or generic ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Leave it like this for now, I need to think more about how we can unify the whole compile_fuzzer thing

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename it to ossfuzz_coverage_runner.go

@@ -63,3 +64,15 @@ ENV MSAN_OPTIONS="print_stats=1:strip_path_prefix=/workspace/:symbolize=1"
ENV UBSAN_OPTIONS="print_stacktrace=1:print_summary=1:silence_unsigned_overflow=1:strip_path_prefix=/workspace/:symbolize=1"
ENV FUZZER_ARGS="-rss_limit_mb=2560 -timeout=25"
ENV AFL_FUZZER_ARGS="-m none"

# Download and install the latest stable Go.
Copy link
Contributor

Choose a reason for hiding this comment

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

so we need golang to be installed in the base-runner image, as it's being used for generating a coverage report?

# Run each fuzz target, generate raw coverage dumps.
for fuzz_target in $FUZZ_TARGETS; do
# Test if fuzz target is a golang one.
grep "go test -v -coverprofile" $fuzz_target > /dev/null 2>&1 && export GO_TARGET="true"
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we now have a language attribute in every project.yaml file, see

def _get_project_language(project_name):
and how it's used

@@ -21,6 +21,22 @@ function compile_fuzzer {
function=$2
fuzzer=$3

if [[ $SANITIZER = *coverage* ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

something like this needs to be done for each Go project, right?

Sadly, we decided not to put compile_fuzzer function anywhere globally and instead copy-pasted it into every Go project's build_file. Now is a moment to reconsider that decision

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this needs to be done for each project.

Do you want this generalization be part of this PR ? (or rather in another one)

Copy link
Contributor

Choose a reason for hiding this comment

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

let's do it in another PR

@catenacyber
Copy link
Contributor Author

Great, thanks for this news

I will do the rebase and check work.

so we need golang to be installed in the base-runner image, as it's being used for generating a coverage report?

Exactly.

Yes, we now have a language attribute in every project.yaml file, see

I noticed but that was not the case when this work was started ;-)
I will use it in the rework

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.

I noticed but that was not the case when this work was started ;-)

Yep, that was added after you implemented this.

@@ -105,6 +106,7 @@ COPY compile compile_afl compile_dataflow compile_libfuzzer compile_honggfuzz \
precompile_honggfuzz srcmap write_labels.py /usr/local/bin/

COPY detect_repo.py /opt/cifuzz/
COPY ossfuzz_test.go /root/go/
Copy link
Contributor

Choose a reason for hiding this comment

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

Leave it like this for now, I need to think more about how we can unify the whole compile_fuzzer thing

@catenacyber
Copy link
Contributor Author

so we need golang to be installed in the base-runner image, as it's being used for generating a coverage report?

Not doing it right now, but you may want to do a gocov-runner image which will be the base-runner + golang
I think that kind of the same is done for msan-builder, right ?

@catenacyber
Copy link
Contributor Author

It should be good now.
Any thoughts about the gocovmerge path ?

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.

Awesome stuff! I've tested locally and it works!

@@ -105,6 +106,7 @@ COPY compile compile_afl compile_dataflow compile_libfuzzer compile_honggfuzz \
precompile_honggfuzz srcmap write_labels.py /usr/local/bin/

COPY detect_repo.py /opt/cifuzz/
COPY ossfuzz_test.go /root/go/
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename it to ossfuzz_coverage_runner.go

@@ -63,3 +64,15 @@ ENV MSAN_OPTIONS="print_stats=1:strip_path_prefix=/workspace/:symbolize=1"
ENV UBSAN_OPTIONS="print_stacktrace=1:print_summary=1:silence_unsigned_overflow=1:strip_path_prefix=/workspace/:symbolize=1"
ENV FUZZER_ARGS="-rss_limit_mb=2560 -timeout=25"
ENV AFL_FUZZER_ARGS="-m none"

# Download and install the latest stable Go.
Copy link
Contributor

Choose a reason for hiding this comment

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

@oliverchang as we also install Go in the base-runner, would you be ok with us doing that once in the base-image instead?

@@ -111,8 +111,22 @@ function run_fuzz_target {
fi
}

export GOPATH=/out/root/go
Copy link
Contributor

Choose a reason for hiding this comment

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

if we decide to install Go in the base-image (the comment above), we should set the GOPATH there too. Then, here we will do GOPATH=$OUT/$GOPATH to keep things less fragile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Waiting for the decision about where to put go, otherwise ok

Copy link
Contributor

@Dor1s Dor1s Nov 6, 2020

Choose a reason for hiding this comment

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

Let me ping @oliverchang once again, as GitHub reviews are terrible. Oliver, we need to have Go installed in both the builder and the runner images. Would you be concerned if we installed Go only once in the base-image instead of base-builder and base-runner?

cd $GOPATH/src
echo "Running go target $fuzz_target"
export FUZZ_CORPUS_DIR="/corpus/${fuzz_target}/"
bash $OUT/$fuzz_target $DUMPS_DIR/$fuzz_target.profdata
Copy link
Contributor

Choose a reason for hiding this comment

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

The existing code runs fuzz targets in parallel. Can we run Go targets in parallel too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like it works :-)
Refactored the code to share the multiprocess handling

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.

A few more comments. Should be good to land after that! I'll outline the remaining work in #2817

@@ -0,0 +1,34 @@
package mypackagebeingfuzzed
Copy link
Contributor

Choose a reason for hiding this comment

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

we would need a Google copyright here:

// Copyright 2020 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//      http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@@ -146,6 +146,7 @@ COPY compile compile_afl compile_dataflow compile_libfuzzer compile_honggfuzz \
precompile_honggfuzz srcmap write_labels.py /usr/local/bin/

COPY detect_repo.py /opt/cifuzz/
COPY ossfuzz_coverage_runner.go /root/go/
Copy link
Contributor

Choose a reason for hiding this comment

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

let's move this file to the base-runner image, as we don't really need it in the builder

Copy link
Contributor

Choose a reason for hiding this comment

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

all other coverage scripts are also in the runner image

Copy link
Contributor

Choose a reason for hiding this comment

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

and instead of hardcoded /root/go/ let's use $GOPATH which is set above on line 65. As you move this to the base-runner image, you'll initialize $GOPATH there too and then please use it instead of the hardcoded path

Copy link
Contributor

Choose a reason for hiding this comment

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

to install this script in the base-runner image, add COPY ossfuzz_coverage_runner.go /usr/local/src on line 58 in base-runner/Dockerfile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's move this file to the base-runner image, as we don't really need it in the builder

We do need this file during the build process.
It is copied, then modified in every call to compile_fuzzer, kind of producing the coverage fuzz target

and instead of hardcoded /root/go/ let's use $GOPATH

ok for this

Copy link
Contributor

Choose a reason for hiding this comment

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

We do need this file during the build process.

Makes sense, thanks! Ok to leave here then

@@ -92,7 +92,7 @@ BUILD_CMD="bash -eux $SRC/build.sh"

# We need to preserve source code files for generating a code coverage report.
# We need exact files that were compiled, so copy both $SRC and $WORK dirs.
COPY_SOURCES_CMD="cp -rL --parents $SRC $WORK /usr/include /usr/local/include $OUT"
COPY_SOURCES_CMD="cp -rL --parents $SRC $WORK /usr/include /usr/local/include $GOPATH $OUT"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the whole $GOPATH to be copied over to $OUT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it can be just $GOPATH/src and $GOPATH/pkg but I am not sure...

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, let's leave it this way

@@ -24,6 +24,7 @@ COPY --from=base-clang /usr/local/bin/llvm-cov /usr/local/bin/
COPY --from=base-clang /usr/local/bin/llvm-profdata /usr/local/bin/
COPY --from=base-clang /usr/local/bin/llvm-symbolizer /usr/local/bin/

RUN apt-get update
Copy link
Contributor

Choose a reason for hiding this comment

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

base-image does this command already. Are you sure we need it here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think so, this must have been a debug leftover

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for verifying that it's needed!

@@ -111,21 +111,35 @@ function run_fuzz_target {
fi
}

export GOPATH=$OUT/root/go
Copy link
Contributor

Choose a reason for hiding this comment

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

as we install Go in base-runner itself, I don't think we need to re-define GOPATH here. Am I wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

if we need to, let's do GOPATH=$OUT/$GOPATH to avoid hardcoding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as we install Go in base-runner itself, I don't think we need to re-define GOPATH here. Am I wrong?

I am afraid you are.
There are indeed 2 GOPATHs in base-runner :

  • the system one (/root/go), which is used by gocovmerge
  • the fuzz targets one (/root/go/root/go), which is used by running the targets

Do you see a better way to do this ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it important to use /root/go/root/go for running fuzz targets?

cp $REPORT_ROOT_DIR/index.html $REPORT_PLATFORM_DIR/index.html
echo "End for go targets"
else

Copy link
Contributor

Choose a reason for hiding this comment

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

please add one indentation level for all the commands from here down to line 190

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I had not done this at first for easier review ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

smart!

@@ -21,6 +21,22 @@ function compile_fuzzer {
function=$2
fuzzer=$3

if [[ $SANITIZER = *coverage* ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

let's do it in another PR

@catenacyber
Copy link
Contributor Author

@Dor1s I pushed some changes.
There is still the question of base-builder GOPATH copy and use in base-runner...
I left the projects changes in one commit until the question is resolved

cp $REPORT_ROOT_DIR/index.html $REPORT_PLATFORM_DIR/index.html
echo "End for go targets"
else

Copy link
Contributor

Choose a reason for hiding this comment

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

smart!

/root/go/bin/gocovmerge $DUMPS_DIR/*.profdata > fuzz.cov
go tool cover -html=fuzz.cov -o $REPORT_ROOT_DIR/index.html
cp $REPORT_ROOT_DIR/index.html $REPORT_PLATFORM_DIR/index.html
echo "End for go targets"
Copy link
Contributor

Choose a reason for hiding this comment

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

change the log message to "Finished generating code coverage report for Go fuzz targets."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@@ -111,21 +111,35 @@ function run_fuzz_target {
fi
}

export GOPATH=$OUT/root/go
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it important to use /root/go/root/go for running fuzz targets?

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.

One more nit and one more question left!

@catenacyber
Copy link
Contributor Author

Is it important to use /root/go/root/go for running fuzz targets?

Answering here as Github seems to be buggy...

It is /out/root/go in facts cf export GOPATH=$OUT/root/go

It is important to have both GOPATHS : this one for the targets, and the system one for gocovmerge and such

@catenacyber
Copy link
Contributor Author

One more nit and one more question left!

Done

Should I now remove the commit "Enables golang coverage report for gonids and go-dns" ?
What about the commit "Generates summary for golang coverage reports" ?
And what about the commit "Performance profile for golang projects" ?

Do you want them in separated/later PRs ?

@catenacyber
Copy link
Contributor Author

Retested on a new machine and it seems we need apt-get update in infra/base-images/base-runner/Dockerfile

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.

Looks great! Testing locally now before merging.

@@ -24,6 +24,7 @@ COPY --from=base-clang /usr/local/bin/llvm-cov /usr/local/bin/
COPY --from=base-clang /usr/local/bin/llvm-profdata /usr/local/bin/
COPY --from=base-clang /usr/local/bin/llvm-symbolizer /usr/local/bin/

RUN apt-get update
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for verifying that it's needed!

@Dor1s Dor1s merged commit 07ea81b into google:master Nov 19, 2020
@AdamKorcz
Copy link
Collaborator

This is great! I can't wait to use it.

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