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.

Sign up for GitHub

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

Jump to bottom

fuzz: introduce continuous fuzzing for Caddy #2723

Merged
merged 5 commits into from
Oct 26, 2019
Merged

fuzz: introduce continuous fuzzing for Caddy #2723

merged 5 commits into from
Oct 26, 2019

Conversation

mohammed90
Copy link
Member

@mohammed90 mohammed90 commented Aug 23, 2019 •
edited
Loading

1. What does this change do, exactly?

Introduce fuzzers to Caddy's codebase for continuous fuzzing on CI. Closes #2710 .

2. Please link to the relevant issues.

#2710

3. Which documentation changes (if any) need to be made because of this PR?

N/A

4. Checklist

  • I have written tests and verified that they fail without my change
  • I have squashed any insignificant commits
  • This change has comments explaining package types, values, functions, and non-obvious lines of code
  • I am willing to help maintain this change if there are issues with it later

@mohammed90 mohammed90 force-pushed the v2-fuzz branch 4 times, most recently from 229005c to 3f66cb5 Compare August 23, 2019 14:42
@mohammed90 mohammed90 changed the title (WIP) fuzz: laydown the foundation for continuous fuzzing (WIP) fuzz: introduce continuous fuzzing for Caddy Aug 23, 2019
@mohammed90 mohammed90 force-pushed the v2-fuzz branch 6 times, most recently from 56f2492 to 71eea0c Compare August 23, 2019 16:37
mholt
mholt reviewed Sep 1, 2019
View reviewed changes
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

This is looking pretty good. What's the status of the draft at this point?

fuzz/go.mod Outdated
@@ -0,0 +1,10 @@
module local.tld/fuzz
Copy link
Member

Choose a reason for hiding this comment

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

Does the fuzzer need to be its own module? AFAIK, having multiple modules in a repository can cause complications in some cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, yes due to go-fuzz not yet being able to handle modules. Creating another module is a workaround as described by mvdan here. It will not work without it due to SIV /v2.

The only pain points I've been experiencing is the tendency of the tools to automatically adding the go-fuzz module to Caddy's go.mod and adding Caddy v1.0.3 to Caddy v2's go.sum. I had to keep an eye on them and constantly revert them.

Copy link
Member

Choose a reason for hiding this comment

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

Are we going to have to keep go.mod and go.sum maintained then, you think? Dang, I wish they'd make it work with modules... this might be kind of annoying.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure. Do you think this is a bug? Should it modify the root go.mod and go.sum with dependencies of the submodule?

@mohammed90
Copy link
Member Author

It's ready for review. The only reason it's still marked as draft for now is because it needs one more commit to uncomment a few lines in the azure-pipelines.yml file and the badge in the README.

@mholt
Copy link
Member

mholt commented Sep 2, 2019

Okay, I have registered the Fuzzit API key with Azure Pipelines and uploaded the targets to Fuzzit. It should be ready to go I think!

mholt
mholt requested changes Sep 2, 2019
View reviewed changes
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

The pipeline is ready to go. This PR looks good, just uncomment the env var bit and I'll probably approve this PR when the fuzzing works in CI. :)

azure-pipelines.yml Outdated Show resolved Hide resolved
fuzz/README.md Outdated Show resolved Hide resolved
fuzz/go.mod Outdated
@@ -0,0 +1,10 @@
module local.tld/fuzz
Copy link
Member

Choose a reason for hiding this comment

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

Are we going to have to keep go.mod and go.sum maintained then, you think? Dang, I wish they'd make it work with modules... this might be kind of annoying.

@mohammed90 mohammed90 force-pushed the v2-fuzz branch 6 times, most recently from 07a5122 to c234ac1 Compare September 3, 2019 20:23
@mohammed90 mohammed90 marked this pull request as ready for review September 3, 2019 22:22
@mholt
Copy link
Member

mholt commented Sep 5, 2019

Waiting on dvyukov/go-fuzz#263

@mholt mholt added the v2 label Sep 5, 2019
@mholt mholt added this to the 2.0 milestone Sep 5, 2019
@mholt
Copy link
Member

mholt commented Oct 15, 2019

Now I guess we're waiting on dvyukov/go-fuzz#274 ?

@mohammed90
Copy link
Member Author

mohammed90 commented Oct 15, 2019 •
edited
Loading

Now I guess we're waiting on dvyukov/go-fuzz#274 ?

For perfect solution? yes, I presume. However, mvdan is in the same boat as us, and he was able to do it without the temporary GOPATH (but still with the submodule) in this commit. I'll fiddle with it again this coming weekend.

@mholt
Copy link
Member

mholt commented Oct 15, 2019

I really really don't want to do a submodule. I guess we can wait until they're ready to support modules.

@mohammed90
Copy link
Member Author

Note for later in case I forget: once go-fuzz gets proper module support, I don't think we'll need the fuzz/ directory. Fuzzers' files can live next to their siblings within the modules they're fuzzing.

@thepudds
Copy link

Now that dvyukov/go-fuzz#274 is merged, I would be curious to hear if it works for you. ;-)

mohammed90
mohammed90 commented Oct 25, 2019
View reviewed changes
azure-pipelines.yml
displayName: 'Scheduled Fuzzing'
# Only run this job on schedules, not PRs.
condition: eq(variables['Build.Reason'], 'Schedule')
Copy link
Member Author

Choose a reason for hiding this comment

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

Azure Pipelines doesn't seem to support having a particular job on schedule while others triggered on PR, hence the condition.

@mohammed90 mohammed90 requested a review from mholt October 25, 2019 19:47
@mohammed90
Copy link
Member Author

@thepudds the latest go-fuzz works like a charm with modules. Thank you!

@mohammed90 mohammed90 changed the title (WIP) fuzz: introduce continuous fuzzing for Caddy fuzz: introduce continuous fuzzing for Caddy Oct 25, 2019
@mholt mholt merged commit 2fbe2ff into caddyserver:v2 Oct 26, 2019
@mholt
Copy link
Member

mholt commented Oct 26, 2019

Great! Thanks so much for all the work you put into this, @mohammed90 !

@mohammed90 mohammed90 deleted the v2-fuzz branch October 26, 2019 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mholt mholt mholt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.x
Development

Successfully merging this pull request may close these issues.
3 participants
@mohammed90 @mholt @thepudds