This is an archive of the discontinued LLVM Phabricator instance.

[fuzzer] Add Windows Visual C++ exception intercept
ClosedPublic

Authored by jopletch on Oct 19 2020, 5:15 PM.

Details

Summary

Adds a new option, handle_winexcept to try to intercept uncaught
Visual C++ exceptions on Windows. On Linux, such exceptions are handled
implicitly by std::terminate() raising SIBABRT. This option brings the
Windows behavior in line with Linux.

Unfortunately this exception code is intentionally undocumented, however
has remained stable for the last decade. More information can be found
here: https://devblogs.microsoft.com/oldnewthing/20100730-00/?p=13273

Diff Detail

Event Timeline

jopletch created this revision.Oct 19 2020, 5:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 19 2020, 5:15 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
jopletch requested review of this revision.Oct 19 2020, 5:15 PM
kcc added a reviewer: metzman.Oct 19 2020, 5:18 PM

please no #ifdefs.
please add a test.

jopletch updated this revision to Diff 299230.Oct 19 2020, 5:49 PM

Removed if defs.

For the test, the following code demonstrates the issue:

extern "C" int LLVMFuzzerTestOneInput(const std::uint8_t *data, size_t size) {
    std::vector<uint8_t> v;
    v.reserve(static_cast<uint64_t>(-1));

    return 0;
}

But I'm not sure how best to integrate this -- are there existing crashing tests somewhere I should add this to?

kcc added a comment.Oct 19 2020, 6:05 PM

But I'm not sure how best to integrate this -- are there existing crashing tests somewhere I should add this to?

compiler-rt/test/fuzzer

This change was a bit hard to review since it doesn't include full context (See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

compiler-rt/lib/fuzzer/FuzzerUtilWindows.cpp
64

nit: include link to https://devblogs.microsoft.com/oldnewthing/20100730-00/?p=13273 and end sentence with period please.

69

nit: "Handle"

jopletch updated this revision to Diff 301204.Oct 28 2020, 2:20 AM

Address review comments, apologies for the delay and incorrectly formatted patch. Should have a test added shortly, but I'm running into some problems executing the test suite on Windows. Are failures expected on Windows, or is it more likely there are problems with my environment? Specifically the testing suite seems to be expecting bash-like environment. Is it expected to run ninja from wsl or cygwin?

Address review comments, apologies for the delay and incorrectly formatted patch. Should have a test added shortly, but I'm running into some problems executing the test suite on Windows. Are failures expected on Windows, or is it more likely there are problems with my environment? Specifically the testing suite seems to be expecting bash-like environment. Is it expected to run ninja from wsl or cygwin?

Have you installed GnuWin32?
These are necessary for running the tests under Windows and sound like the missing thing here.

Address review comments, apologies for the delay and incorrectly formatted patch. Should have a test added shortly, but I'm running into some problems executing the test suite on Windows. Are failures expected on Windows, or is it more likely there are problems with my environment? Specifically the testing suite seems to be expecting bash-like environment. Is it expected to run ninja from wsl or cygwin?

Overall, failures should not happen under Windows.
Most of the tests that need gnu-like tools work with gnuwin32, the ones that don't are explicitly disabled on Win.
So if you get failures running the tests under Win without making any changes first, it probably is something wrong with your environment.

jopletch updated this revision to Diff 301473.Oct 28 2020, 4:35 PM

Add test.

Address review comments, apologies for the delay and incorrectly formatted patch. Should have a test added shortly, but I'm running into some problems executing the test suite on Windows. Are failures expected on Windows, or is it more likely there are problems with my environment? Specifically the testing suite seems to be expecting bash-like environment. Is it expected to run ninja from wsl or cygwin?

Have you installed GnuWin32?
These are necessary for running the tests under Windows and sound like the missing thing here.

Yeah, this is in fact what I was missing, thanks! FWIW, the testing scripts also work with the bash-like utilities that ship with git (e.g. C:\Program Files\Git\usr\bin), which is probably more likely to be install on a developer machine these days :)

jopletch marked 2 inline comments as done.Oct 30 2020, 5:23 PM

