-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Coverage as in https://github.com/google/oss-fuzz/issues/26 #35
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this.
@@ -202,6 +202,12 @@ $ python scripts/helper.py run_fuzzer $LIB_NAME name_of_a_fuzzer | |||
If everything works locally, then it should also work on our automated builders | |||
and ClusterFuzz. | |||
|
|||
It's recommended look at coverage as a sanity check to make sure that fuzzer gets to the code you expect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recommended to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
'-w', '/cov', | ||
'-e', 'ASAN_OPTIONS=coverage=1,detect_leaks=0', | ||
'-t', 'ossfuzz/libfuzzer-runner', | ||
'/out/%s/%s' % (args.library_name, args.fuzzer_name), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised by this additional directory. AFAIK our build scripts do not create second level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what do you mean.
pipe = subprocess.Popen(command) | ||
pipe.communicate() | ||
|
||
checkout_dir = os.path.join(BUILD_DIR, args.library_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't have to run it in docker. You can run all of it on host => no need to install in the image python.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it's insecure to run random python script downloaded over http
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm...
Can you try running it within standard image? For example, one of these: https://hub.docker.com/_/python/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not, let's create a special image for server. I am not keen on pulling new packages just for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
It's recommended to look at coverage as a sanity check to make sure that fuzzer gets to the code you expect. | ||
|
||
```bash | ||
$ sudo python scripts/helper.py run_with_coverage $LIB_NAME name_of_a_fuzzer seconds_to_run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace run_with_coverage with just coverage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
9e2e28b
to
f66ee54
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
almost there. 2 small changes please.
@@ -19,4 +19,4 @@ docker build --pull -t ossfuzz/base $@ infra/base-images/base | |||
docker build -t ossfuzz/base-clang $@ infra/base-images/base-clang | |||
docker build -t ossfuzz/base-libfuzzer $@ infra/base-images/base-libfuzzer | |||
docker build -t ossfuzz/libfuzzer-runner $@ infra/base-images/libfuzzer-runner | |||
|
|||
docker build -t ossfuzz/coverage $@ infra/base-images/coverage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will also need to change Jenkinsfile in this folder & Jenkinsfile in ../push-images/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
parser = argparse.ArgumentParser('helper.py coverage') | ||
parser.add_argument('library_name', help='name of the library') | ||
parser.add_argument('fuzzer_name', help='name of the fuzzer') | ||
parser.add_argument('run_time', help='time in seconds to run fuzzer') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add default (60s?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
No description provided.