-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Update fuzzer and integrate with OneFuzz #4135
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
azure-pipelines.yml
Outdated
- job: 'Fuzzing' | ||
timeoutInMinutes: 60 | ||
dependsOn: 'GetReleaseTag' | ||
condition: always() |
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 feel strongly that this condition should be changed so that the fuzzing
job only runs for the master branch. I don't think we should overload the onefuzz task by submitting artifacts for every PR build as the task doesn't immediately give us any information about fuzzing bugs. Those are autogenerated by the onefuzz service and sent as an ADO notification. It would be less noisy if it only ran for a master branch commit because that is actual code that is checked in and not being worked on.
If there are no objections, I will change this after this PR is reviewed and approved so that we can see it run to completion.
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.
Yes, if "the task doesn't immediately give us any information about fuzzing bugs", then we should consider running it only on ci builds
"AdoTemplate": { | ||
"Org": "ms", | ||
"Project": "winget-cli", | ||
"AssignedTo": "[email protected]", |
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.
nit: Is this required field? Unless you are responsible for triaging all fuzzing related bugs?
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.
yes this is required, and I'll volunteer myself to help with triaging :)
Updates the WinGetYamlFuzzer so that it builds successfully for only the fuzzing configuration and x64 or x86 platforms.
Building a libfuzzer with MSVC is now supported so I removed everything related to creating our own libfuzzer.
I started by removing the WINGET_DISABLE_FOR_FUZZING macro and trying to get the project to build successfully, since a lot has changed since this was last implemented and things are a lot more complex now. I worked backwards by removing references that were not needed and building with the the required ASan and SanCov compiler options.
Microsoft Reviewers: Open in CodeFlow