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

Added fuzzer #501

Merged
merged 4 commits into from
May 10, 2020
Merged

Added fuzzer #501

merged 4 commits into from
May 10, 2020

Conversation

AdamKorcz
Copy link
Contributor

This PR adds a fuzzer that targets Compile()

I managed to get the fuzzer running on oss-fuzz's platform as well, and I propose setting ferret up on in oss-fuzz. This will allow oss-fuzz to run this fuzzer as well as future fuzzers continuously. If a bug is discovered, oss-fuzz sends a bug report via email to all maintainers on their contact list.
It doesn't cost anything to be a part of oss-fuzz, but the bugs do need to get fixed.

If you would like to setup ferret on oss-fuzz, I need a primary contact email address and the email addresses of all maintainers to add to the email list. This can be updated at anytime.

Google might ask about the userbase of ferret in order to accept the project, so if you have any information about any companies or other packages using ferret, it would help with getting ferret accepted.

@ziflex
Copy link
Member

ziflex commented May 10, 2020

Hey, Adam, that's a great idea, thanks for offering!
The Ferret team would like to participate in this. Where should I leave the information?

@AdamKorcz
Copy link
Contributor Author

AdamKorcz commented May 10, 2020

@ziflex Awesome! Feel free to leave the email address here, or if you prefer, please send it to me at adam /at/ adalogics /dot/ com.

Also, kindly accept the fuzzer before I set things up on the oss-fuzz side. Otherwise their Travis will look for the fuzzer and fail when it doesn't find it.

Copy link
Member

@3timeslazy 3timeslazy left a comment

Choose a reason for hiding this comment

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

According to oss-fuzz guide there are also should be following:

  • project.yaml file
fuzzing_engines:
  - libfuzzer

sanitizers:
  - address
  • Dockerfile should contain
RUN go get github.com/ianlancetaylor/demangle
  • call go-fuzz command

If I made a mistake somewhere, correct me :)

func Fuzz(data []byte) int {
c := compiler.New()
p, err := c.Compile(string(data))
if err!=nil || p==nil{
Copy link
Member

Choose a reason for hiding this comment

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

variables and operators should be separated by space. So, line should be if err != nil || p == nil

"github.com/MontFerret/ferret/pkg/compiler"
)

func Fuzz(data []byte) int {

This comment was marked as outdated.

@AdamKorcz
Copy link
Contributor Author

AdamKorcz commented May 10, 2020

@3timeslazy This is not needed on the upstream repository. These commands are needed to setup the fuzzers on oss-fuzz's systems. The fuzzer uploaded is what is needed on upstream repository to run the fuzzer.

You are correct that they are needed. I have the build scripts all ready with the exact commands that you are mentioning :)

func Fuzz(data []byte) int {
c := compiler.New()
p, err := c.Compile(string(data))
if err != nil || p == nil{
Copy link
Member

Choose a reason for hiding this comment

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

"github.com/MontFerret/ferret/pkg/compiler"
)

// This function is our fuzzer
Copy link
Member

Choose a reason for hiding this comment

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

This comment is not descriptive enough and does not satisfy Go's code style. Write something like

// Fuzz is a fuzzer for oss-fuzz platform.
//
// Read more about it here: https://github.com/google/oss-fuzz
func Fuzz(data []byte) int {}

@3timeslazy
Copy link
Member

@AdamKorcz thanks for explanation!

P.S.
Added a few comments :)

@AdamKorcz
Copy link
Contributor Author

AdamKorcz commented May 10, 2020

@3timeslazy Thanks for the comments. Regarding the in-code comment: The fuzzer is not specific to oss-fuzz and it can be run outside of oss-fuzz as well. Hence I suggest to not make the description of it too specific to oss-fuzz.
If ferret is added to oss-fuzz, I suggest adding the link to the project in the code. That should be more explanatory.

@codecov
Copy link

codecov bot commented May 10, 2020

Codecov Report

Merging #501 into master will decrease coverage by 0.1%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master    #501     +/-   ##
========================================
- Coverage    41.3%   41.3%   -0.1%     
========================================
  Files         234     234             
  Lines        8966    8966             
========================================
- Hits         3706    3701      -5     
- Misses       4903    4906      +3     
- Partials      357     359      +2     

@3timeslazy
Copy link
Member

@AdamKorcz you're right about a too specific comment.

And the last comment :) Replace Below function with Fuzz pls. This is a code style requirement. Comment should begin with the function name.

Source: https://github.com/golang/go/wiki/CodeReviewComments#comment-sentences

@ziflex ziflex requested a review from 3timeslazy May 10, 2020 17:47
Copy link
Member

@3timeslazy 3timeslazy left a comment

Choose a reason for hiding this comment

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

Amazing! Thanks for PR!

@ziflex ziflex merged commit 311d07a into MontFerret:master May 10, 2020
@ziflex
Copy link
Member

ziflex commented May 10, 2020

Thank you all for the PR!
Keep fingers crossed that the project gets approval.

@AdamKorcz
Copy link
Contributor Author

Thank you from here as well for all the good comments on a sunday. Crossing my fingers!

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.

3 participants