-
Notifications
You must be signed in to change notification settings - Fork 8.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
Add a Fuzzing configuration and a version of conhost that can be fuzzed #9604
Conversation
Misspellings found, please review:
To accept these changes, run the following commands from this repository on this branch
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉 🗜️ If you see a bunch of garbage and it relates to a binary-ish string, please add a file path to the File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
|
@@ -39,7 +39,11 @@ try | |||
{ | |||
Globals& Globals = ServiceLocator::LocateGlobals(); | |||
|
|||
Globals.pDeviceComm = new ConDrvDeviceComm(Server); | |||
if (!Globals.pDeviceComm) |
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.
these were the only changes necessary to conhost.
The only reason they are necessary is so that I can pass an invalid server handle 😄
👀 |
6ef1506
to
2868fc5
Compare
Misspellings found, please review:
To accept these changes, run the following commands from this repository on this branch
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉 🗜️ If you see a bunch of garbage and it relates to a binary-ish string, please add a file path to the File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
|
So is this the result of discussions mentioned in #7638 and will be part of CI? |
Sort of. It just so happened to be Fix Hack Learn week this week and @DHowett was playing with fuzzers to try to tackle the |
I think nightly would be best to keep the CI from becoming too slow and each WT/conhost release should have the results published somewhere for the public to review. That GH Action they publish is still likely the best way forward. |
Thanks for your input. |
I've marked this PR as "ready for review" and filled out the body. |
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.
Looks pretty good. Just the two comments but I trust you to resolve them before hitting merge so I'm just gonna leave the Approve check.
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 look forward to messing up the configuration on more projects in the future
{099193A0-1E43-4BBC-BA7F-7B351E1342DF}.Fuzzing|Any CPU.ActiveCfg = Debug|Win32 | ||
{099193A0-1E43-4BBC-BA7F-7B351E1342DF}.Fuzzing|ARM.ActiveCfg = Debug|Win32 | ||
{099193A0-1E43-4BBC-BA7F-7B351E1342DF}.Fuzzing|ARM64.ActiveCfg = Fuzzing|ARM64 | ||
{099193A0-1E43-4BBC-BA7F-7B351E1342DF}.Fuzzing|DotNet_x64Test.ActiveCfg = Fuzzing|Win32 | ||
{099193A0-1E43-4BBC-BA7F-7B351E1342DF}.Fuzzing|DotNet_x86Test.ActiveCfg = Debug|Win32 | ||
{099193A0-1E43-4BBC-BA7F-7B351E1342DF}.Fuzzing|x64.ActiveCfg = Debug|x64 | ||
{099193A0-1E43-4BBC-BA7F-7B351E1342DF}.Fuzzing|x86.ActiveCfg = Fuzzing|Win32 |
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.
this one's got a weird mix of debug/fuzzing
{376FE273-6B84-4EB5-8B30-8DE9D21B022C}.Fuzzing|Any CPU.ActiveCfg = Debug|Any CPU | ||
{376FE273-6B84-4EB5-8B30-8DE9D21B022C}.Fuzzing|ARM.ActiveCfg = Debug|Any CPU | ||
{376FE273-6B84-4EB5-8B30-8DE9D21B022C}.Fuzzing|ARM64.ActiveCfg = Fuzzing|Any CPU | ||
{376FE273-6B84-4EB5-8B30-8DE9D21B022C}.Fuzzing|DotNet_x64Test.ActiveCfg = Fuzzing|Any CPU | ||
{376FE273-6B84-4EB5-8B30-8DE9D21B022C}.Fuzzing|DotNet_x86Test.ActiveCfg = Debug|Any CPU | ||
{376FE273-6B84-4EB5-8B30-8DE9D21B022C}.Fuzzing|x64.ActiveCfg = Debug|Any CPU | ||
{376FE273-6B84-4EB5-8B30-8DE9D21B022C}.Fuzzing|x86.ActiveCfg = Fuzzing|Any CPU |
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.
Same with this one, but it's all Any Cpu so maybe that doesn't matter?
{1588FD7C-241E-4E7D-9113-43735F3E6BAD}.Fuzzing|Any CPU.ActiveCfg = Debug|Any CPU | ||
{1588FD7C-241E-4E7D-9113-43735F3E6BAD}.Fuzzing|ARM.ActiveCfg = Debug|Any CPU | ||
{1588FD7C-241E-4E7D-9113-43735F3E6BAD}.Fuzzing|ARM64.ActiveCfg = Fuzzing|Any CPU | ||
{1588FD7C-241E-4E7D-9113-43735F3E6BAD}.Fuzzing|DotNet_x64Test.ActiveCfg = Fuzzing|Any CPU | ||
{1588FD7C-241E-4E7D-9113-43735F3E6BAD}.Fuzzing|DotNet_x86Test.ActiveCfg = Debug|Any CPU | ||
{1588FD7C-241E-4E7D-9113-43735F3E6BAD}.Fuzzing|x64.ActiveCfg = Debug|x64 | ||
{1588FD7C-241E-4E7D-9113-43735F3E6BAD}.Fuzzing|x86.ActiveCfg = Fuzzing|x86 |
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.
another mixed|any cpu
{F75E29D0-D288-478B-8D83-2C190F321A3F}.Fuzzing|Any CPU.ActiveCfg = Release|Any CPU | ||
{F75E29D0-D288-478B-8D83-2C190F321A3F}.Fuzzing|ARM.ActiveCfg = Fuzzing|ARM | ||
{F75E29D0-D288-478B-8D83-2C190F321A3F}.Fuzzing|ARM64.ActiveCfg = Fuzzing|ARM64 | ||
{F75E29D0-D288-478B-8D83-2C190F321A3F}.Fuzzing|DotNet_x64Test.ActiveCfg = Fuzzing|Any CPU | ||
{F75E29D0-D288-478B-8D83-2C190F321A3F}.Fuzzing|DotNet_x86Test.ActiveCfg = Release|Any CPU | ||
{F75E29D0-D288-478B-8D83-2C190F321A3F}.Fuzzing|x64.ActiveCfg = Release|x64 | ||
{F75E29D0-D288-478B-8D83-2C190F321A3F}.Fuzzing|x86.ActiveCfg = Release|x86 |
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.
curiously mixed (release, fuzzing)|any cpu
So @zadjii-msft, the mixed weird tuples are because those projects are just not selected to build in the fuzzing config. The burden of fixing the mapping would be on the person who enables one of them :) anyCPU is the most annoying obviously, since it’s a CPU type that only 3 of our 3,100 projects have so we need to carry it around like an empty husk |
we should almost just add a separate sln for those 😐 |
Validation Steps PerformedI've been running conhost fuzzing for hours! Right now, the WCL fuzzer is completely stateful since we never clear/reset the buffer. It appends and appends and appends and leaks heaps of memory (for some reason...). The code in the fuzz function is written in a very "if you can find it, use it" way. I'm calling weird stuff from all over the codebase because it does what we need. I have been treating this as a magic input generator. If I am looking at WriteCharsLegacy, and I wonder "what sort of input will get me to this line of code?" I can literally set a breakpoint and half a second later I'm staring at inputs that got me there. It's freaking amazing. |
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
This commit introduces a new build configuration, "Fuzzing", which
enables the new address sanitizer (shipped in VS 16.9) and code
coverage over the entire solution. Only a small subset of projects
(those comprising original conhost, right now) are selected to build in
this configuration, and even then only in Fuzzing|x64.
It also adds a fuzzing-adapted build of conhost, which makes no server
connections and handles no client applications. To do this, I've
replicated a bit of the console startup routine into fuzzmain.cpp and
made up some fake data. This is the bare minimum required to boot up
Win32 interactivity (or VT interactivity!) and pretend that a process
has connected.
If we don't pretend that a process has connected, "conhost" will exit
immediately. If we don't forge the process list, conhost will exit. If
we can't provide a server handle, we can't provide a "device comm".
Minor changes were necessary to server/host such that they would accept
a preexisting "device comm". We use this new behavior to provide a
"null" one that only hangs up threads and otherwise responds to requests
successfully.
This fuzzing-adapted build links LLVM's libFuzzer, which is an excellent
coverage-based fuzzer that will produce a corpus of inputs that exercise
unique codepaths. Eventually, we can use this to generate known-"good"
inputs for anything.
I've gone ahead and added a fuzz function that yeets bytes directly into
WriteCharsLegacy, which was the original reason I went down this path.
The implementation of LLVMFuzzerTestOneInput should be replaced with
whatever you want to fuzz.