-
Notifications
You must be signed in to change notification settings - Fork 275
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
Integrate fuzzer #427
base: master
Are you sure you want to change the base?
Integrate fuzzer #427
Conversation
These are the matching changes on the oss-fuzz side; https://github.com/google/oss-fuzz/compare/master...tdsmith:libgd-integration?expand=1 |
i'd prefer fuzzers/ just to match examples/ and tests/ is this corpus something you built up or just fetched from somewhere else ? if it's an existing project that maintains inputs, fetching & unpacking an archive is fine. for pending commits, just squash them together rather than add files and then modify/shuffle things in follow up changes should add a fuzzers/README.md to document things |
1d70605
to
16a84dd
Compare
Add fuzzing targets for oss-fuzz.
16a84dd
to
51a6888
Compare
Sorry for the delay! I made these changes. |
Hi @vapier; is this a good place to merge, or would you like to see some additional progress? Compared to what's running now, this PR adds some additional corpora and mutes stderr, which should already help improve oss-fuzz's efficiency a little. |
It would be great to have that in the github actions, thoughts? |
I think we should merge, and add missing ones (all codecs are keys). I see that libgd is already on ossfuzz, not sure what that means. I need to sit down reading their docs, how we can have that as part of the CI etc. :) |
Google's [oss-fuzz](https://github.com/google/oss-fuzz) initiative. | ||
|
||
Test changes to these files by running, from the root of a local oss-fuzz | ||
checkout, |
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.
change the ,
to :
@@ -0,0 +1,39 @@ | |||
#!/bin/bash -eu | |||
|
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.
there should be a comment block at the top here explaining a bit more about this file and its runtime. it seems to make assumptions about certain vars being set ahead of time and it's not clear at all what all those are.
|
||
# Limit the size of buffer allocations to avoid bogus OOM issues | ||
# https://github.com/libgd/libgd/issues/422 | ||
sed -i'' -e 's/INT_MAX/100000/' "$SRC/libgd/src/gd_security.c" |
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.
isn't the source dir already guaranteed to be the cwd ? afterall, the script just ran ./bootstrap.sh
above.
./configure --prefix="$WORK" --disable-shared | ||
make -j$(nproc) install | ||
|
||
cd $SRC/libgd/fuzzers |
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 should run this script through shellcheck
and fix the issues it finds
cd $SRC/libgd/fuzzers | ||
|
||
# Assemble a corpus | ||
curl -Lo afl_testcases.tgz http://lcamtuf.coredump.cx/afl/demo/afl_testcases.tgz |
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.
is this URL guaranteed to be stable ?
shouldn't this be https:// ?
# Add test cases to the fuzzing corpus | ||
mkdir -p "corpus/$lowercase" | ||
find "$SRC/libgd/tests" -type f -name '*.'$lowercase -exec cp {} corpus/$lowercase/ \; | ||
if [ -n "$(ls -A corpus/$lowercase)" ]; then |
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.
really should avoid relying on ls
for anything. since you really only care if the zip exists, test for that instead.
@@ -0,0 +1,27 @@ | |||
// Copyright 2018 Google Inc. | |||
// | |||
// Licensed under the Apache License, Version 2.0 (the "License"); |
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.
GD doesn't use an Apache license. let's avoid adding yet another one to the project if possible.
Here are some details: https://google.github.io/oss-fuzz/getting-started/new-project-guide/#prerequisites We are already covered there (I did not check the report here). This PR and the other follows google's recommendation to have these configs on our side. I can see a few tweaks are needed and also to add more arch. Then we could run them daily on master or whatever branch we like to. |
implicit docs aren't really docs. they need to be linked from the relevant files so people can find them easily. |
I know, only foi, I think I will dig into it anyway, many options or tools
not setup. :)
…On Thu, Aug 26, 2021, 7:43 PM Mike Frysinger ***@***.***> wrote:
Here are some details:
implicit docs aren't really docs. they need to be linked from the relevant
files so people can find them easily.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#427 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACE6KBBVPPYIMOWBFTYUPLT6YZGVANCNFSM4ENXSDSA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
Some questions:
cf #426