From 644f0e98ec68f4392ced80fad9502dbf3811c75b Mon Sep 17 00:00:00 2001 From: Zoltan Martonka Date: Wed, 30 Oct 2024 11:21:16 +0100 Subject: [PATCH 1/4] Fix null terminator in Demangle function. If the demangled name is longer than out_size, the null terminator is missing from the output. This will cause a crash in the DemangleInplace() function (symbolize.cc) when calling strlen on the buffer. --- src/demangle.cc | 8 +++++++- src/symbolize_unittest.cc | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/src/demangle.cc b/src/demangle.cc index 9902c1e10..a0578e430 100644 --- a/src/demangle.cc +++ b/src/demangle.cc @@ -1350,7 +1350,13 @@ bool Demangle(const char* mangled, char* out, size_t out_size) { return false; } - std::copy_n(unmangled.get(), std::min(n, out_size), out); + if (out_size > 0) { + // n is the size of the allocated buffer, not the length of the string. + // Therefore, it includes the terminating zero (and possibly additional space). + std::size_t copy_size = std::min(n-1, out_size - 1); + std::copy_n(unmangled.get(), copy_size, out); + out[copy_size] = '\0'; // Ensure terminating null if n > out_size + } return status == 0; #else State state; diff --git a/src/symbolize_unittest.cc b/src/symbolize_unittest.cc index f07484e63..d0e3c05a4 100644 --- a/src/symbolize_unittest.cc +++ b/src/symbolize_unittest.cc @@ -34,7 +34,11 @@ #include "symbolize.h" #include +#include #include +#include +#include + #include "config.h" #include "glog/logging.h" @@ -133,6 +137,18 @@ TEST(Symbolize, Symbolize) { struct Foo { static void func(int x); + static size_t longParamFunc( + std::map,std::map > p0, + std::map,std::map > p1, + std::map,std::map > p2, + std::map,std::map > p3, + std::map,std::map > p4, + std::map,std::map > p5, + std::map,std::map > p6, + std::map,std::map > p7, + std::map,std::map > p8, + std::map,std::map > p9 + ); }; void ATTRIBUTE_NOINLINE Foo::func(int x) { @@ -142,6 +158,21 @@ void ATTRIBUTE_NOINLINE Foo::func(int x) { a = a + 1; } +size_t ATTRIBUTE_NOINLINE Foo::longParamFunc( + std::map,std::map > p0, + std::map,std::map > p1, + std::map,std::map > p2, + std::map,std::map > p3, + std::map,std::map > p4, + std::map,std::map > p5, + std::map,std::map > p6, + std::map,std::map > p7, + std::map,std::map > p8, + std::map,std::map > p9 + ) { + return p0.size() + p1.size() + p2.size() + p3.size() + p4.size() + p5.size() + p6.size() + p7.size() + p8.size() + p9.size(); +} + // With a modern GCC, Symbolize() should return demangled symbol // names. Function parameters should be omitted. # ifdef TEST_WITH_MODERN_GCC @@ -150,6 +181,10 @@ TEST(Symbolize, SymbolizeWithDemangling) { # if !defined(_MSC_VER) || !defined(NDEBUG) # if defined(HAVE___CXA_DEMANGLE) EXPECT_STREQ("Foo::func(int)", TrySymbolize((void*)(&Foo::func))); + // Very long functions can be truncated, but we should not crash or return null. + // Also the result should start properly. + const char* symbol = TrySymbolize((void*)(&Foo::longParamFunc)); + EXPECT_TRUE(symbol == std::strstr(symbol, "Foo::longParamFunc(")); # else EXPECT_STREQ("Foo::func()", TrySymbolize((void*)(&Foo::func))); # endif From 93922d5b832c6ef94d220b2ce0b24db1a73cbb3e Mon Sep 17 00:00:00 2001 From: Sergiu Deitsch Date: Thu, 31 Oct 2024 16:24:32 +0100 Subject: [PATCH 2/4] format --- src/demangle.cc | 7 +-- src/symbolize_unittest.cc | 95 +++++++++++++++++++++++++++------------ 2 files changed, 70 insertions(+), 32 deletions(-) diff --git a/src/demangle.cc b/src/demangle.cc index a0578e430..8e5189d0e 100644 --- a/src/demangle.cc +++ b/src/demangle.cc @@ -1352,10 +1352,11 @@ bool Demangle(const char* mangled, char* out, size_t out_size) { if (out_size > 0) { // n is the size of the allocated buffer, not the length of the string. - // Therefore, it includes the terminating zero (and possibly additional space). - std::size_t copy_size = std::min(n-1, out_size - 1); + // Therefore, it includes the terminating zero (and possibly additional + // space). + std::size_t copy_size = std::min(n - 1, out_size - 1); std::copy_n(unmangled.get(), copy_size, out); - out[copy_size] = '\0'; // Ensure terminating null if n > out_size + out[copy_size] = '\0'; // Ensure terminating null if n > out_size } return status == 0; #else diff --git a/src/symbolize_unittest.cc b/src/symbolize_unittest.cc index d0e3c05a4..ac04d4e91 100644 --- a/src/symbolize_unittest.cc +++ b/src/symbolize_unittest.cc @@ -39,12 +39,11 @@ #include #include - #include "config.h" #include "glog/logging.h" #include "googletest.h" -#include "utilities.h" #include "stacktrace.h" +#include "utilities.h" #ifdef GLOG_USE_GFLAGS # include @@ -137,18 +136,36 @@ TEST(Symbolize, Symbolize) { struct Foo { static void func(int x); - static size_t longParamFunc( - std::map,std::map > p0, - std::map,std::map > p1, - std::map,std::map > p2, - std::map,std::map > p3, - std::map,std::map > p4, - std::map,std::map > p5, - std::map,std::map > p6, - std::map,std::map > p7, - std::map,std::map > p8, - std::map,std::map > p9 - ); + static size_t longParamFunc(std::map, + std::map> + p0, + std::map, + std::map> + p1, + std::map, + std::map> + p2, + std::map, + std::map> + p3, + std::map, + std::map> + p4, + std::map, + std::map> + p5, + std::map, + std::map> + p6, + std::map, + std::map> + p7, + std::map, + std::map> + p8, + std::map, + std::map> + p9); }; void ATTRIBUTE_NOINLINE Foo::func(int x) { @@ -158,19 +175,39 @@ void ATTRIBUTE_NOINLINE Foo::func(int x) { a = a + 1; } -size_t ATTRIBUTE_NOINLINE Foo::longParamFunc( - std::map,std::map > p0, - std::map,std::map > p1, - std::map,std::map > p2, - std::map,std::map > p3, - std::map,std::map > p4, - std::map,std::map > p5, - std::map,std::map > p6, - std::map,std::map > p7, - std::map,std::map > p8, - std::map,std::map > p9 - ) { - return p0.size() + p1.size() + p2.size() + p3.size() + p4.size() + p5.size() + p6.size() + p7.size() + p8.size() + p9.size(); +size_t ATTRIBUTE_NOINLINE +Foo::longParamFunc(std::map, + std::map> + p0, + std::map, + std::map> + p1, + std::map, + std::map> + p2, + std::map, + std::map> + p3, + std::map, + std::map> + p4, + std::map, + std::map> + p5, + std::map, + std::map> + p6, + std::map, + std::map> + p7, + std::map, + std::map> + p8, + std::map, + std::map> + p9) { + return p0.size() + p1.size() + p2.size() + p3.size() + p4.size() + p5.size() + + p6.size() + p7.size() + p8.size() + p9.size(); } // With a modern GCC, Symbolize() should return demangled symbol @@ -181,8 +218,8 @@ TEST(Symbolize, SymbolizeWithDemangling) { # if !defined(_MSC_VER) || !defined(NDEBUG) # if defined(HAVE___CXA_DEMANGLE) EXPECT_STREQ("Foo::func(int)", TrySymbolize((void*)(&Foo::func))); - // Very long functions can be truncated, but we should not crash or return null. - // Also the result should start properly. + // Very long functions can be truncated, but we should not crash or return + // null. Also the result should start properly. const char* symbol = TrySymbolize((void*)(&Foo::longParamFunc)); EXPECT_TRUE(symbol == std::strstr(symbol, "Foo::longParamFunc(")); # else From 5fd3750f165b78f2340fc9587cc6cec08ff68694 Mon Sep 17 00:00:00 2001 From: Sergiu Deitsch Date: Fri, 1 Nov 2024 14:05:00 +0100 Subject: [PATCH 3/4] assert and simplify --- src/demangle.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/demangle.cc b/src/demangle.cc index 8e5189d0e..8f4db36fa 100644 --- a/src/demangle.cc +++ b/src/demangle.cc @@ -37,6 +37,7 @@ #include "demangle.h" #include +#include #include #include @@ -1351,10 +1352,11 @@ bool Demangle(const char* mangled, char* out, size_t out_size) { } if (out_size > 0) { + assert(n > 1); // n is the size of the allocated buffer, not the length of the string. // Therefore, it includes the terminating zero (and possibly additional // space). - std::size_t copy_size = std::min(n - 1, out_size - 1); + std::size_t copy_size = std::min(n, out_size) - 1; std::copy_n(unmangled.get(), copy_size, out); out[copy_size] = '\0'; // Ensure terminating null if n > out_size } From f57187842a752d6ea339c3daaf2c8a42ed5e7822 Mon Sep 17 00:00:00 2001 From: Sergiu Deitsch Date: Fri, 1 Nov 2024 15:03:36 +0100 Subject: [PATCH 4/4] rework unit tests --- CMakeLists.txt | 4 ---- src/demangle_unittest.cc | 19 +++++++++++++++---- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 59bac783e..c3fc1a238 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -586,10 +586,6 @@ if (BUILD_TESTING) add_test (NAME demangle COMMAND demangle_unittest) - if (HAVE___CXA_DEMANGLE) - # Demangle tests use a different (reduced) representation of symbols - set_tests_properties (demangle PROPERTIES DISABLED ON) - endif (HAVE___CXA_DEMANGLE) if (HAVE_STACKTRACE) add_executable (stacktrace_unittest diff --git a/src/demangle_unittest.cc b/src/demangle_unittest.cc index a85166463..2dcbd34fe 100644 --- a/src/demangle_unittest.cc +++ b/src/demangle_unittest.cc @@ -89,12 +89,10 @@ TEST(Demangle, CornerCases) { EXPECT_STREQ(demangled, tmp); EXPECT_TRUE(Demangle(mangled, tmp, size - 1)); EXPECT_STREQ(demangled, tmp); - EXPECT_FALSE(Demangle(mangled, tmp, size - 2)); // Not enough. - EXPECT_FALSE(Demangle(mangled, tmp, 1)); - EXPECT_FALSE(Demangle(mangled, tmp, 0)); - EXPECT_FALSE(Demangle(mangled, nullptr, 0)); // Should not cause SEGV. } +// Demangle tests use a different (reduced) representation of symbols +# if !defined(HAVE___CXA_DEMANGLE) // Test handling of functions suffixed with .clone.N, which is used by GCC // 4.5.x, and .constprop.N and .isra.N, which are used by GCC 4.6.x. These // suffixes are used to indicate functions which have been cloned during @@ -141,6 +139,19 @@ TEST(Demangle, FromFile) { EXPECT_EQ(demangled, DemangleIt(mangled.c_str())); } } +# endif // defined(HAVE___CXA_DEMANGLE) + +TEST(Demangle, SmallBuffer) { + constexpr std::size_t size = 10; + char tmp[size]; + const char* const mangled = "_Z6foobarv"; + EXPECT_FALSE(Demangle(mangled, tmp, size - 2)); // Not enough. + EXPECT_FALSE(Demangle(mangled, tmp, 1)); + EXPECT_FALSE(Demangle(mangled, tmp, 0)); + EXPECT_FALSE(Demangle(mangled, nullptr, 0)); // Should not cause SEGV. + EXPECT_FALSE(Demangle("", tmp, sizeof(tmp))); + EXPECT_TRUE(Demangle("_Z3foo", nullptr, 0)); +} #endif