search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.03k stars 11.57k forks source link

Asan error: SEH exception with code 0xc000001d thrown in the test body. #39667

Open steveire opened 5 years ago

steveire commented 5 years ago
Bugzilla Link 40320
Version trunk
OS Windows NT
CC @zygoloid,@rnk,@smeenai

Extended Description

Clang-CL can build the assimp libraries (https://github.com/assimp/assimp.git) and the tests pass.

However, when asan is enabled, several tests no longer pass.

This patch enables asan on windows for assimp:

diff --git a/CMakeLists.txt b/CMakeLists.txt index 645d9268..52e52815 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -38,6 +38,31 @@ SET(CMAKE_LEGACY_CYGWIN_WIN32 0) # Remove when CMake >= 2.8.4 is required CMAKE_MINIMUM_REQUIRED( VERSION 2.8 ) PROJECT( Assimp )

+set(LLVM_PREFIX "c:/Program Files/LLVM/lib/clang/7.0.0/lib/windows") + +add_library(clang::AddressSanitizerDynamicRuntime INTERFACE IMPORTED) +set_property(TARGET clang::AddressSanitizerDynamicRuntime PROPERTY

$ test\unit.exe --gtest_filter=.importBinaryglTF2FromFileTest Note: Google Test filter = .importBinaryglTF2FromFileTest [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from utglTF2ImportExport [ RUN ] utglTF2ImportExport.importBinaryglTF2FromFileTest unknown file: error: SEH exception with code 0xc000001d thrown in the test body. [ FAILED ] utglTF2ImportExport.importBinaryglTF2FromFileTest (3 ms) [----------] 1 test from utglTF2ImportExport (3 ms total)

[----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (5 ms total) [ PASSED ] 0 tests. [ FAILED ] 1 test, listed below: [ FAILED ] utglTF2ImportExport.importBinaryglTF2FromFileTest

In total 9 tests fail:

[ FAILED ] 9 tests, listed below: [ FAILED ] utglTF2ImportExport.importBinaryglTF2FromFileTest [ FAILED ] utIFCImportExport.importComplextypeAsColor [ FAILED ] ImporterTest.testPluginInterface [ FAILED ] utSTLImporterExporter.test_with_empty_solid [ FAILED ] utXImporterExporter.heap_overflow_in_tokenizer [ FAILED ] utObjImportExport.issue1453_segfault [ FAILED ] utObjImportExport.homogeneous_coordinates_divide_by_zero_Test [ FAILED ] utObjImportExport.0based_array_Test [ FAILED ] utMDCImportExport.importMDCFromFileTest

It is not clear what 'error: SEH exception with code' means.

steveire commented 5 years ago

Sorry, I feel that was a bit short. To state this more positively, I think it shouldn't be too hard to adapt the solution that was used for ARC to work for ASan.

Great, it looks like there is a description of a way forward now anyway.

I don't personally have a day or two to try it and test it right now, so I don't want to commit to a timeline, but hopefully I or someone else finds time to in the near future.

Cool. I will test any change with AssImp, if possible. I tried decorating lots of functions with __attribute__((no_sanitize_address)) but it's not practical to discover which ones to decorate. It's more practical at this point to just not use ASAN.

Does this mean that ASAN can't be used with projects which use exceptions generally, or is that only true on Windows/Clang-CL?

Thanks,

rnk commented 5 years ago

It is more likely that we will change the way windows EH funclets work to make it so that ASan's instrumentation is more naturally compatible with Windows EH than that we will fix ASan to deal with the current system.

Adding the funclet tokens to ASan's calls shouldn't be terrible, I think. I forget exactly what you had in mind for removing the need for the funclet token altogether, and that would definitely be much nicer, but I don't know how much work that would end up being.

I think it would be a reasonable short term fix. Designing away funclet bundle operands on call instructions is a longer term project.

smeenai commented 5 years ago

But, what's really needed is for someone to fix the bug, which is not much more work than deciding on the exact circumstances of the warning.

Sorry, I feel that was a bit short. To state this more positively, I think it shouldn't be too hard to adapt the solution that was used for ARC to work for ASan. I don't personally have a day or two to try it and test it right now, so I don't want to commit to a timeline, but hopefully I or someone else finds time to in the near future.

To be clear, the solution that was adopted for ARC was to ensure the funclet token was preserved on calls, which seems to run counter to https://github.com/google/sanitizers/issues/749#issuecomment-395813875:

It is more likely that we will change the way windows EH funclets work to make it so that ASan's instrumentation is more naturally compatible with Windows EH than that we will fix ASan to deal with the current system.

Adding the funclet tokens to ASan's calls shouldn't be terrible, I think. I forget exactly what you had in mind for removing the need for the funclet token altogether, and that would definitely be much nicer, but I don't know how much work that would end up being.

rnk commented 5 years ago

But, what's really needed is for someone to fix the bug, which is not much more work than deciding on the exact circumstances of the warning.

Sorry, I feel that was a bit short. To state this more positively, I think it shouldn't be too hard to adapt the solution that was used for ARC to work for ASan. I don't personally have a day or two to try it and test it right now, so I don't want to commit to a timeline, but hopefully I or someone else finds time to in the near future.

rnk commented 5 years ago

Is there some way for clang to emit a diagnostic indicating which function needs to get the attribute? ie, the function which contains the problematic throw?

Pedantically, yes and no. The exception could come from some part of the application not compiled by clang, so we can't be sure that we'll warn on the throwing function. At the same time, yes, we could warn if exceptions and asan are enabled, or if asan is enabled and a throw is compiled.

But, what's really needed is for someone to fix the bug, which is not much more work than deciding on the exact circumstances of the warning.

steveire commented 5 years ago

Is there some way for clang to emit a diagnostic indicating which function needs to get the attribute? ie, the function which contains the problematic throw?

steveire commented 5 years ago

Hmm, so yes, probably an instance of https://github.com/google/sanitizers/issues/749

steveire commented 5 years ago

I reduced the c++ test case to

#include <gtest/gtest.h>

struct FooObject;

struct BarObject
{
    FooObject&           mFooObject;

    BarObject(FooObject& FooObject);
};

struct FooObject
{
    char data[(8 * 45) + 1];

    BarObject   member;

    FooObject()
        : member(*this)
    {
    }
};

inline BarObject::BarObject(FooObject& FooObject)
    : mFooObject(FooObject)
{
}

class MyError : public std::runtime_error
{
public:
    explicit MyError(const std::string& errorText)
        : std::runtime_error(errorText)
    {
    }
};

TEST(Test, throwTest) {
    FooObject FooObject;
    try {
        throw MyError("Bad");
    }
    catch (...) {
    }
}

int main(int argc, char* argv[])
{
    ::testing::InitGoogleTest(&argc, argv);

    int result = RUN_ALL_TESTS();

    return result;
}

with the following CMake: 

CMAKE_MINIMUM_REQUIRED( VERSION 3.10 )
PROJECT( ASAN_test )

set(LLVM_PREFIX "c:/Program Files/LLVM/lib/clang/7.0.0/lib/windows")

add_library(clang::AddressSanitizer INTERFACE IMPORTED)

set_property(TARGET clang::AddressSanitizer PROPERTY 
  INTERFACE_COMPILE_OPTIONS -fsanitize=address
  )
set_property(TARGET clang::AddressSanitizer PROPERTY 
  INTERFACE_LINK_LIBRARIES
  "${LLVM_PREFIX}/clang_rt.asan_dynamic-x86_64.lib"
  "${LLVM_PREFIX}/clang_rt.asan_dynamic_runtime_thunk-x86_64.lib"
  "-include:__asan_seh_interceptor"
  -wholearchive:"${LLVM_PREFIX}/clang_rt.asan_dynamic_runtime_thunk-x86_64.lib"
  )

INCLUDE_DIRECTORIES(
  ${CMAKE_SOURCE_DIR}/contrib/gtest/include
  ${CMAKE_SOURCE_DIR}/contrib/gtest/
)

add_executable( unit
    contrib/gtest/src/gtest-all.cc
    Main.cpp
)
target_link_libraries(unit PRIVATE
    clang::AddressSanitizer
    )
smeenai commented 5 years ago

I believe the discussion we'd had was in the context of ARC optimizations and how a bunch of those had to be adjusted to preserve or add a funclet token, otherwise you'd silently generate an unreachable. You had some ideas on how to eliminate the need for the funclet tokens entirely (which I don't remember the details of, unfortunately), and we also discussed having an error when we run into a call without (or with an invalid) funclet token inside an EH pad. Neither of those have been implemented yet though ... for my specific case I just had to make a few one-off fixes.

rnk commented 5 years ago

Exception codes are usually NTSTATUS values, listed here: https://msdn.microsoft.com/en-us/library/cc704588.aspx I really wish that page ranked higher in search, so that I could just google 0xC000* and get the info I want... =/

In any case, this is what it means: 0xC000001D STATUS_ILLEGAL_INSTRUCTION

This usually means the program executed a UD2(A) instruction, which LLVM emits in place of unreachable on Windows.

Does this particular test exercise exception handling? If so, this is probably a symptom of this bug: https://github.com/google/sanitizers/issues/749

I remember explaining why we have the same problem with coverage instrumentation to Shoaib months ago. Did we fix it there? Is the same fix applicable to ASan instrumentation?

It could always be some other illegal instruction, though, like a noreturn function returning unexpectedly.

sylvain-audi commented 1 year ago

Hello, We had the same issue with ASan on Win EH code. The crash was due to a trap generated in machine code, due to winehprepare pass discarding eh cleanup blocks where calls without funclets were found, see WinEHPrepare::removeImplausibleInstructions

I created a patch for the solution you mentionned (adding the funclet OpBundle to ASan's calls that need it): https://reviews.llvm.org/D143108