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

Integrate fuzzer #427

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Integrate fuzzer #427

wants to merge 1 commit into from

Conversation

tdsmith
Copy link
Contributor

@tdsmith tdsmith commented Jan 26, 2018

Some questions:

  • Is this a good place to put the fuzzer targets and build scripts?
  • It's nice to have a place to manually put reproducers for discovered bugs, so storing at least part of the corpus explicitly in the repository is useful. OTOH, the AFL gif and bmp corpuses are together about 1300 files and 7mb, which is unwieldy. Maybe I should go back to just fetching the AFL corpus from the web in the oss-fuzz Dockerfile and merging them in during the build. What do you think?
  • Should this be more refined before it's merged or is it okay to work on it piecemeal?

cf #426

@tdsmith
Copy link
Contributor Author

tdsmith commented Jan 26, 2018

These are the matching changes on the oss-fuzz side; https://github.com/google/oss-fuzz/compare/master...tdsmith:libgd-integration?expand=1

@vapier
Copy link
Member

vapier commented Jan 26, 2018

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

Add fuzzing targets for oss-fuzz.
@tdsmith
Copy link
Contributor Author

tdsmith commented Feb 4, 2018

Sorry for the delay! I made these changes.

@tdsmith tdsmith mentioned this pull request Feb 4, 2018
9 tasks
@tdsmith
Copy link
Contributor Author

tdsmith commented Feb 12, 2018

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.

@pierrejoye
Copy link
Contributor

It would be great to have that in the github actions, thoughts?

@pierrejoye
Copy link
Contributor

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,
Copy link
Member

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

Copy link
Member

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"
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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");
Copy link
Member

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.

@pierrejoye
Copy link
Contributor

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.

@vapier
Copy link
Member

vapier commented Aug 26, 2021

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.

@pierrejoye
Copy link
Contributor

pierrejoye commented Aug 26, 2021 via email

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.

None yet

3 participants