Oops, didn't realize those comments were individually resolvable.

metzman added inline comments.Nov 2 2020, 7:45 AM
compiler-rt/test/fuzzer/uncaught-exception.test
2

Just confirming, on Windows, v.reserve(static_cast<uint64_t>(-1)); throws 0xE06D7363 ?

metzman added inline comments.Nov 2 2020, 8:05 AM
compiler-rt/test/fuzzer/uncaught-exception.test
2

I'll approve if this is the case.

jopletch marked 2 inline comments as done.Nov 2 2020, 10:27 AM

Yes, without this patch and with windbg set as the jit debugger if you run this testcase you'll see the following:

(431c.1fcc): C++ EH exception - code e06d7363 (!!! second chance !!!)
*** WARNING: Unable to verify checksum for E:\libfuzzer\llvm-project\build\projects\compiler-rt\test\fuzzer\X86_64DefaultWindowsConfig\Output\uncaught-exception.test.tmp-UncaughtException
KERNELBASE!RaiseException+0x69:
00007ff9`db093e49 0f1f440000      nop     dword ptr [rax+rax]
0:000> .exr -1
ExceptionAddress: 00007ff9db093e49 (KERNELBASE!RaiseException+0x0000000000000069)
   ExceptionCode: e06d7363 (C++ EH exception)
  ExceptionFlags: 00000001
NumberParameters: 4
   Parameter[0]: 0000000019930520
   Parameter[1]: 0000006f0c50edf0
   Parameter[2]: 00007ff7bc6f3368
   Parameter[3]: 00007ff7bc4a0000
metzman accepted this revision.Nov 12 2020, 10:55 AM

Sorry, didn't notice your response.
Do you need me to merge this for you?

This revision is now accepted and ready to land.Nov 12 2020, 10:55 AM

No problem -- yes, if you wouldn't mind that would be fantastic.

Thanks for the assistance in cleaning this up!

This revision was automatically updated to reflect the committed changes.

The test seems to fail on some Linux buildbots, see here. Please review.

Looking at these failures, they all appear to happen on platforms where programs can get ASAN allocation too large errors which doesn't include Windows or vanilla Linux x64. The fast solution is adding REQUIRE: windows to the test, but as the original intent was to bring the behavior in line with Linux we'd obviously lose the xplat testing aspect. However, I also don't think there's a REQUIRE keyword for "platform does not have an ASAN allocation size limit" (is there?), and adding one for this test seems a bit much. Happy to take guidance on what you'd prefer here.

As a procedural question, should whatever fix be added onto this changeset or a separate one?

dyung added a subscriber: dyung.Nov 13 2020, 5:29 PM

The fix would normally be put in a separate change. If it is a small change, you could probably just do a post-commit review. We would like to get this either fixed or reverted so that we can get our internal build bot green again.

Does setting ASAN_OPTIONS=allocator_may_return_null=1 help at all? I think it would cause std::bad_alloc on these platforms, which should also crash.

Does setting ASAN_OPTIONS=allocator_may_return_null=1 help at all? I think it would cause std::bad_alloc on these platforms, which should also crash.

Trying this here: https://github.com/llvm/llvm-project/commit/a3be1287091463f4099cdb1710883645329cda7e

metzman added a comment.EditedNov 16 2020, 9:45 AM

Looks like my fix broke windows.
http://lab.llvm.org:8011/#/builders/127/builds/1548
I'm going to undo my Win fix and disable it on Linux for now (https://github.com/llvm/llvm-project/commit/91703085f53428ea6305770d41db148e0ebbdea7)
I'll try again if http://lab.llvm.org:8011/#/builders/112/builds/1191 passes since I think it contains https://github.com/llvm/llvm-project/commit/a3be1287091463f4099cdb1710883645329cda7e
The problem is, I don't really know how I can set an env var in a way that works on Linux but doesn't cause failure on Win.
I have both platforms to test though

Disabling on Linux was right call as http://lab.llvm.org:8011/#/builders/112/builds/1191/steps/5/logs/stdio is failing with ASAN_OPTIONS fix.