Fix null pointer dereference in weak_ref::get() (#1232)

microsoft · Nov 21, 2022 · 37bd17f · 37bd17f
1 parent d81ec9f
commit 37bd17f
Show file tree

Hide file tree

Showing 5 changed files with 38 additions and 13 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -278,6 +278,7 @@ jobs:
     strategy:
       matrix:
         arch: [i686, x86_64]
+        config: [Debug, Release]
     runs-on: windows-latest
     steps:
       - uses: actions/checkout@v3
@@ -290,12 +291,21 @@ jobs:
           rm llvm-mingw.zip
           if (!(Test-Path "$pwd\llvm-mingw-${llvm_mingw_version}-ucrt-${{ matrix.arch }}\bin\clang++.exe")) { return 1 }
           Add-Content $env:GITHUB_PATH "$pwd\llvm-mingw-${llvm_mingw_version}-ucrt-${{ matrix.arch }}\bin"
+          # for the ASAN runtime DLL:
+          Add-Content $env:GITHUB_PATH "$pwd\llvm-mingw-${llvm_mingw_version}-ucrt-${{ matrix.arch }}\${{ matrix.arch }}-w64-mingw32\bin"
 
       - name: Build cppwinrt
         run: |
           mkdir build
           cd build
-          cmake ../ -G"MinGW Makefiles" -DCMAKE_BUILD_TYPE=Debug -DDOWNLOAD_WINDOWSNUMERICS=TRUE
+          if ("${{ matrix.config }}" -eq "Debug") {
+            $sanitizers = "TRUE"
+          } else {
+            $sanitizers = "FALSE"
+          }
+          cmake ../ -G"MinGW Makefiles" -DCMAKE_BUILD_TYPE=${{ matrix.config }} `
+            -DDOWNLOAD_WINDOWSNUMERICS=TRUE `
+            -DENABLE_TEST_SANITIZERS=$sanitizers
           cmake --build . -j2 --target cppwinrt
 
       - name: Upload cppwinrt.exe
@@ -318,6 +328,7 @@ jobs:
       - name: Run tests
         run: |
           cd build
+          $env:UBSAN_OPTIONS = "print_stacktrace=1"
           ctest --verbose
 
   build-msvc-natvis:

diff --git a/strings/base_weak_ref.h b/strings/base_weak_ref.h
@@ -23,8 +23,11 @@ WINRT_EXPORT namespace winrt
             {
                 impl::com_ref<default_interface<T>> temp;
                 m_ref->Resolve(guid_of<T>(), put_abi(temp));
-                void* result = get_self<T>(temp);
-                detach_abi(temp);
+                void* result = nullptr;
+                if (temp) {
+                    result = get_self<T>(temp);
+                    detach_abi(temp);
+                }
                 return impl::com_ref<T>{ result, take_ownership_from_abi };
             }
             else

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
@@ -65,6 +65,13 @@ add_custom_target(build-cppwinrt-projection
 )
 
 
+set(ENABLE_TEST_SANITIZERS FALSE CACHE BOOL "Enable ASan and UBSan for the tests.")
+if(ENABLE_TEST_SANITIZERS)
+    # Disable the 'vptr' check because it seems to produce false-positives when using COM classes.
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=undefined,address -fno-sanitize=vptr")
+endif()
+
+
 add_subdirectory(test)
 add_subdirectory(test_cpp20)
 add_subdirectory(test_win7)

diff --git a/test/catch.hpp b/test/catch.hpp
@@ -10807,7 +10807,7 @@ namespace Catch {
         { static_cast<DWORD>(EXCEPTION_INT_DIVIDE_BY_ZERO), "Divide by zero error" },
     };
 
-    static LONG CALLBACK handleVectoredException(PEXCEPTION_POINTERS ExceptionInfo) {
+    static LONG CALLBACK topLevelExceptionFilter(PEXCEPTION_POINTERS ExceptionInfo) {
         for (auto const& def : signalDefs) {
             if (ExceptionInfo->ExceptionRecord->ExceptionCode == def.id) {
                 reportFatal(def.name);
@@ -10821,7 +10821,7 @@ namespace Catch {
     // Since we do not support multiple instantiations, we put these
     // into global variables and rely on cleaning them up in outlined
     // constructors/destructors
-    static PVOID exceptionHandlerHandle = nullptr;
+    static LPTOP_LEVEL_EXCEPTION_FILTER previousTopLevelExceptionFilter = nullptr;
 
     // For MSVC, we reserve part of the stack memory for handling
     // memory overflow structured exception.
@@ -10841,18 +10841,15 @@ namespace Catch {
     FatalConditionHandler::~FatalConditionHandler() = default;
 
     void FatalConditionHandler::engage_platform() {
-        // Register as first handler in current chain
-        exceptionHandlerHandle = AddVectoredExceptionHandler(1, handleVectoredException);
-        if (!exceptionHandlerHandle) {
-            CATCH_RUNTIME_ERROR("Could not register vectored exception handler");
-        }
+        // Register as a the top level exception filter.
+        previousTopLevelExceptionFilter = SetUnhandledExceptionFilter(topLevelExceptionFilter);
     }
 
     void FatalConditionHandler::disengage_platform() {
-        if (!RemoveVectoredExceptionHandler(exceptionHandlerHandle)) {
-            CATCH_RUNTIME_ERROR("Could not unregister vectored exception handler");
+        if (SetUnhandledExceptionFilter(reinterpret_cast<LPTOP_LEVEL_EXCEPTION_FILTER>(previousTopLevelExceptionFilter)) != topLevelExceptionFilter) {
+            CATCH_RUNTIME_ERROR("Could not restore previous top level exception filter");
         }
-        exceptionHandlerHandle = nullptr;
+        previousTopLevelExceptionFilter = nullptr;
     }
 
 } // end namespace Catch

diff --git a/test/test_cpp20/CMakeLists.txt b/test/test_cpp20/CMakeLists.txt
@@ -3,6 +3,13 @@ set(CMAKE_CXX_STANDARD 20)
 # are experimental in libc++ as of Clang 15.
 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fexperimental-library")
 
+if(ENABLE_TEST_SANITIZERS)
+    # As of LLVM 15, custom_error.cpp doesn't build with ASAN due to:
+    # error: cannot make section .ASAN$GL associative with sectionless symbol _ZNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEE4nposE
+    set_source_files_properties(custom_error.cpp PROPERTIES COMPILE_OPTIONS "-fno-sanitize=address")
+    set_source_files_properties(custom_error.cpp PROPERTIES SKIP_PRECOMPILE_HEADERS true)
+endif()
+
 file(GLOB TEST_SRCS
     LIST_DIRECTORIES false
     CONFIGURE_DEPENDS