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 initial fuzz test #196

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DavidKorczynski
Copy link

Hi,

I was wondering if you would like to integrate continuous fuzzing of python-rsa by way of OSS-Fuzz? In this PR google/oss-fuzz#7516 I do exactly that, namely created the necessary logic from an OSS-Fuzz perspective to integrate python-rsa.

This includes developing initial fuzzers as well as integrating into OSS-Fuzz, however, it is preferable to have the fuzzers upstream so I included it in this PR - if you are happy with having the fuzzers here then I will remove them from the OSS-Fuzz repository.

Essentially, OSS-Fuzz is a free service run by Google that performs continuous fuzzing of important open source projects. The only expectation of integrating into OSS-Fuzz is that bugs will be fixed. This is not a "hard" requirement in that no one enforces this and the main point is if bugs are not fixed then it is a waste of resources to run the fuzzers, which we would like to avoid.

If you would like to integrate, the only thing I need is as list of email(s) that will get access to the data produced by OSS-Fuzz, such as bug reports, coverage reports and more stats. Notice the emails affiliated with the project will be public in the OSS-Fuzz repo, as they will be part of a configuration file.

Initial fuzz test for string operations roundtrip
@sybrenstuvel
Copy link
Owner

Thanks for the offer! I'm quite curious to see what the fuzz tests can expose.

As for the pull request, I can't accept it as-is. The atheris module should be listed as optional development dependency, and the test should be gracefully skipped when it cannot be imported.

I'm also curious why the tests simply return on a ValueError or OverflowError. IMO this should be documented in a comment above the respective return statements, so that it's clear for anyone reading the code.

@DavidKorczynski
Copy link
Author

Thanks for letting me know you're interested in fuzzing @sybrenstuvel -- I will look to address the issues you mention!

@DavidKorczynski
Copy link
Author

The atheris module should be listed as optional development dependency, and the test should be gracefully skipped when it cannot be imported.

Am not sure how you would prefer this. The fuzzer is not meant to be run similar to the other tests in that a fuzzer never really terminates but is meant to be run continuously. I would advice to have the continuous running of it handled by oss-fuzz, and as such it may be better to move it to another directory than tests/? In that case, where would you prefer to have the dependency atheris listed? Is it here

[tool.poetry.dev-dependencies]
?

I'm also curious why the tests simply return on a ValueError or OverflowError. IMO this should be documented in a comment above the respective return statements, so that it's clear for anyone reading the code.

I put in comments in the exception handling now, let me know what you think.

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.

2 participants