-
Notifications
You must be signed in to change notification settings - Fork 560
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
Adds a fuzz test #2233
base: main
Are you sure you want to change the base?
Adds a fuzz test #2233
Conversation
test/fuzz_rdflib.py
Outdated
from xml.sax import SAXParseException | ||
|
||
|
||
def TestOneInput(data): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is the style that atheris examples use, but I would prefer rather using PEP-8 compatible naming.
def TestOneInput(data): | |
def test_one_input(data): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what is happening will be clearer to reviewers if there are type hints, I am not sure what data will be here, and not sure why you check the length of data and why it must be less than 20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Let me know if anything needs more clarification.
Thanks for the Pull request, I'm open to the idea of adding Fuzz testing to RDFLib, but I'm not that familiar with Fuzz testing, so some clarification would be helpful on these points:
If we do add fuzz testing, it should ideally be integrated with our CI and pipeline, so it keeps working. To do that you would have to do the following:
|
@RDFLib/core-reviewers if anyone has some input on this, it will be appreciated. |
test/fuzz_rdflib.py
Outdated
except (SAXParseException, IndexError, Exception): | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having Exception
here may be over broad, as that includes the other two errors and potentially errors that should not occur.
I think even IndexError may constitute an error, we should possibly consider what Exceptions make sense, and possibly clean up the exceptions raised by RDFLib to be more sane.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I have changed the fuzz test to only except issues with inappropriate inputs (as far as I can tell - let me know if you think they could also be bugs)
Fuzz tests run indefinitely or until a bug is found. You can also set a time-out. If you integrate with OSS-Fuzz it will run once a day for a few hours across multiple computers.
I am making a few changes to this fuzz test and did see that the IndexError is likely a bug. Would you like me to report any bugs I find? I believe all the errors I'm catching now are the result of inappropriate data being parsed, but please let me know if you would like me to remove any and report the error.
Locally, when it crashes it will output the stacktrace and a crash file with the data that caused it. Through OSS-Fuzz, it will give you a web interface to view statistics like coverage, crashes, speed and memory usage, which only developers can see. Crash reports are also provided with a stacktrace and a crash file with the data that caused it.
Theoretically yes, but it does not necessarily make sense to. Pytest runs unit tests serially with a defined start and end time, however, fuzz tests run for an arbitrary amount of time (or until they find a bug), so if the first fuzz test does not find a bug or timeout, the following tests will never run.
An easy way to integrate into your CI and pipeline would be through OSS-Fuzz github action. This link explains it best, but briefly, it uses CIFuzz which runs for a configurable amount of time (default is 10min) on each pull request. |
4e156ab
to
faca78f
Compare
I'm quite well-disposed towards this PR (thank you Jess for the initiative). There are a few RDFLIb functions/methods (both internal and API-exposed) that are likely to be even better-suited to fuzz testing than the RDF parsers. I found the fuzzing test setup for the rfc3986 Python package to be informative is this aspect ... e.g. def fuzz_iri(data):
fdp = atheris.FuzzedDataProvider(data)
iri_ref = rfc3986.IRIReference.from_string(
fdp.ConsumeUnicodeNoSurrogates(256)) |
@gjhiggins I am happy to add to this fuzzer and update it here. |
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
A nice and easy way to find the functions that will result in the most amount of code covered by the fuzzer is to integrate with OSS-Fuzz and utilise introspector. Introspector tells you the optimal functions to target that result in the maximum amount of code coverage by the fuzzer. Therefore, the most efficient way to develop a fuzzer is to integrate a small fuzzer and go from there. If this interests you, I have submitted a PR with OSS-Fuzz to get the ball rolling but I still need a developer's primary contact email and a comment on said PR giving their approval. |
I think it is quite essential to integrate this with the CI and some Taskfile targets; otherwise we really don't get the benefit from this, as people will forget to run it, and it will also likely stop working otherwise as various things change while the fuzz testing execution does not get updated. |
PRs to V6 is closed until further notice. See this for more details: |
That issue is now settled and PRs are open again, but this is still the case.
|
Summary of changes
Adds a fuzz test that targets the parser for each default plugin using atheris to further ensure rdflib is reliable and secure.
Fuzz tests throw thousands of different scenarios at the code, ensuring it can handle invalid and unusual inputs as well as find and test those hard-to-reach/overlooked edge cases sitting deep within your code.
Checklist
the same change.
so maintainers can fix minor issues and keep your PR up to date.