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

Add Jerryscript to OSS-fuzz #4344

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AdamKorcz
Copy link

@AdamKorcz AdamKorcz commented Nov 19, 2020

I have been working on settin up continuous fuzzing of Jerryscript on OSS-fuzz. This will allow Google to run the existing and all future fuzzers continuously with the goal of finding bugs and security vulnerabilities. If and when a bug is found, all maintainers on the mailing list get notified with a detailed bug report.

Fuzzing continuously has many advantages, some of which are:

  • It makes sure that the fuzzers are actually run. It happens often that efforts are put into writing the fuzzers, but they are only rarely run if at all.
  • Fuzzers can find deeper bugs with more time. There are cases of bugs having been found after several CPU years of continuous fuzzing.

I have the integration build ready to setup continuous fuzzing of Jerryscript by way of OSS-fuzz. To complete the integration, all I need are the email addresses of maintainers to receive potential bug reports. These will be added to a public list that can be changed at any time. No more changes will need to be made on the Jerryscript repository at this point, but overtime it is recommended to move the build scripts upstream from OSS-fuzz. This usually happens a few months after integration.

Regarding the changes made in this PR: If there is interest from the maintainers to setup continuous fuzzing by way of OSS-fuzz, then the -fsanitize=address flag will need to be removed from the build system. Once integrated, I will be happy to bring this flag back with a seperate flag in the tools/build.py script, but for now I suggest it just gets removed.

JerryScript-DCO-1.0-Signed-off-by: AdamKorcz [email protected]

@akosthekiss
Copy link
Member

Hi @AdamKorcz , thanks for reaching out to us with the idea of setting up OSS-fuzz for JerryScript. The general idea is OK, even appealing, I'd say. But I have some "buts", or questions.

This will allow Google to run the existing and all future fuzzers

Q: Are you speaking on behalf of Google?

all I need are the email addresses of maintainers to receive potential bug reports. These will be added to a public list that can be changed at any time.

Q: Is that you who needs the email addresses? Who collects the email addresses / where are the email addresses collected? Where is that mailing list? How does a change happen? Is there a proxy who needs to be contacted to manage the addresses? (GDPR?)

the -fsanitize=address flag will need to be removed from the build system. Once integrated, I will be happy to bring this flag back with a seperate flag in the tools/build.py script, but for now I suggest it just gets removed.

Q: Put simply: Why? I see no reason to remove a working feature from the build system and degrade its behaviour for a promise of future re-addition (in several months time?). If the tweaking of -fsanitize=address is the only way to get things working, then that change should be proposed now so that we can see what this is all about.

@AdamKorcz
Copy link
Author

Hi @akosthekiss, thank you for your good questions.

Q: Are you speaking on behalf of Google?

No. I am an independent contributor to the OSS-fuzz project. I will be happy to refer you to a member of the OSS-fuzz team.

Q: Is that you who needs the email addresses? Who collects the email addresses / where are the email addresses collected? Where is that mailing list? How does a change happen? Is there a proxy who needs to be contacted to manage the addresses? (GDPR?)

I need them for the purpose of adding them to the mailing list. You can see an example of a mailing list has here. A change to that list happens by making a pull request to the oss-fuzz repository like this. In terms of a proxy who needs to manage the addresses: No there isn't one. All updates are reported directly on the OSS-fuzz repository.
I am completely fine with integrating Jerryscript into OSS-fuzz and you sharing email addresses over there instead of here. Just let me know beforehand if you are open to sharing these.

Q: Put simply: Why? I see no reason to remove a working feature from the build system and degrade its behaviour for a promise of future re-addition (in several months time?). If the tweaking of -fsanitize=address is the only way to get things working, then that change should be proposed now so that we can see what this is all about.

The -fsanitize flag needs to be set by OSS-fuzz. In terms of the timeframe for bringing back the address sanitizer in the build script: Once there is an OK from OSS-fuzz and from Jerryscript's side, I will be happy to write a separate rule to manually add the address sanitizer before anyting is merged anywhere.

I hope the above answers your questions, otherwise please feel free to ask. Also, I should mention that the public disclosure policy of OSS-fuzz is 90 days.

@AdamKorcz
Copy link
Author

I have made a few modifications to allow the -fsanitize flag to be added in case the --oss-fuzz flag is not passed to build.py.

jerry-main/CMakeLists.txt Outdated Show resolved Hide resolved
tools/build.py Outdated Show resolved Hide resolved
JerryScript-DCO-1.0-Signed-off-by: AdamKorcz [email protected]
Copy link
Member

@rerobika rerobika left a comment

Choose a reason for hiding this comment

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

LGTM

@rerobika rerobika requested a review from akosthekiss December 12, 2020 13:37
@rerobika rerobika added the fuzzing Related to fuzz testing of the engine label Dec 12, 2020
tools/build.py Show resolved Hide resolved
jerry-main/CMakeLists.txt Show resolved Hide resolved
@akosthekiss
Copy link
Member

Also, please, squash commits. The project has a one commit per PR policy.

@akosthekiss
Copy link
Member

Hm. I've been experimenting with OSS-Fuzz during the past few days and read the relevant docs. It seems that it it possible to set up JerryScript for/in OSS-Fuzz without any modifications to the project (build system included). Let's put this PR on hold a bit.

@AdamKorcz
Copy link
Author

It seems that it it possible to set up JerryScript for/in OSS-Fuzz without any modifications to the project (build system included)

That is correct. I will amend the setup to not introduce any intrusive modifications to Jerryscript. Are you interested in receiving the bug reports for Jerryscript @akosthekiss ?

@AdamKorcz
Copy link
Author

As discussed I have amended the build files on the OSS-fuzz side, and the build now succeeds: google/oss-fuzz#4831

@AdamKorcz
Copy link
Author

@akosthekiss Kind reminder. Let me know if there is anything you need from my side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fuzzing Related to fuzz testing of the engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants