projects: Add GStreamer #905

bilboed · 2017-10-19T11:22:09Z

This is an initial fuzzer which goes over ogg/theora/vorbis files
using the discoverer process

This is an initial fuzzer which goes over ogg/theora/vorbis files using the discoverer process

googlebot · 2017-10-19T11:22:13Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

bilboed · 2017-10-19T11:23:38Z

I signed it!

googlebot · 2017-10-19T11:23:42Z

CLAs look good, thanks!

Dor1s

Sorry for the delayed review! Looks good, I left a few minor questions comments. Didn't review the fuzz target itself though.

Dor1s · 2017-10-20T16:24:33Z

projects/gstreamer/Dockerfile

@@ -0,0 +1,38 @@
+# Copyright 2016 Google Inc.


2017 here and in other files

Dor1s · 2017-10-20T16:27:48Z

projects/gstreamer/build.sh

+################################################################################
+
+# build project
+# e.g.


can you remove these comments please (lines 18 - 28)?

Dor1s · 2017-10-20T16:30:16Z

projects/gstreamer/build.sh

+# $CXX $CXXFLAGS -std=c++11 -Iinclude \
+#     /path/to/name_of_fuzzer.cc -o /out/name_of_fuzzer \
+#     -lFuzzingEngine /path/to/library.a
+BASEFLAGS="-Wno-constant-conversion -Wno-header-guard -Wno-mismatched-tags -O2"


I think it would be better to remove -O2. We are using -O1 for fuzzing builds, as it seems to have a better balance between speed and number of branches.

Dor1s · 2017-10-20T16:31:49Z

projects/gstreamer/build.sh

+mkdir -p $PREFIX
+cd $WORK
+
+env


is it really needed?

Dor1s

Forgot to select "Request changes"

bilboed · 2017-10-20T16:46:59Z

Thanks for the review. There was indeed some cruft/comments that I removed.

Dor1s · 2017-10-20T17:13:20Z

Thanks for the cleanup Edward!

One more question: can you move fuzz target to the GStreamer repo? This is what we expect projects to do as a part of "ideal integration". Plus, it's much more convenient when you (or anyone else) need to make some changes in the fuzz target itself.

kcc · 2017-10-21T05:53:32Z

projects/gstreamer/build.sh

+echo
+
+cd $OUT
+wget -nd https://people.freedesktop.org/~bilboed/gst-discoverer_seed_corpus.zip


this is better be done in Dockerfile

Downloading the corpus ? Or putting it in $OUT/ ?

Downloading, e.g. https://github.com/google/oss-fuzz/blob/master/projects/libreoffice/Dockerfile

ok, fixing it

kcc · 2017-10-21T05:53:45Z

projects/gstreamer/build.sh

+cd $OUT
+wget -nd https://people.freedesktop.org/~bilboed/gst-discoverer_seed_corpus.zip
+
+cp $LIB_FUZZING_ENGINE /work/


Why do you need this here?

whoops, leftover testing, will remove

kcc · 2017-10-21T05:54:13Z

projects/gstreamer/gst-discoverer.c

