From 76098326064dd05e08078621ba442a605ec0caf3 Mon Sep 17 00:00:00 2001 From: Robert Schulze Date: Fri, 28 Apr 2023 17:00:58 +0000 Subject: [PATCH 1/3] Unpoison stack frame ptrs from libunwind for msan Turned out that in msan builds, stack frame pointers explicitly returned by unw_backtrace() in Common/StackTrace.cpp were unpoisened, but stack frame pointers returned from the same function under the STD_EXCEPTION_HAS_STACK_TRACE define in std::exception (see (*)) were not. I tried unpoisoning directly in libcxx. Since this didn't work, this PR unpoisons where get_stack_trace_frames() is called. I don't know why this issue never occurred in CI but it fixes #48385 for me (where msan complains about reads of uninitialized data when query cache squashes and compresses chunks produced from SELECT last_error_trace FROM system.errors). Fixes: #48385 (*) https://github.com/ClickHouse/llvm-project/pull/7/files#diff-8c81d74ef72967be2fa27a31275000aa18532a583490f9673c62d7b981a5b39c --- src/Common/Exception.cpp | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/Common/Exception.cpp b/src/Common/Exception.cpp index 7e7ccfa4877..f126235b0da 100644 --- a/src/Common/Exception.cpp +++ b/src/Common/Exception.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -96,7 +97,10 @@ Exception::Exception(CreateFromPocoTag, const Poco::Exception & exc) : Poco::Exception(exc.displayText(), ErrorCodes::POCO_EXCEPTION) { #ifdef STD_EXCEPTION_HAS_STACK_TRACE - set_stack_trace(exc.get_stack_trace_frames(), exc.get_stack_trace_size()); + auto stack_trace_frames = exc.get_stack_trace_frames(); + auto stack_trace_size = exc.get_stack_trace_size(); + __msan_unpoison(stack_trace_frames, stack_trace_size * sizeof(stack_trace_frames[0])); + set_stack_trace(stack_trace_frames, stack_trace_size); #endif } @@ -104,7 +108,10 @@ Exception::Exception(CreateFromSTDTag, const std::exception & exc) : Poco::Exception(demangle(typeid(exc).name()) + ": " + String(exc.what()), ErrorCodes::STD_EXCEPTION) { #ifdef STD_EXCEPTION_HAS_STACK_TRACE - set_stack_trace(exc.get_stack_trace_frames(), exc.get_stack_trace_size()); + auto stack_trace_frames = exc.get_stack_trace_frames(); + auto stack_trace_size = exc.get_stack_trace_size(); + __msan_unpoison(stack_trace_frames, stack_trace_size * sizeof(stack_trace_frames[0])); + set_stack_trace(stack_trace_frames, stack_trace_size); #endif } @@ -112,7 +119,10 @@ Exception::Exception(CreateFromSTDTag, const std::exception & exc) std::string getExceptionStackTraceString(const std::exception & e) { #ifdef STD_EXCEPTION_HAS_STACK_TRACE - return StackTrace::toString(e.get_stack_trace_frames(), 0, e.get_stack_trace_size()); + auto stack_trace_frames = e.get_stack_trace_frames(); + auto stack_trace_size = e.get_stack_trace_size(); + __msan_unpoison(stack_trace_frames, stack_trace_size * sizeof(stack_trace_frames[0])); + return StackTrace::toString(stack_trace_frames, 0, stack_trace_size); #else if (const auto * db_exception = dynamic_cast(&e)) return db_exception->getStackTraceString(); @@ -140,7 +150,10 @@ std::string getExceptionStackTraceString(std::exception_ptr e) std::string Exception::getStackTraceString() const { #ifdef STD_EXCEPTION_HAS_STACK_TRACE - return StackTrace::toString(get_stack_trace_frames(), 0, get_stack_trace_size()); + auto stack_trace_frames = e.get_stack_trace_frames(); + auto stack_trace_size = e.get_stack_trace_size(); + __msan_unpoison(stack_trace_frames, stack_trace_size * sizeof(stack_trace_frames[0])); + return StackTrace::toString(stack_trace_frames, 0, stack_trace_size); #else return trace.toString(); #endif @@ -156,6 +169,7 @@ Exception::FramePointers Exception::getStackFramePointers() const { frame_pointers[i] = get_stack_trace_frames()[i]; } + __msan_unpoison(frame_pointers.data(), frame_pointers.size() * sizeof(frame_pointers[0])); } #else { From 3d467febe0de1bf9a2b168bcf64e189f56a7f3a8 Mon Sep 17 00:00:00 2001 From: Robert Schulze Date: Mon, 1 May 2023 20:58:10 +0000 Subject: [PATCH 2/3] Fix build --- src/Common/Exception.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Common/Exception.cpp b/src/Common/Exception.cpp index f126235b0da..df4e906005c 100644 --- a/src/Common/Exception.cpp +++ b/src/Common/Exception.cpp @@ -150,8 +150,8 @@ std::string getExceptionStackTraceString(std::exception_ptr e) std::string Exception::getStackTraceString() const { #ifdef STD_EXCEPTION_HAS_STACK_TRACE - auto stack_trace_frames = e.get_stack_trace_frames(); - auto stack_trace_size = e.get_stack_trace_size(); + auto stack_trace_frames = get_stack_trace_frames(); + auto stack_trace_size = get_stack_trace_size(); __msan_unpoison(stack_trace_frames, stack_trace_size * sizeof(stack_trace_frames[0])); return StackTrace::toString(stack_trace_frames, 0, stack_trace_size); #else From ee49f8f0facedf819c6af9b65f711561e0a72b81 Mon Sep 17 00:00:00 2001 From: Robert Schulze Date: Tue, 2 May 2023 13:41:38 +0000 Subject: [PATCH 3/3] Fix clang-tidy build --- src/Common/Exception.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Common/Exception.cpp b/src/Common/Exception.cpp index df4e906005c..20206b76225 100644 --- a/src/Common/Exception.cpp +++ b/src/Common/Exception.cpp @@ -97,7 +97,7 @@ Exception::Exception(CreateFromPocoTag, const Poco::Exception & exc) : Poco::Exception(exc.displayText(), ErrorCodes::POCO_EXCEPTION) { #ifdef STD_EXCEPTION_HAS_STACK_TRACE - auto stack_trace_frames = exc.get_stack_trace_frames(); + auto * stack_trace_frames = exc.get_stack_trace_frames(); auto stack_trace_size = exc.get_stack_trace_size(); __msan_unpoison(stack_trace_frames, stack_trace_size * sizeof(stack_trace_frames[0])); set_stack_trace(stack_trace_frames, stack_trace_size); @@ -108,7 +108,7 @@ Exception::Exception(CreateFromSTDTag, const std::exception & exc) : Poco::Exception(demangle(typeid(exc).name()) + ": " + String(exc.what()), ErrorCodes::STD_EXCEPTION) { #ifdef STD_EXCEPTION_HAS_STACK_TRACE - auto stack_trace_frames = exc.get_stack_trace_frames(); + auto * stack_trace_frames = exc.get_stack_trace_frames(); auto stack_trace_size = exc.get_stack_trace_size(); __msan_unpoison(stack_trace_frames, stack_trace_size * sizeof(stack_trace_frames[0])); set_stack_trace(stack_trace_frames, stack_trace_size); @@ -119,7 +119,7 @@ Exception::Exception(CreateFromSTDTag, const std::exception & exc) std::string getExceptionStackTraceString(const std::exception & e) { #ifdef STD_EXCEPTION_HAS_STACK_TRACE - auto stack_trace_frames = e.get_stack_trace_frames(); + auto * stack_trace_frames = e.get_stack_trace_frames(); auto stack_trace_size = e.get_stack_trace_size(); __msan_unpoison(stack_trace_frames, stack_trace_size * sizeof(stack_trace_frames[0])); return StackTrace::toString(stack_trace_frames, 0, stack_trace_size); @@ -150,7 +150,7 @@ std::string getExceptionStackTraceString(std::exception_ptr e) std::string Exception::getStackTraceString() const { #ifdef STD_EXCEPTION_HAS_STACK_TRACE - auto stack_trace_frames = get_stack_trace_frames(); + auto * stack_trace_frames = get_stack_trace_frames(); auto stack_trace_size = get_stack_trace_size(); __msan_unpoison(stack_trace_frames, stack_trace_size * sizeof(stack_trace_frames[0])); return StackTrace::toString(stack_trace_frames, 0, stack_trace_size);