Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix null terminator in Demangle function. #1137

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix null terminator in Demangle function. #1137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
644f0e9
Fix null terminator in Demangle function.
martonka Oct 30, 2024
93922d5
format
sergiud Oct 31, 2024
5fd3750
assert and simplify
sergiud Nov 1, 2024
f571878
rework unit tests
sergiud Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 10 additions & 1 deletion src/demangle.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#include "demangle.h"

#include <algorithm>
#include <cassert>
#include <cstdlib>
#include <limits>

Expand Down Expand Up @@ -1350,7 +1351,15 @@ 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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return false early if n > out_size.

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, out_size) - 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the checks are not necessary after all.

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;
Expand Down
19 changes: 15 additions & 4 deletions src/demangle_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
74 changes: 73 additions & 1 deletion src/symbolize_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,16 @@
#include "symbolize.h"

#include <csignal>
#include <cstring>
#include <iostream>
#include <map>
#include <string>

#include "config.h"
#include "glog/logging.h"
#include "googletest.h"
#include "utilities.h"
#include "stacktrace.h"
#include "utilities.h"

#ifdef GLOG_USE_GFLAGS
# include <gflags/gflags.h>
Expand Down Expand Up @@ -133,6 +136,36 @@ TEST(Symbolize, Symbolize) {

struct Foo {
static void func(int x);
static size_t longParamFunc(std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p0,
std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p1,
std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p2,
std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p3,
std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p4,
std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p5,
std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p6,
std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p7,
std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p8,
std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p9);
};

void ATTRIBUTE_NOINLINE Foo::func(int x) {
Expand All @@ -142,6 +175,41 @@ void ATTRIBUTE_NOINLINE Foo::func(int x) {
a = a + 1;
}

size_t ATTRIBUTE_NOINLINE
Foo::longParamFunc(std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p0,
std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p1,
std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p2,
std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p3,
std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p4,
std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p5,
std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p6,
std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p7,
std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
p8,
std::map<std::map<std::string, std::string>,
std::map<std::string, std::string>>
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
Expand All @@ -150,6 +218,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
Expand Down
Loading