@@ -0,0 +1,505 @@
+/* GStreamer


Yea, we encourage to have the fuzz target in the project's repository, not here.
https://github.com/google/oss-fuzz/blob/master/docs/ideal_integration.md

Also check the license.
We don't want files with such license in this repository.

kcc · 2017-10-21T05:55:08Z

projects/gstreamer/project.yaml

+  - address
+  - memory
+  - undefined
+


Just in case, make sure there is a new line at EOF

kcc · 2017-10-21T05:55:39Z

projects/gstreamer/project.yaml

+primary_contact: "bilboed@bilboed.com"
+sanitizers:
+  - address
+  - memory


Assuming you've verified that msan works and doesn't give false positives right away.

Should I just leave the default then ? (i.e. nothing)

kcc · 2017-10-21T05:56:30Z

projects/gstreamer/gst-discoverer.c

+  g_signal_connect (dc, "source-setup", (GCallback) appsrc_configuration, NULL);
+
+  info = gst_discoverer_discover_uri (dc, "appsrc://", &err);
+  print_info (info, err);


If you print something while executing every input, fuzzing will be very inefficient (will spend all the time printing)

Is one/two lines ok for debugging purposes ?

I think you may have it under some debug switch, in case you are going to debug things locally, but for fuzzing build we need to avoid any output.

Fair enough, I'll fix that.

kcc · 2017-10-21T05:58:53Z

projects/gstreamer/build.sh

+      $SRC/gst-discoverer.o \
+      $PLUGINS \
+      $BUILD_LDFLAGS \
+      $LIB_FUZZING_ENGINE \


I'm confused. Why do you link $OUT/gst-discoverer here and gst-discoverer above?

This is the linking phase. The phase above is the compilation phase (C compilation vs C++ linking)

kcc · 2017-10-21T05:59:52Z

projects/gstreamer/build.sh

+#
+################################################################################
+
+BASEFLAGS="-Wno-constant-conversion -Wno-header-guard -Wno-mismatched-tags"


Do you need these?

Will try recompiling without

kcc · 2017-10-21T06:00:32Z

projects/gstreamer/Dockerfile

+
+WORKDIR gstreamer
+COPY build.sh $SRC/
+COPY gst-discoverer.c $SRC/


Just in case, add a new line at EOF

Use the security mailing list as the primary contact Remove explicit sanitizer listing

Removed almost all outputting I am the original author of the code this is taken for, relicensing an ultra-simplified version of my original code to Apache.

Data provided by the fuzzer shouldn't be freed (but the wrapping GstBuffer should). Avoid logging by default

And extract in build.sh

kcc · 2017-10-24T17:13:10Z

projects/gstreamer/build.sh

+
+echo "CFLAGS" $CFLAGS
+echo "CXXFLAGS" $CXXFLAGS
+export LDFLAGS="$SANITIZER_FLAGS $COVERAGE_FLAGS"


I don't think COVERAGE_FLAGS is a part of our interface.

Looks legit https://github.com/google/oss-fuzz/blob/master/infra/base-images/base-builder/README.md#build-configuration

My bad, looked at the wrong section: https://github.com/google/oss-fuzz/blob/master/infra/base-images/base-builder/README.md#compiler-flags

So which is it ? Good or not good ? :)

Not good :)
Just use LDFLAGS="$CXXFLAGS"

We actually didn't need a custom LDFLAGS. Removed it altogether

kcc · 2017-10-24T17:15:24Z

projects/gstreamer/gst-discoverer.c

@@ -0,0 +1,152 @@
+/*


So, any chance to have this file upstream?
https://github.com/google/oss-fuzz/blob/master/docs/ideal_integration.md#tldr

I was going to setup a new repository for GStreamer for all our CI/QA stuff (still located on freedesktop/gstreamer). I'll migrate the code over there and rebase all previous commits. Thanks for your patience/review :)

Done, now upstream : https://cgit.freedesktop.org/gstreamer/gst-ci/tree/fuzzing/gst-discoverer.c

Remove custom LDFLAGS (not needed) Use fuzzing target code from upstream repository

bilboed · 2017-10-29T07:33:06Z

Do you want me to squash everything into one commit before upstreaming ? Instead of several fix-this, fix-this, fix-this ?

Dor1s · 2017-10-29T23:51:35Z

@bilboed, that's fine, github allows us to squash the commits altogether via single click :)

oliverchang · 2017-10-30T02:53:48Z

I think most comments have been addressed (thanks!)

I'm going to go ahead and merge this.

* projects: Add GStreamer This is an initial fuzzer which goes over ogg/theora/vorbis files using the discoverer process * gstreamer/build.sh: Cleanup file * gstreamer/Dockerfile: Update copyright date * gstreamer: Update project.yaml Use the security mailing list as the primary contact Remove explicit sanitizer listing * gstreamer: Simplify base fuzzer Removed almost all outputting I am the original author of the code this is taken for, relicensing an ultra-simplified version of my original code to Apache. * gstreamer: Cleanup of build file and dockerfile * gstreamer: Code minimization and avoid leaks Data provided by the fuzzer shouldn't be freed (but the wrapping GstBuffer should). Avoid logging by default * gstreamer: Download corpus in Dockerfile And extract in build.sh * gstreamer: Move code to repository and more cleanups Remove custom LDFLAGS (not needed) Use fuzzing target code from upstream repository

projects: Add GStreamer

1cb5d2f

This is an initial fuzzer which goes over ogg/theora/vorbis files using the discoverer process

bilboed mentioned this pull request Oct 19, 2017

Potential new projects to integrate with OSS-Fuzz #402

Closed

44 tasks

Dor1s reviewed Oct 20, 2017

View reviewed changes

Dor1s suggested changes Oct 20, 2017

View reviewed changes

gstreamer/build.sh: Cleanup file

a4ae51d

gstreamer/Dockerfile: Update copyright date

029ef53

Dor1s approved these changes Oct 20, 2017

View reviewed changes

kcc reviewed Oct 21, 2017

View reviewed changes

bilboed added 5 commits October 21, 2017 11:14

gstreamer: Update project.yaml

5ed9b51

Use the security mailing list as the primary contact Remove explicit sanitizer listing

gstreamer: Simplify base fuzzer

a9673d5

Removed almost all outputting I am the original author of the code this is taken for, relicensing an ultra-simplified version of my original code to Apache.

gstreamer: Cleanup of build file and dockerfile

57610cc

gstreamer: Code minimization and avoid leaks

09c1dd6

Data provided by the fuzzer shouldn't be freed (but the wrapping GstBuffer should). Avoid logging by default

gstreamer: Download corpus in Dockerfile

5889da1

And extract in build.sh

kcc reviewed Oct 24, 2017

View reviewed changes

gstreamer: Move code to repository and more cleanups

af88fc7

Remove custom LDFLAGS (not needed) Use fuzzing target code from upstream repository

oliverchang merged commit 5a748a1 into google:master Oct 30, 2017

projects: Add GStreamer #905

projects: Add GStreamer #905

Conversation

bilboed commented Oct 19, 2017

googlebot commented Oct 19, 2017

bilboed commented Oct 19, 2017

googlebot commented Oct 19, 2017

Dor1s left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dor1s left a comment

Choose a reason for hiding this comment

bilboed commented Oct 20, 2017

Dor1s commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bilboed commented Oct 29, 2017

Dor1s commented Oct 29, 2017

oliverchang commented Oct 30, 2017

Dor1s left a comment •

edited

Loading