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

Adds a fuzz test #2233

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

Adds a fuzz test #2233

wants to merge 3 commits into from

Conversation

jesslatimer
Copy link

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

  • Checked that there aren't other open pull requests for
    the same change.
  • Checked that all tests and type checking passes.
  • Considered granting push permissions to the PR branch,
    so maintainers can fix minor issues and keep your PR up to date.

from xml.sax import SAXParseException


def TestOneInput(data):
Copy link
Member

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.

Suggested change
def TestOneInput(data):
def test_one_input(data):

Copy link
Member

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.

Copy link
Author

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.

@aucampia
Copy link
Member

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:

  • How long does the fuzz tester take to run?
  • Does it find anything interesting, and if it does, how will it report it?
  • Could we integrate it with pytest?

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:

  • Add atheris as a dependency in pyproject.toml
  • Add a task to run atheris to Taskfile.yml
  • To then run as part of the CI pipeline, the simplest would possibly be to add it as an extra-task similar to this
    matrix:
    include:
    - task: "gha:flake8"
    python-version: 3.8
  • This should have a corresponding gha:atheris entry in the taskfile similar to this:

    rdflib/Taskfile.yml

    Lines 260 to 266 in 4da67f9

    gha:flake8:
    desc: GitHub Actions flake8 workflow
    cmds:
    - task: poetry:configure
    vars:
    CLI_ARGS: --no-root --only=flake8
    - task: flake8

@aucampia
Copy link
Member

@RDFLib/core-reviewers if anyone has some input on this, it will be appreciated.

Comment on lines 21 to 28
except (SAXParseException, IndexError, Exception):
pass
Copy link
Member

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.

Copy link
Author

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)

@jesslatimer
Copy link
Author

How long does the fuzz tester take to run?

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.

Does it find anything interesting?

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.

How will it report it?

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.

Could we integrate it with pytest?

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.

If we do add fuzz testing, it should ideally be integrated with our CI and pipeline, so it keeps working.

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.

@jesslatimer jesslatimer force-pushed the main branch 2 times, most recently from 4e156ab to faca78f Compare April 1, 2023 20:24
@ghost
Copy link

ghost commented Apr 2, 2023

@RDFLib/core-reviewers if anyone has some input on this, it will be appreciated.

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))

@jesslatimer
Copy link
Author

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 ...

@gjhiggins I am happy to add to this fuzzer and update it here.
Are there any particular functions/methods you would like me to focus on?

@jesslatimer
Copy link
Author

pre-commit.ci autofix

@jesslatimer jesslatimer marked this pull request as ready for review April 18, 2023 18:17
@jesslatimer
Copy link
Author

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.

@aucampia
Copy link
Member

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.

@aucampia aucampia added the on hold Progress on this issue is blocked by something. label May 21, 2023
@aucampia
Copy link
Member

PRs to V6 is closed until further notice. See this for more details:

@aucampia aucampia added needs more work The PR needs more work before being merged or further reviewed. and removed on hold Progress on this issue is blocked by something. labels Jun 2, 2023
@aucampia
Copy link
Member

aucampia commented Jun 9, 2023

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more work The PR needs more work before being merged or further reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants