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 for frontend dockerfile parser #1813

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

Conversation

AdamKorcz
Copy link

This PR reopens #1518

From that PR the following modifications have been made:

1: A docker image has been added that runs the fuzzer. To test this, do the following:

cd buildkit-root
make fuzz

In this regard, the /hack script has been significantly rewritten, as the previous script installed dependencies on the host machine.

Please note that this does affect the host machine in the sense that the fuzzer is copied into the hack/dockerfiles directory. This will be removed once the fuzzer is merged in and is merely a way to test the fuzzer prior to merging.

2: With suggestion from @AkihiroSuda, the file creation inside of the fuzzer has been replaced by bytes.NewReader(data) which provided a big effectivity boost, so thank you for that.

3: The fuzzer has been moved to the util/testutil/fuzz directory.

I am tagging @tiborvass and @tonistiigi as you were also involved in the previous PR.

Also, I would like to suggest setting up continuous fuzzing of the Moby project. Some benefits of this will be:

  1. It makes sure that the fuzzers are actually executed regularly. It happens often that efforts are being put into writing the fuzzing suite, and they are only rarely run for a short ammount of time or not run at all.
  2. Fuzzing for longer periods of time helps find bugs that aren't found through shorter runs. There are cases of some bugs having been found after several CPU-years of fuzzing, and this vulnerability in Kubernetes has been discovered through continuous fuzzing by oss-fuzz.
  3. It will make further addition of fuzzers easy, as they will be run continuously immediately after integration. No additional setup (besides a miniscule addition to the build scripts) is necessary after the initial setup has been made.

I have worked on the integration with oss-fuzz to set up continuous fuzzing for the Moby project, and I will be happy to complete that integration. All I need for that is a list of maintainer emails for potential bug reports. These will be added to a public list that can be changed at any time.
oss-fuzz has a public disclosure policy of 90 days.

hack/fuzz Outdated Show resolved Hide resolved
Signed-off-by: AdamKorcz <[email protected]>
@moby moby deleted a comment from codecov-io Nov 25, 2020
@AdamKorcz
Copy link
Author

@AkihiroSuda The hack/fuzz breaks now because the fuzzer is not yet merged. I assume this is why the build fails

Signed-off-by: AdamKorcz <[email protected]>
@AdamKorcz
Copy link
Author

My previous assumption was partly correct, and my latest fix demonstrates that it was not because the fuzzer hasn't been merged yet but rather because the util/testutil/fuzz directory has not been created yet which it will be when this PR is merged.

@AdamKorcz
Copy link
Author

If there is anything I can do from my side, please let me know.

@tonistiigi
Copy link
Member

Either the Dockerfile or the hack script should actually start the fuzzer.

Should we close this for now and revisit then fuzzer is directly added in Go(I believe next version)?

@AdamKorcz
Copy link
Author

Should we close this for now and revisit then fuzzer is directly added in Go

The upside of waiting as I see it would be to avoid having to rewrite the fuzzer.

The downside is that Buildkit does not get fuzzed, and any bugs that fuzzing could find will not be found until fuzzing is released as part of Go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants