From 9865634c81faa22a9e55ecf4b715ea1d2b4de938 Mon Sep 17 00:00:00 2001 From: Paul Fournillon Date: Mon, 9 Nov 2026 22:27:49 +0100 Subject: [PATCH 01/15] fix: read ELF symbol entries through aligned copies (PROF-16551) Symbol-table addresses come from untrusted sh_offset / DT_SYMTAB values with no alignment guarantee; casting them to ElfSymbol* and dereferencing is undefined behavior and aborts the ASan+UBSan gtest build at symbols_linux.cpp:974. Copy entries (or just st_name) into aligned locals instead. Adds an elfparser_ut regression that places a .symtab at a misaligned file offset and asserts both symbols and the PLT relocation name resolve. --- ddprof-lib/src/main/cpp/symbols_linux.cpp | 51 +++++++---- ddprof-lib/src/test/cpp/elfparser_ut.cpp | 102 +++++++++++++++++++++- 2 files changed, 131 insertions(+), 22 deletions(-) diff --git a/ddprof-lib/src/main/cpp/symbols_linux.cpp b/ddprof-lib/src/main/cpp/symbols_linux.cpp index 6318605e47..1df7467032 100644 --- a/ddprof-lib/src/main/cpp/symbols_linux.cpp +++ b/ddprof-lib/src/main/cpp/symbols_linux.cpp @@ -322,6 +322,21 @@ typedef Elf64_Dyn ElfDyn; static char _debuginfod_cache_buf[PATH_MAX] = {0}; +// Symbol-table addresses (file sh_offset or DT_SYMTAB) carry no alignment +// guarantee; these copies avoid member access through a misaligned ElfSymbol*, +// which is undefined behavior under -fsanitize=alignment. +static uint32_t symbolNameAt(const void* sym) { + uint32_t name; + memcpy(&name, sym, sizeof(name)); // st_name sits at offset 0 of Elf64_Sym + return name; +} + +static ElfSymbol readSymbol(const void* sym) { + ElfSymbol copy; + memcpy(©, sym, sizeof(copy)); + return copy; +} + class ElfParser { private: CodeCache* _cc; @@ -684,9 +699,9 @@ void ElfParser::parseDynamicSection() { // Parse .rela.plt table for (size_t offs = 0; offs < pltrelsz; offs += relent) { ElfRelocation* r = (ElfRelocation*)(jmprel + offs); - ElfSymbol* sym = (ElfSymbol*)(symtab + ELF_R_SYM(r->r_info) * syment); - if (sym->st_name != 0) { - const char* sym_name = strAt(strtab, strsz, sym->st_name); + uint32_t st_name = symbolNameAt(symtab + ELF_R_SYM(r->r_info) * syment); + if (st_name != 0) { + const char* sym_name = strAt(strtab, strsz, st_name); if (sym_name != NULL) { _cc->addImport((void**)(base_addr + r->r_offset), sym_name); } @@ -701,9 +716,9 @@ void ElfParser::parseDynamicSection() { for (size_t offs = relcount * relent; offs < relsz; offs += relent) { ElfRelocation* r = (ElfRelocation*)(rel + offs); if (ELF_R_TYPE(r->r_info) == R_GLOB_DAT || ELF_R_TYPE(r->r_info) == R_ABS64) { - ElfSymbol* sym = (ElfSymbol*)(symtab + ELF_R_SYM(r->r_info) * syment); - if (sym->st_name != 0) { - const char* sym_name = strAt(strtab, strsz, sym->st_name); + uint32_t st_name = symbolNameAt(symtab + ELF_R_SYM(r->r_info) * syment); + if (st_name != 0) { + const char* sym_name = strAt(strtab, strsz, st_name); if (sym_name != NULL) { _cc->addImport((void**)(base_addr + r->r_offset), sym_name); } @@ -970,22 +985,22 @@ void ElfParser::loadSymbolTable(const char* symbols, size_t total_size, size_t e // to a small pointer that still compares <= end, walking off the image. The // `ent_size <= total_size - off` form keeps off <= total_size with no overflow. for (size_t off = 0; ent_size <= total_size - off; off += ent_size) { - ElfSymbol* sym = (ElfSymbol*)(symbols + off); - if (sym->st_name != 0 && sym->st_value != 0) { + ElfSymbol sym = readSymbol(symbols + off); + if (sym.st_name != 0 && sym.st_value != 0) { // Resolve the name through the bounded string table; a bad st_name // offset (or unterminated string) drops the symbol instead of reading // out of bounds. - const char* sym_name = strAt(strings, strings_size, sym->st_name); + const char* sym_name = strAt(strings, strings_size, sym.st_name); if (sym_name == NULL) { continue; } // Skip special AArch64 mapping symbols: $x and $d - if (sym->st_size != 0 || sym->st_info != 0 || sym_name[0] != '$') { + if (sym.st_size != 0 || sym.st_info != 0 || sym_name[0] != '$') { const char* addr; if (base != NULL) { - // Check for overflow when adding sym->st_value to base + // Check for overflow when adding sym.st_value to base uintptr_t base_addr = (uintptr_t)base; - uint64_t symbol_value = sym->st_value; + uint64_t symbol_value = sym.st_value; // Skip this symbol if addition would overflow // First check if symbol_value exceeds the address space @@ -1000,9 +1015,9 @@ void ElfParser::loadSymbolTable(const char* symbols, size_t total_size, size_t e // Perform addition using integer arithmetic to avoid pointer overflow addr = (const char*)(base_addr + (uintptr_t)symbol_value); } else { - addr = (const char*)sym->st_value; + addr = (const char*)sym.st_value; } - _cc->add(addr, (int)sym->st_size, sym_name); + _cc->add(addr, (int)sym.st_size, sym_name); } } } @@ -1043,13 +1058,13 @@ void ElfParser::addRelocationSymbols(ElfSection* reltab, const char* plt) { if (ELF_R_SYM(r->r_info) > max_sym_index) { continue; } - ElfSymbol* sym = (ElfSymbol*)(symbols + (size_t)ELF_R_SYM(r->r_info) * sym_ent); + ElfSymbol sym = readSymbol(symbols + (size_t)ELF_R_SYM(r->r_info) * sym_ent); char name[256]; - if (sym->st_name == 0) { + if (sym.st_name == 0) { strcpy(name, "@plt"); } else { - const char* sym_name = strAt(strings, strings_size, sym->st_name); + const char* sym_name = strAt(strings, strings_size, sym.st_name); if (sym_name == NULL) { continue; // plt advances via the for-increment } @@ -1348,7 +1363,7 @@ bool hashSymbolRelocations(const LoadedImage& image, uintptr_t rel, size_t size, if (index == 0) continue; // e.g. R_*_RELATIVE: no import uintptr_t sym = symtab + index * syment; if (!image.readable(sym, sizeof(ElfSymbol))) return false; - size_t name = ((const ElfSymbol*)sym)->st_name; + size_t name = symbolNameAt((const void*)sym); if (name >= strsz) return false; const char* str = (const char*)(strtab + name); hash = fnv1a(&r->r_offset, sizeof(r->r_offset), hash); diff --git a/ddprof-lib/src/test/cpp/elfparser_ut.cpp b/ddprof-lib/src/test/cpp/elfparser_ut.cpp index c0e7cf5842..18cddefb19 100644 --- a/ddprof-lib/src/test/cpp/elfparser_ut.cpp +++ b/ddprof-lib/src/test/cpp/elfparser_ut.cpp @@ -1,3 +1,8 @@ +/* + * Copyright 2026, Datadog, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + #ifdef __linux__ #include @@ -396,8 +401,9 @@ Elf64_Ehdr validEhdr() { } // Write the bytes to a unique temp file and run ElfParser::parseFile over it, -// mirroring how Symbols::parseLibraries() parses an on-disk library. -void parseElfBytes(const std::vector& bytes) { +// mirroring how Symbols::parseLibraries() parses an on-disk library. The +// caller-visible CodeCache lets tests assert what the parser resolved. +void parseElfBytes(const std::vector& bytes, CodeCache* cc, bool use_debug) { char path[] = "/tmp/elf_regress_XXXXXX"; int fd = mkstemp(path); ASSERT_NE(fd, -1); @@ -408,11 +414,15 @@ void parseElfBytes(const std::vector& bytes) { FAIL() << "short write to " << path; return; } - CodeCache cc("regress"); - ElfParser::parseFile(&cc, nullptr, path, /*use_debug=*/false); + ElfParser::parseFile(cc, nullptr, path, use_debug); unlink(path); } +void parseElfBytes(const std::vector& bytes) { + CodeCache cc("regress"); + parseElfBytes(bytes, &cc, /*use_debug=*/false); +} + } // namespace // Regression test for the build-id parser hardening (found by fuzz_elf). @@ -573,4 +583,88 @@ TEST_F(ElfTest, symbolNameOffsetOutOfBounds) { parseElfBytes(b); // must not crash: strAt() rejects st_name == strtab_size } +// A .symtab whose sh_offset is not a multiple of alignof(Elf64_Sym) (the +// 42-byte section-header string table lands it at 490) drives the misaligned +// entries through loadSymbolTable() and addRelocationSymbols(): both symbols +// and the PLT relocation name must still resolve. Before the alignment fix +// UBSan aborted here at the first ElfSymbol member access. +TEST_F(ElfTest, symbolTableMisalignedEntries) { + const uint16_t NSEC = 6; + const uint64_t shoff = sizeof(Elf64_Ehdr); + const uint64_t shstr_off = shoff + NSEC * sizeof(Elf64_Shdr); + const char shstrtab[] = "\0.symtab\0.strtab\0.shstrtab\0.plt\0.rela.plt"; + const uint64_t sym_off = shstr_off + sizeof(shstrtab); + ASSERT_NE(sym_off % alignof(Elf64_Sym), 0u); // layout must stay misaligned + + Elf64_Sym sym[2]; + memset(sym, 0, sizeof(sym)); + sym[0].st_name = 1; // "mis_main" + sym[0].st_value = 0x20000; + sym[0].st_size = 4; + sym[1].st_name = 10; // "mis_helper" + sym[1].st_value = 0x21000; + sym[1].st_size = 4; + const uint64_t str_off = sym_off + sizeof(sym); + const char strtab[] = "\0mis_main\0mis_helper\0"; // 22 bytes: names at 1 and 10 + const uint64_t rela_off = str_off + sizeof(strtab); + Elf64_Rela rela; + memset(&rela, 0, sizeof(rela)); + rela.r_offset = 0x4000; + rela.r_info = ELF64_R_INFO(1, 1026); // JUMP_SLOT-like; type is not interpreted + + Elf64_Ehdr e = validEhdr(); + e.e_shoff = shoff; + e.e_shentsize = sizeof(Elf64_Shdr); + e.e_shnum = NSEC; + e.e_shstrndx = 3; + + Elf64_Shdr sh[6]; + memset(sh, 0, sizeof(sh)); + sh[1].sh_name = 1; // ".symtab" + sh[1].sh_type = SHT_SYMTAB; + sh[1].sh_offset = sym_off; + sh[1].sh_size = sizeof(sym); + sh[1].sh_link = 2; + sh[1].sh_entsize = sizeof(Elf64_Sym); + sh[2].sh_name = 9; // ".strtab" + sh[2].sh_type = SHT_STRTAB; + sh[2].sh_offset = str_off; + sh[2].sh_size = sizeof(strtab); + sh[3].sh_name = 17; // ".shstrtab" + sh[3].sh_type = SHT_STRTAB; + sh[3].sh_offset = shstr_off; + sh[3].sh_size = sizeof(shstrtab); + sh[4].sh_name = 27; // ".plt" + sh[4].sh_type = SHT_PROGBITS; + sh[4].sh_addr = 0x3000; + sh[4].sh_size = 32; + sh[5].sh_name = 32; // ".rela.plt" + sh[5].sh_type = SHT_RELA; + sh[5].sh_offset = rela_off; + sh[5].sh_size = sizeof(rela); + sh[5].sh_link = 1; + sh[5].sh_entsize = sizeof(Elf64_Rela); + + std::vector b; + auto app = [&](const void* p, size_t n) { + const char* c = static_cast(p); + b.insert(b.end(), c, c + n); + }; + app(&e, sizeof(e)); + app(sh, sizeof(sh)); + app(shstrtab, sizeof(shstrtab)); + app(sym, sizeof(sym)); + app(strtab, sizeof(strtab)); + app(&rela, sizeof(rela)); + + CodeCache cc("regress"); + parseElfBytes(b, &cc, /*use_debug=*/true); + // Both misaligned .symtab entries resolved at their st_value addresses. + EXPECT_EQ(cc.findSymbol("mis_main"), (const void*)0x20000); + EXPECT_EQ(cc.findSymbol("mis_helper"), (const void*)0x21000); + // The PLT name came from the misaligned symbol-index lookup in + // addRelocationSymbols(): symbol 1 resolves to "mis_helper@plt". + EXPECT_NE(cc.findSymbol("mis_helper@plt"), nullptr); +} + #endif //__linux__ From 35bf983364935dbc79a4add5bdcca181d44996a2 Mon Sep 17 00:00:00 2001 From: Paul Fournillon Date: Mon, 9 Nov 2026 23:17:02 +0100 Subject: [PATCH 02/15] Address review findings on the ELF symbol entry copies (PROF-16551) State that the misaligned member access is unconditionally undefined behavior (the sanitizer flag only detects it), rename readSymbol to readSymbolEntry to avoid the VMStructs::readSymbol grep collision, and use reinterpret_cast instead of a C-style cast in hashSymbolRelocations. --- ddprof-lib/src/main/cpp/symbols_linux.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ddprof-lib/src/main/cpp/symbols_linux.cpp b/ddprof-lib/src/main/cpp/symbols_linux.cpp index 1df7467032..f6683900e5 100644 --- a/ddprof-lib/src/main/cpp/symbols_linux.cpp +++ b/ddprof-lib/src/main/cpp/symbols_linux.cpp @@ -324,14 +324,14 @@ static char _debuginfod_cache_buf[PATH_MAX] = {0}; // Symbol-table addresses (file sh_offset or DT_SYMTAB) carry no alignment // guarantee; these copies avoid member access through a misaligned ElfSymbol*, -// which is undefined behavior under -fsanitize=alignment. +// which is undefined behavior; UBSan's alignment check detects it. static uint32_t symbolNameAt(const void* sym) { uint32_t name; memcpy(&name, sym, sizeof(name)); // st_name sits at offset 0 of Elf64_Sym return name; } -static ElfSymbol readSymbol(const void* sym) { +static ElfSymbol readSymbolEntry(const void* sym) { ElfSymbol copy; memcpy(©, sym, sizeof(copy)); return copy; @@ -985,7 +985,7 @@ void ElfParser::loadSymbolTable(const char* symbols, size_t total_size, size_t e // to a small pointer that still compares <= end, walking off the image. The // `ent_size <= total_size - off` form keeps off <= total_size with no overflow. for (size_t off = 0; ent_size <= total_size - off; off += ent_size) { - ElfSymbol sym = readSymbol(symbols + off); + ElfSymbol sym = readSymbolEntry(symbols + off); if (sym.st_name != 0 && sym.st_value != 0) { // Resolve the name through the bounded string table; a bad st_name // offset (or unterminated string) drops the symbol instead of reading @@ -1058,7 +1058,7 @@ void ElfParser::addRelocationSymbols(ElfSection* reltab, const char* plt) { if (ELF_R_SYM(r->r_info) > max_sym_index) { continue; } - ElfSymbol sym = readSymbol(symbols + (size_t)ELF_R_SYM(r->r_info) * sym_ent); + ElfSymbol sym = readSymbolEntry(symbols + (size_t)ELF_R_SYM(r->r_info) * sym_ent); char name[256]; if (sym.st_name == 0) { @@ -1363,7 +1363,7 @@ bool hashSymbolRelocations(const LoadedImage& image, uintptr_t rel, size_t size, if (index == 0) continue; // e.g. R_*_RELATIVE: no import uintptr_t sym = symtab + index * syment; if (!image.readable(sym, sizeof(ElfSymbol))) return false; - size_t name = symbolNameAt((const void*)sym); + size_t name = symbolNameAt(reinterpret_cast(sym)); if (name >= strsz) return false; const char* str = (const char*)(strtab + name); hash = fnv1a(&r->r_offset, sizeof(r->r_offset), hash); From 7592f6fe0cfd1be79f2e9dbf18903ada14c54a58 Mon Sep 17 00:00:00 2001 From: Paul Fournillon Date: Mon, 9 Nov 2026 23:17:04 +0100 Subject: [PATCH 03/15] Make elfparser_ut run and stay clean under sanitizer CI (PROF-16551) Resource-dependent tests called exit(0)/exit(1) when the cwd-relative native-libs paths did not resolve, so the whole binary exited before any test ran from the project-root cwd CI uses; replace the early exits with GTEST_SKIP()/FAIL() so the resource-independent tests always execute. Free the CodeCaches that parseLibraries publishes into the tests' stack CodeCacheArrays (and the ElfReladyn fixture's process-wide array via new #ifdef UNIT_TEST freeForTest/freeNativeLibsForTest helpers), so the binary exits LeakSanitizer-clean for the first time. Teardown in the DelayedUnmap tests happens after the unmapper thread joins. --- ddprof-lib/src/main/cpp/codeCache.h | 13 +++++ ddprof-lib/src/main/cpp/libraries.h | 9 ++++ ddprof-lib/src/test/cpp/elfparser_ut.cpp | 68 +++++++++++++++--------- 3 files changed, 65 insertions(+), 25 deletions(-) diff --git a/ddprof-lib/src/main/cpp/codeCache.h b/ddprof-lib/src/main/cpp/codeCache.h index 6ced816af8..1b1db8a370 100644 --- a/ddprof-lib/src/main/cpp/codeCache.h +++ b/ddprof-lib/src/main/cpp/codeCache.h @@ -481,6 +481,19 @@ class CodeCacheArray { } return total; } + +#ifdef UNIT_TEST + // Deletes every registered CodeCache and resets the array to empty. Test + // teardown only: production arrays are append-only for process lifetime. + void freeForTest() { + for (int i = 0; i < count(); i++) { + delete at(i); + } + memset(_libs, 0, sizeof(_libs)); + _reserved = 0; + _count = 0; + } +#endif }; #endif // _CODECACHE_H diff --git a/ddprof-lib/src/main/cpp/libraries.h b/ddprof-lib/src/main/cpp/libraries.h index a7045a934c..e3487eaf9b 100644 --- a/ddprof-lib/src/main/cpp/libraries.h +++ b/ddprof-lib/src/main/cpp/libraries.h @@ -1,3 +1,8 @@ +/* + * Copyright 2026, Datadog, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + #ifndef _LIBRARIES_H #define _LIBRARIES_H @@ -89,6 +94,10 @@ class Libraries { // outlive every later lookup. The real population path is updateSymbols() // reading the loaded libraries; compiled only into gtest binaries. bool addLibraryForTest(CodeCache *lib) { return _native_libs.add(lib); } + + // Frees every CodeCache published into the process-wide array by + // updateSymbols(). Test teardown only (see CodeCacheArray::freeForTest). + void freeNativeLibsForTest() { _native_libs.freeForTest(); } #endif // Delete copy constructor and assignment operator to prevent copies diff --git a/ddprof-lib/src/test/cpp/elfparser_ut.cpp b/ddprof-lib/src/test/cpp/elfparser_ut.cpp index 18cddefb19..9bb33a9028 100644 --- a/ddprof-lib/src/test/cpp/elfparser_ut.cpp +++ b/ddprof-lib/src/test/cpp/elfparser_ut.cpp @@ -40,6 +40,14 @@ class ElfParser { static bool parseFile(CodeCache* cc, const char* base, const char* file_name, bool use_debug); }; +// parseLibraries() hands one heap CodeCache per library to the array and +// nothing frees it (production arrays live for the process lifetime). +static void freeCodeCacheArray(CodeCacheArray& libs) { + for (int i = 0; i < libs.count(); i++) { + delete libs[i]; + } +} + // Test name for crash handler static constexpr char ELF_TEST_NAME[] = "ElfParserTest"; @@ -60,13 +68,12 @@ static ElfParserGlobalSetup global_setup; TEST(Elf, readSymTable) { char cwd[PATH_MAX - 64]; if (getcwd(cwd, sizeof(cwd)) == nullptr) { - exit(1); + FAIL() << "getcwd() failed"; } char path[PATH_MAX]; snprintf(path, sizeof(path) - 1, "%s/../build/test/resources/native-libs/unresolved-functions/main", cwd); if (access(path, R_OK) != 0) { - fprintf(stdout, "Missing test resource %s. Skipping the test\n", path); - exit(0); + GTEST_SKIP() << "missing test resource " << path; } CodeCache cc("test"); ElfParser::parseFile(&cc, nullptr, path, false); @@ -81,13 +88,12 @@ class ElfReladyn : public ::testing::Test { void SetUp() override { char cwd[PATH_MAX - 64]; if (getcwd(cwd, sizeof(cwd)) == nullptr) { - exit(1); + FAIL() << "getcwd() failed"; } char path[PATH_MAX]; snprintf(path, sizeof(path) - 1, "%s/../build/test/resources/native-libs/reladyn-lib/libreladyn.so", cwd); if (access(path, R_OK) != 0) { - fprintf(stdout, "Missing test resource %s. Skipping the test\n", path); - exit(0); + GTEST_SKIP() << "missing test resource " << path; } void* handle = dlopen(path, RTLD_NOW); ASSERT_THAT(handle, ::testing::NotNull()); @@ -100,7 +106,13 @@ class ElfReladyn : public ::testing::Test { // This method is called after each test. void TearDown() override { - // Clean up resources. + if (_libs == nullptr) { + return; // SetUp skipped (missing resource): nothing was parsed + } + // parseLibraries skips libraries already in _parsed_inodes, so the + // next SetUp can only reparse after clearing them. + Symbols::clearParsingCaches(); + _libs->freeNativeLibsForTest(); } CodeCache* libreladyn() { @@ -129,10 +141,6 @@ class ElfTest : public ::testing::Test { // Reset global or static state Symbols::clearParsingCaches(); } - - void TearDown() override { - // probably some free of the array cache to be done - } }; @@ -175,6 +183,10 @@ TEST_F(ElfTest, invalidElf) { // Call the parsing function with the invalid ELF mapping Symbols::parseLibraries(&cc_array, false); + // The binary's own mapping is always collected and parsed. + ASSERT_GT(cc_array.count(), 0); + freeCodeCacheArray(cc_array); + munmap(addr, 4096); unlink("/tmp/invalid_elf"); } @@ -203,6 +215,9 @@ TEST_F(ElfTest, nonElfFile) { // Call the parsing function with the non-ELF mapping Symbols::parseLibraries(&cc_array, false); + ASSERT_GT(cc_array.count(), 0); + freeCodeCacheArray(cc_array); + // we could add checks here, though I am mainly relying on asan to crash // if something is wrong munmap(addr, contentSize); @@ -231,6 +246,9 @@ TEST_F(ElfTest, invalidElfSmallMapping) { // Call the parsing function with the small invalid ELF mapping Symbols::parseLibraries(&cc_array, false); + ASSERT_GT(cc_array.count(), 0); + freeCodeCacheArray(cc_array); + munmap(addr, 16); unlink("/tmp/invalid_elf_small"); } @@ -257,6 +275,8 @@ TEST_F(ElfTest, nonElfFileSmallMapping) { // Call the parsing function with the small non-ELF mapping Symbols::parseLibraries(&cc_array, false); + ASSERT_GT(cc_array.count(), 0); + freeCodeCacheArray(cc_array); munmap(addr, contentSize); unlink("/tmp/non_elf_small"); } @@ -267,10 +287,6 @@ class ElfTestParam : public ::testing::TestWithParam { // Reset global or static state Symbols::clearParsingCaches(); } - - void TearDown() override { - // probably some free of the array cache to be done - } }; @@ -314,6 +330,9 @@ TEST_P(ElfTestParam, invalidElfSmallMappingAfterUnmap) { // Join the unmapper thread to ensure it has finished unmapper.join(); + + ASSERT_GT(cc_array.count(), 0); + freeCodeCacheArray(cc_array); } INSTANTIATE_TEST_SUITE_P( @@ -326,7 +345,7 @@ INSTANTIATE_TEST_SUITE_P( TEST_P(ElfTestParam, invalidElfSmallMappingAfterUnmap) { char cwd[PATH_MAX - 64]; if (getcwd(cwd, sizeof(cwd)) == nullptr) { - exit(1); + FAIL() << "getcwd() failed"; } // Configure logging (assuming Log is defined elsewhere) @@ -336,13 +355,11 @@ TEST_P(ElfTestParam, invalidElfSmallMappingAfterUnmap) { char path[PATH_MAX]; snprintf(path, sizeof(path) - 1, "%s/../build/test/resources/native-libs/small-lib/libsmall-lib.so", cwd); if (access(path, R_OK) != 0) { - fprintf(stdout, "Missing test resource %s. Skipping the test\n", path); - exit(1); + GTEST_SKIP() << "missing test resource " << path; } void* handle = dlopen(path, RTLD_NOW); if (!handle) { - fprintf(stderr, "dlopen failed: %s\n", dlerror()); - exit(1); + FAIL() << "dlopen failed: " << dlerror(); } CodeCacheArray cc_array; @@ -359,6 +376,9 @@ TEST_P(ElfTestParam, invalidElfSmallMappingAfterUnmap) { // Call the parsing function in the main thread, this is where we can crash Symbols::parseLibraries(&cc_array, false); unmapper.join(); + + ASSERT_GT(cc_array.count(), 0); + freeCodeCacheArray(cc_array); } INSTANTIATE_TEST_SUITE_P( @@ -583,11 +603,9 @@ TEST_F(ElfTest, symbolNameOffsetOutOfBounds) { parseElfBytes(b); // must not crash: strAt() rejects st_name == strtab_size } -// A .symtab whose sh_offset is not a multiple of alignof(Elf64_Sym) (the -// 42-byte section-header string table lands it at 490) drives the misaligned -// entries through loadSymbolTable() and addRelocationSymbols(): both symbols -// and the PLT relocation name must still resolve. Before the alignment fix -// UBSan aborted here at the first ElfSymbol member access. +// A .symtab misaligned by the 42-byte section-header string table (sh_offset +// 490, not a multiple of alignof(Elf64_Sym)) must still resolve both symbols +// and the PLT relocation name through loadSymbolTable()/addRelocationSymbols(). TEST_F(ElfTest, symbolTableMisalignedEntries) { const uint16_t NSEC = 6; const uint64_t shoff = sizeof(Elf64_Ehdr); From f06c0c213df1d00b8f1b7d6c22943a11752daf4b Mon Sep 17 00:00:00 2001 From: Paul Fournillon Date: Tue, 10 Nov 2026 10:37:12 +0100 Subject: [PATCH 04/15] Drop the sanitizer reference from the ELF entry copy comment (PROF-16551) The clause named the detection tooling, not a constraint on the code. --- ddprof-lib/src/main/cpp/symbols_linux.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddprof-lib/src/main/cpp/symbols_linux.cpp b/ddprof-lib/src/main/cpp/symbols_linux.cpp index f6683900e5..56540878e1 100644 --- a/ddprof-lib/src/main/cpp/symbols_linux.cpp +++ b/ddprof-lib/src/main/cpp/symbols_linux.cpp @@ -324,7 +324,7 @@ static char _debuginfod_cache_buf[PATH_MAX] = {0}; // Symbol-table addresses (file sh_offset or DT_SYMTAB) carry no alignment // guarantee; these copies avoid member access through a misaligned ElfSymbol*, -// which is undefined behavior; UBSan's alignment check detects it. +// which is undefined behavior. static uint32_t symbolNameAt(const void* sym) { uint32_t name; memcpy(&name, sym, sizeof(name)); // st_name sits at offset 0 of Elf64_Sym From fc87c334138bd5a017c1dc373e3759d61b590c75 Mon Sep 17 00:00:00 2001 From: Paul Fournillon Date: Tue, 10 Nov 2026 11:51:46 +0100 Subject: [PATCH 05/15] fix(symbols): keep DT_SYMTAB entry reads conditional (PROF-16551) --- ddprof-lib/src/main/cpp/symbols_linux.cpp | 98 +++++++++++++---------- ddprof-lib/src/test/cpp/elfparser_ut.cpp | 75 +++++++++++++++++ 2 files changed, 131 insertions(+), 42 deletions(-) diff --git a/ddprof-lib/src/main/cpp/symbols_linux.cpp b/ddprof-lib/src/main/cpp/symbols_linux.cpp index 56540878e1..a83e1c09a3 100644 --- a/ddprof-lib/src/main/cpp/symbols_linux.cpp +++ b/ddprof-lib/src/main/cpp/symbols_linux.cpp @@ -323,7 +323,7 @@ typedef Elf64_Dyn ElfDyn; static char _debuginfod_cache_buf[PATH_MAX] = {0}; // Symbol-table addresses (file sh_offset or DT_SYMTAB) carry no alignment -// guarantee; these copies avoid member access through a misaligned ElfSymbol*, +// guarantee; this copy avoids member access through a misaligned ElfSymbol*, // which is undefined behavior. static uint32_t symbolNameAt(const void* sym) { uint32_t name; @@ -331,12 +331,6 @@ static uint32_t symbolNameAt(const void* sym) { return name; } -static ElfSymbol readSymbolEntry(const void* sym) { - ElfSymbol copy; - memcpy(©, sym, sizeof(copy)); - return copy; -} - class ElfParser { private: CodeCache* _cc; @@ -985,40 +979,60 @@ void ElfParser::loadSymbolTable(const char* symbols, size_t total_size, size_t e // to a small pointer that still compares <= end, walking off the image. The // `ent_size <= total_size - off` form keeps off <= total_size with no overflow. for (size_t off = 0; ent_size <= total_size - off; off += ent_size) { - ElfSymbol sym = readSymbolEntry(symbols + off); - if (sym.st_name != 0 && sym.st_value != 0) { - // Resolve the name through the bounded string table; a bad st_name - // offset (or unterminated string) drops the symbol instead of reading - // out of bounds. - const char* sym_name = strAt(strings, strings_size, sym.st_name); - if (sym_name == NULL) { - continue; - } - // Skip special AArch64 mapping symbols: $x and $d - if (sym.st_size != 0 || sym.st_info != 0 || sym_name[0] != '$') { - const char* addr; - if (base != NULL) { - // Check for overflow when adding sym.st_value to base - uintptr_t base_addr = (uintptr_t)base; - uint64_t symbol_value = sym.st_value; - - // Skip this symbol if addition would overflow - // First check if symbol_value exceeds the address space - if (symbol_value > UINTPTR_MAX) { - continue; - } - // Then check if addition would overflow - if (base_addr > UINTPTR_MAX - (uintptr_t)symbol_value) { - continue; - } - - // Perform addition using integer arithmetic to avoid pointer overflow - addr = (const char*)(base_addr + (uintptr_t)symbol_value); - } else { - addr = (const char*)sym.st_value; + const char* sym = symbols + off; + // Field copies instead of dereferencing a misaligned ElfSymbol*; + // dynamic-table entries are not extent-validated, so reads stay + // conditional to avoid reading past the mapped region. + uint32_t st_name = symbolNameAt(sym); + if (st_name == 0) { + continue; + } + uint64_t st_value; + memcpy(&st_value, sym + offsetof(ElfSymbol, st_value), sizeof(st_value)); + if (st_value == 0) { + continue; + } + // Resolve the name through the bounded string table; a bad st_name + // offset (or unterminated string) drops the symbol instead of reading + // out of bounds. + const char* sym_name = strAt(strings, strings_size, st_name); + if (sym_name == NULL) { + continue; + } + // Skip special AArch64 mapping symbols: $x and $d + uint64_t st_size; + memcpy(&st_size, sym + offsetof(ElfSymbol, st_size), sizeof(st_size)); + bool keep = st_size != 0; + if (!keep) { + // st_info is read only when st_size is zero, keeping the read + // conditional; both must be zero before the name decides. + uint8_t st_info; + memcpy(&st_info, sym + offsetof(ElfSymbol, st_info), sizeof(st_info)); + keep = st_info != 0 || sym_name[0] != '$'; + } + if (keep) { + const char* addr; + if (base != NULL) { + // Check for overflow when adding st_value to base + uintptr_t base_addr = (uintptr_t)base; + uint64_t symbol_value = st_value; + + // Skip this symbol if addition would overflow + // First check if symbol_value exceeds the address space + if (symbol_value > UINTPTR_MAX) { + continue; + } + // Then check if addition would overflow + if (base_addr > UINTPTR_MAX - (uintptr_t)symbol_value) { + continue; } - _cc->add(addr, (int)sym.st_size, sym_name); + + // Perform addition using integer arithmetic to avoid pointer overflow + addr = (const char*)(base_addr + (uintptr_t)symbol_value); + } else { + addr = (const char*)st_value; } + _cc->add(addr, (int)st_size, sym_name); } } } @@ -1058,13 +1072,13 @@ void ElfParser::addRelocationSymbols(ElfSection* reltab, const char* plt) { if (ELF_R_SYM(r->r_info) > max_sym_index) { continue; } - ElfSymbol sym = readSymbolEntry(symbols + (size_t)ELF_R_SYM(r->r_info) * sym_ent); + uint32_t st_name = symbolNameAt(symbols + (size_t)ELF_R_SYM(r->r_info) * sym_ent); char name[256]; - if (sym.st_name == 0) { + if (st_name == 0) { strcpy(name, "@plt"); } else { - const char* sym_name = strAt(strings, strings_size, sym.st_name); + const char* sym_name = strAt(strings, strings_size, st_name); if (sym_name == NULL) { continue; // plt advances via the for-increment } diff --git a/ddprof-lib/src/test/cpp/elfparser_ut.cpp b/ddprof-lib/src/test/cpp/elfparser_ut.cpp index 9bb33a9028..93d584f356 100644 --- a/ddprof-lib/src/test/cpp/elfparser_ut.cpp +++ b/ddprof-lib/src/test/cpp/elfparser_ut.cpp @@ -38,6 +38,7 @@ class ElfParser { public: static bool parseFile(CodeCache* cc, const char* base, const char* file_name, bool use_debug); + static void parseProgramHeaders(CodeCache* cc, const char* base, const char* end, bool relocate_dyn); }; // parseLibraries() hands one heap CodeCache per library to the array and @@ -443,6 +444,14 @@ void parseElfBytes(const std::vector& bytes) { parseElfBytes(bytes, &cc, /*use_debug=*/false); } +// Releases a test mapping on every exit path, including the early returns +// that gtest ASSERT macros perform. +struct MappingGuard { + void* addr; + size_t size; + ~MappingGuard() { munmap(addr, size); } +}; + } // namespace // Regression test for the build-id parser hardening (found by fuzz_elf). @@ -685,4 +694,70 @@ TEST_F(ElfTest, symbolTableMisalignedEntries) { EXPECT_NE(cc.findSymbol("mis_helper@plt"), nullptr); } +// A dynamic symbol table (DT_SYMTAB) points into live mapped memory whose +// extent is not validated by contentAt(); an entry may straddle the end of the +// readable region. The entry with st_name == 0 must be skipped after the +// st_name read alone: an unconditional full-entry copy faults on the PROT_NONE +// page that follows the readable region. +TEST_F(ElfTest, dynamicSymtabStraddlesReadableEnd) { + const size_t page = sysconf(_SC_PAGESIZE); + const size_t readable = 2 * page; + uint8_t* img = (uint8_t*)mmap(NULL, 3 * page, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + ASSERT_NE(img, MAP_FAILED); + MappingGuard mapping{img, 3 * page}; + memset(img, 0, readable); + + const size_t dyn_off = 0x200, hash_off = 0x2a0, str_off = 0x2b0; + const size_t sym_off = readable - sizeof(Elf64_Sym) - 8; // entry 1 straddles + + Elf64_Ehdr e = validEhdr(); + e.e_phoff = sizeof(Elf64_Ehdr); + e.e_phentsize = sizeof(Elf64_Phdr); + e.e_phnum = 2; + memcpy(img, &e, sizeof(e)); + + Elf64_Phdr ph[2]; + memset(ph, 0, sizeof(ph)); + ph[0].p_type = PT_LOAD; + ph[0].p_flags = PF_R | PF_X; + ph[0].p_memsz = readable; + ph[1].p_type = PT_DYNAMIC; + ph[1].p_vaddr = dyn_off; + ph[1].p_memsz = 7 * sizeof(Elf64_Dyn); + memcpy(img + sizeof(Elf64_Ehdr), ph, sizeof(ph)); + + // relent must be non-zero or parseDynamicSection() returns before the + // symbol-table loop; no relocation tables are present to consume it. + const Elf64_Dyn dyn[] = { + {DT_HASH, hash_off}, {DT_STRTAB, str_off}, {DT_SYMTAB, sym_off}, + {DT_STRSZ, 14}, {DT_SYMENT, sizeof(Elf64_Sym)}, {DT_RELAENT, sizeof(Elf64_Rela)}, + {DT_NULL, 0}, + }; + memcpy(img + dyn_off, dyn, sizeof(dyn)); + + const uint32_t hash[] = {1, 2}; // nbucket, nchain: two symbols + memcpy(img + hash_off, hash, sizeof(hash)); + + const char strtab[] = "\0strad_helper\0"; + memcpy(img + str_off, strtab, sizeof(strtab)); + + Elf64_Sym sym[2]; + memset(sym, 0, sizeof(sym)); + sym[0].st_name = 1; // "strad_helper" + sym[0].st_value = 0x1000; + sym[0].st_size = 4; + sym[1].st_name = 0; // straddling entry: skipped after the st_name read + memcpy(img + sym_off, sym, sizeof(sym)); + + ASSERT_EQ(mprotect(img, readable, PROT_READ), 0); + ASSERT_EQ(mprotect(img + readable, page, PROT_NONE), 0); + + CodeCache cc("regress"); + ElfParser::parseProgramHeaders(&cc, (const char*)img, (const char*)img + readable, false); + // Entry 0 resolved through the dynamic table; entry 1 was skipped without + // reading past the readable end. + EXPECT_NE(cc.findSymbol("strad_helper"), nullptr); +} + #endif //__linux__ From 07b533db911d6217e9ec87a28374fc7d280b2563 Mon Sep 17 00:00:00 2001 From: Paul Fournillon Date: Tue, 10 Nov 2026 14:15:54 +0100 Subject: [PATCH 06/15] test: address review polish on elfparser regressions (PROF-16551) --- ddprof-lib/src/main/cpp/symbols_linux.cpp | 4 +-- ddprof-lib/src/test/cpp/elfparser_ut.cpp | 37 ++++++++--------------- 2 files changed, 15 insertions(+), 26 deletions(-) diff --git a/ddprof-lib/src/main/cpp/symbols_linux.cpp b/ddprof-lib/src/main/cpp/symbols_linux.cpp index a83e1c09a3..193d06d6a9 100644 --- a/ddprof-lib/src/main/cpp/symbols_linux.cpp +++ b/ddprof-lib/src/main/cpp/symbols_linux.cpp @@ -981,8 +981,8 @@ void ElfParser::loadSymbolTable(const char* symbols, size_t total_size, size_t e for (size_t off = 0; ent_size <= total_size - off; off += ent_size) { const char* sym = symbols + off; // Field copies instead of dereferencing a misaligned ElfSymbol*; - // dynamic-table entries are not extent-validated, so reads stay - // conditional to avoid reading past the mapped region. + // the DT_SYMTAB caller does not extent-validate, so each field is + // read only when the entry survives the earlier checks. uint32_t st_name = symbolNameAt(sym); if (st_name == 0) { continue; diff --git a/ddprof-lib/src/test/cpp/elfparser_ut.cpp b/ddprof-lib/src/test/cpp/elfparser_ut.cpp index 93d584f356..ec54baabe0 100644 --- a/ddprof-lib/src/test/cpp/elfparser_ut.cpp +++ b/ddprof-lib/src/test/cpp/elfparser_ut.cpp @@ -34,21 +34,12 @@ #include // Forward declaration for ElfParser functionality from symbols_linux.cpp -// The actual implementation will be available through the patched upstream file class ElfParser { public: static bool parseFile(CodeCache* cc, const char* base, const char* file_name, bool use_debug); static void parseProgramHeaders(CodeCache* cc, const char* base, const char* end, bool relocate_dyn); }; -// parseLibraries() hands one heap CodeCache per library to the array and -// nothing frees it (production arrays live for the process lifetime). -static void freeCodeCacheArray(CodeCacheArray& libs) { - for (int i = 0; i < libs.count(); i++) { - delete libs[i]; - } -} - // Test name for crash handler static constexpr char ELF_TEST_NAME[] = "ElfParserTest"; @@ -186,7 +177,7 @@ TEST_F(ElfTest, invalidElf) { // The binary's own mapping is always collected and parsed. ASSERT_GT(cc_array.count(), 0); - freeCodeCacheArray(cc_array); + cc_array.freeForTest(); munmap(addr, 4096); unlink("/tmp/invalid_elf"); @@ -217,7 +208,7 @@ TEST_F(ElfTest, nonElfFile) { Symbols::parseLibraries(&cc_array, false); ASSERT_GT(cc_array.count(), 0); - freeCodeCacheArray(cc_array); + cc_array.freeForTest(); // we could add checks here, though I am mainly relying on asan to crash // if something is wrong @@ -248,7 +239,7 @@ TEST_F(ElfTest, invalidElfSmallMapping) { Symbols::parseLibraries(&cc_array, false); ASSERT_GT(cc_array.count(), 0); - freeCodeCacheArray(cc_array); + cc_array.freeForTest(); munmap(addr, 16); unlink("/tmp/invalid_elf_small"); @@ -277,7 +268,7 @@ TEST_F(ElfTest, nonElfFileSmallMapping) { // Call the parsing function with the small non-ELF mapping Symbols::parseLibraries(&cc_array, false); ASSERT_GT(cc_array.count(), 0); - freeCodeCacheArray(cc_array); + cc_array.freeForTest(); munmap(addr, contentSize); unlink("/tmp/non_elf_small"); } @@ -333,7 +324,7 @@ TEST_P(ElfTestParam, invalidElfSmallMappingAfterUnmap) { unmapper.join(); ASSERT_GT(cc_array.count(), 0); - freeCodeCacheArray(cc_array); + cc_array.freeForTest(); } INSTANTIATE_TEST_SUITE_P( @@ -379,7 +370,7 @@ TEST_P(ElfTestParam, invalidElfSmallMappingAfterUnmap) { unmapper.join(); ASSERT_GT(cc_array.count(), 0); - freeCodeCacheArray(cc_array); + cc_array.freeForTest(); } INSTANTIATE_TEST_SUITE_P( @@ -422,8 +413,8 @@ Elf64_Ehdr validEhdr() { } // Write the bytes to a unique temp file and run ElfParser::parseFile over it, -// mirroring how Symbols::parseLibraries() parses an on-disk library. The -// caller-visible CodeCache lets tests assert what the parser resolved. +// mirroring an on-disk library parse. The caller-visible CodeCache lets tests +// assert what the parser resolved. void parseElfBytes(const std::vector& bytes, CodeCache* cc, bool use_debug) { char path[] = "/tmp/elf_regress_XXXXXX"; int fd = mkstemp(path); @@ -694,16 +685,14 @@ TEST_F(ElfTest, symbolTableMisalignedEntries) { EXPECT_NE(cc.findSymbol("mis_helper@plt"), nullptr); } -// A dynamic symbol table (DT_SYMTAB) points into live mapped memory whose -// extent is not validated by contentAt(); an entry may straddle the end of the -// readable region. The entry with st_name == 0 must be skipped after the -// st_name read alone: an unconditional full-entry copy faults on the PROT_NONE -// page that follows the readable region. +// A DT_SYMTAB entry may straddle the end of the readable region (the dynamic +// path does not extent-validate). An entry with st_name == 0 must be dropped +// after the st_name read alone: a full-entry copy faults on the PROT_NONE page. TEST_F(ElfTest, dynamicSymtabStraddlesReadableEnd) { const size_t page = sysconf(_SC_PAGESIZE); const size_t readable = 2 * page; - uint8_t* img = (uint8_t*)mmap(NULL, 3 * page, PROT_READ | PROT_WRITE, - MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + uint8_t* img = static_cast(mmap(NULL, 3 * page, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0)); ASSERT_NE(img, MAP_FAILED); MappingGuard mapping{img, 3 * page}; memset(img, 0, readable); From e846461a42ccbc1ef68976c71a5deb6a9bb0a789 Mon Sep 17 00:00:00 2001 From: Paul Fournillon Date: Tue, 10 Nov 2026 16:35:26 +0100 Subject: [PATCH 07/15] test: make MappingGuard non-copyable (PROF-16551) --- .gitignore | 1 + ddprof-lib/src/test/cpp/elfparser_ut.cpp | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 26938bb954..ba3cd4cecb 100644 --- a/.gitignore +++ b/.gitignore @@ -32,6 +32,7 @@ datadog/maven/resources **/launcher* /gradle.properties **/hs_err* +*.core # cursor AI history .history diff --git a/ddprof-lib/src/test/cpp/elfparser_ut.cpp b/ddprof-lib/src/test/cpp/elfparser_ut.cpp index ec54baabe0..0e6dedac4b 100644 --- a/ddprof-lib/src/test/cpp/elfparser_ut.cpp +++ b/ddprof-lib/src/test/cpp/elfparser_ut.cpp @@ -438,9 +438,13 @@ void parseElfBytes(const std::vector& bytes) { // Releases a test mapping on every exit path, including the early returns // that gtest ASSERT macros perform. struct MappingGuard { + MappingGuard(void* mapping, size_t size) : addr(mapping), size(size) {} + ~MappingGuard() { munmap(addr, size); } + MappingGuard(const MappingGuard&) = delete; + MappingGuard& operator=(const MappingGuard&) = delete; + void* addr; size_t size; - ~MappingGuard() { munmap(addr, size); } }; } // namespace From 9d7ffae96e50d135cad85da571d1de055bab266b Mon Sep 17 00:00:00 2001 From: Paul Fournillon Date: Wed, 11 Nov 2026 00:24:21 +0100 Subject: [PATCH 08/15] test: rebuild the library singleton with cleared parse caches (PROF-16551) --- ddprof-lib/src/test/cpp/elfparser_ut.cpp | 30 ++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/ddprof-lib/src/test/cpp/elfparser_ut.cpp b/ddprof-lib/src/test/cpp/elfparser_ut.cpp index 0e6dedac4b..6d136991ba 100644 --- a/ddprof-lib/src/test/cpp/elfparser_ut.cpp +++ b/ddprof-lib/src/test/cpp/elfparser_ut.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -91,6 +92,8 @@ class ElfReladyn : public ::testing::Test { ASSERT_THAT(handle, ::testing::NotNull()); _libs = Libraries::instance(); + // Local-array parses also fill the global inode cache; rebuild the singleton independently. + Symbols::clearParsingCaches(); _libs->updateSymbols(false); _libreladyn = _libs->findLibraryByName("libreladyn"); ASSERT_THAT(_libreladyn, ::testing::NotNull()); @@ -101,8 +104,6 @@ class ElfReladyn : public ::testing::Test { if (_libs == nullptr) { return; // SetUp skipped (missing resource): nothing was parsed } - // parseLibraries skips libraries already in _parsed_inodes, so the - // next SetUp can only reparse after clearing them. Symbols::clearParsingCaches(); _libs->freeNativeLibsForTest(); } @@ -117,6 +118,31 @@ TEST_F(ElfReladyn, resolveFromRela_plt) { ASSERT_THAT(sym, ::testing::NotNull()); } +TEST_F(ElfReladyn, reinitializeAfterLocalLibraryParse) { + const u64 reladyn_file_id = libreladyn()->fileId(); + TearDown(); + + { + CodeCacheArray local_libraries; + auto free_libraries = [](CodeCacheArray* libs) { libs->freeForTest(); }; + std::unique_ptr cleanup(&local_libraries, free_libraries); + Symbols::parseLibraries(&local_libraries, false); + + bool parsed_reladyn = false; + for (int i = 0; i < local_libraries.count(); i++) { + CodeCache* lib = local_libraries[i]; + if (lib != nullptr && lib->fileId() == reladyn_file_id) { + parsed_reladyn = true; + break; + } + } + ASSERT_TRUE(parsed_reladyn); + } + + ASSERT_NO_FATAL_FAILURE(SetUp()); + ASSERT_THAT(libreladyn()->findImport(im_pthread_create), ::testing::NotNull()); +} + TEST_F(ElfReladyn, resolveFromRela_dyn_R_GLOB_DAT) { void* sym = libreladyn()->findImport(im_pthread_setspecific); ASSERT_THAT(sym, ::testing::NotNull()); From d6e5c79afc3128b294d32179c06d643c68ac0147 Mon Sep 17 00:00:00 2001 From: Paul Fournillon Date: Wed, 11 Nov 2026 00:24:23 +0100 Subject: [PATCH 09/15] test: reverse NativeMem accounting in freeForTest (PROF-16551) --- ddprof-lib/src/main/cpp/codeCache.h | 5 +- ddprof-lib/src/test/cpp/codeCache_ut.cpp | 68 ++++++++++++++++++++---- 2 files changed, 63 insertions(+), 10 deletions(-) diff --git a/ddprof-lib/src/main/cpp/codeCache.h b/ddprof-lib/src/main/cpp/codeCache.h index 1b1db8a370..0e04b5268e 100644 --- a/ddprof-lib/src/main/cpp/codeCache.h +++ b/ddprof-lib/src/main/cpp/codeCache.h @@ -487,7 +487,10 @@ class CodeCacheArray { // teardown only: production arrays are append-only for process lifetime. void freeForTest() { for (int i = 0; i < count(); i++) { - delete at(i); + CodeCache *lib = at(i); + NativeMem::record(NM_NATIVE_SYMBOLS, -lib->memoryUsage()); + NativeMem::recordOverhead(NM_NATIVE_SYMBOLS, -lib->nameOverhead()); + delete lib; } memset(_libs, 0, sizeof(_libs)); _reserved = 0; diff --git a/ddprof-lib/src/test/cpp/codeCache_ut.cpp b/ddprof-lib/src/test/cpp/codeCache_ut.cpp index dd6806be53..052f2ad81e 100644 --- a/ddprof-lib/src/test/cpp/codeCache_ut.cpp +++ b/ddprof-lib/src/test/cpp/codeCache_ut.cpp @@ -6,6 +6,7 @@ #include #include "codeCache.h" +#include #include // memoryUsage() must reflect the actual per-symbol name length, not a fixed @@ -81,25 +82,74 @@ TEST(CodeCacheTest, CopyIsDeepAndDoesNotDoubleFree) { EXPECT_EQ(expected, src.memoryUsage()); // src's names still intact } // src destroyed — no double free +class CodeCacheArrayTest : public ::testing::Test { +protected: + CodeCacheArray arr; + + void TearDown() override { + arr.freeForTest(); + } +}; + // CodeCacheArray::memoryUsage() is the aggregation production actually consumes // (Profiler::dump()). Verify it sums the per-library values — guards the loop // bound and the accumulation against regressions the per-instance tests miss. -TEST(CodeCacheTest, ArrayMemoryUsageSumsLibraries) { - CodeCacheArray arr; - CodeCache *a = new CodeCache("liba"); - CodeCache *b = new CodeCache("libb"); +TEST_F(CodeCacheArrayTest, ArrayMemoryUsageSumsLibraries) { + auto a = std::make_unique("liba"); + auto b = std::make_unique("libb"); // Populate before registering: add() must run pre-publication. a->add(reinterpret_cast(0x1000), 16, "alpha_symbol"); b->add(reinterpret_cast(0x2000), 16, "beta_symbol_longer_name"); long long expected = (long long)a->memoryUsage() + (long long)b->memoryUsage(); - ASSERT_TRUE(arr.add(a)); - ASSERT_TRUE(arr.add(b)); + ASSERT_TRUE(arr.add(a.get())); + a.release(); + ASSERT_TRUE(arr.add(b.get())); + b.release(); EXPECT_EQ(expected, (long long)arr.memoryUsage()); +} - // CodeCacheArray does not own the entries. - delete a; - delete b; +TEST_F(CodeCacheArrayTest, FreeForTestRestoresAccountingAndAllowsReuse) { + const long long live_before = NativeMem::live(NM_NATIVE_SYMBOLS); + const long long overhead_before = NativeMem::overhead(NM_NATIVE_SYMBOLS); + arr.freeForTest(); + EXPECT_EQ(live_before, NativeMem::live(NM_NATIVE_SYMBOLS)); + EXPECT_EQ(overhead_before, NativeMem::overhead(NM_NATIVE_SYMBOLS)); + + for (int cycle = 0; cycle < 2; cycle++) { + auto a = std::make_unique("cleanup_liba"); + auto b = std::make_unique("cleanup_libb_with_longer_name"); + a->add(reinterpret_cast(0x1000), 16, "alpha_symbol"); + b->add(reinterpret_cast(0x2000), 32, "beta_symbol_longer_name"); + b->add(reinterpret_cast(0x3000), 8, "gamma"); + CodeCache *first = a.get(); + CodeCache *second = b.get(); + const long long live = a->memoryUsage() + b->memoryUsage(); + const long long overhead = a->nameOverhead() + b->nameOverhead(); + ASSERT_GT(live, 0); + + ASSERT_TRUE(arr.add(a.get())); + a.release(); + ASSERT_TRUE(arr.add(b.get())); + b.release(); + ASSERT_EQ(2, arr.count()); + EXPECT_EQ(first, arr[0]); + EXPECT_EQ(second, arr[1]); + EXPECT_EQ(live, static_cast(arr.memoryUsage())); + EXPECT_EQ(live_before + live, NativeMem::live(NM_NATIVE_SYMBOLS)); + EXPECT_EQ(overhead_before + overhead, NativeMem::overhead(NM_NATIVE_SYMBOLS)); + + for (int cleanup = 0; cleanup < 2; cleanup++) { + arr.freeForTest(); + EXPECT_EQ(live_before, NativeMem::live(NM_NATIVE_SYMBOLS)); + EXPECT_EQ(overhead_before, NativeMem::overhead(NM_NATIVE_SYMBOLS)); + EXPECT_EQ(0, arr.count()); + EXPECT_EQ(0u, arr.memoryUsage()); + for (int slot = 0; slot < MAX_NATIVE_LIBS; slot++) { + EXPECT_EQ(nullptr, arr.at(slot)) << slot; + } + } + } } // The two-phase add used under JitCodeCache's stubs lock: prepareAdd() hands From b39066502619f8eeb13eb68b24d05e955371fa19 Mon Sep 17 00:00:00 2001 From: Paul Fournillon Date: Wed, 11 Nov 2026 01:03:17 +0100 Subject: [PATCH 10/15] test: address reviewer notes on test teardown (PROF-16551) --- ddprof-lib/src/main/cpp/codeCache.h | 8 +++++--- ddprof-lib/src/test/cpp/codeCache_ut.cpp | 2 ++ ddprof-lib/src/test/cpp/elfparser_ut.cpp | 4 ++++ 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/ddprof-lib/src/main/cpp/codeCache.h b/ddprof-lib/src/main/cpp/codeCache.h index 0e04b5268e..10380c8f33 100644 --- a/ddprof-lib/src/main/cpp/codeCache.h +++ b/ddprof-lib/src/main/cpp/codeCache.h @@ -488,9 +488,11 @@ class CodeCacheArray { void freeForTest() { for (int i = 0; i < count(); i++) { CodeCache *lib = at(i); - NativeMem::record(NM_NATIVE_SYMBOLS, -lib->memoryUsage()); - NativeMem::recordOverhead(NM_NATIVE_SYMBOLS, -lib->nameOverhead()); - delete lib; + if (lib != nullptr) { + NativeMem::record(NM_NATIVE_SYMBOLS, -lib->memoryUsage()); + NativeMem::recordOverhead(NM_NATIVE_SYMBOLS, -lib->nameOverhead()); + delete lib; + } } memset(_libs, 0, sizeof(_libs)); _reserved = 0; diff --git a/ddprof-lib/src/test/cpp/codeCache_ut.cpp b/ddprof-lib/src/test/cpp/codeCache_ut.cpp index 052f2ad81e..07862037c6 100644 --- a/ddprof-lib/src/test/cpp/codeCache_ut.cpp +++ b/ddprof-lib/src/test/cpp/codeCache_ut.cpp @@ -109,6 +109,8 @@ TEST_F(CodeCacheArrayTest, ArrayMemoryUsageSumsLibraries) { EXPECT_EQ(expected, (long long)arr.memoryUsage()); } +// freeForTest() must restore the NM gauges to their pre-add baseline and +// leave the array reusable for a later cycle. TEST_F(CodeCacheArrayTest, FreeForTestRestoresAccountingAndAllowsReuse) { const long long live_before = NativeMem::live(NM_NATIVE_SYMBOLS); const long long overhead_before = NativeMem::overhead(NM_NATIVE_SYMBOLS); diff --git a/ddprof-lib/src/test/cpp/elfparser_ut.cpp b/ddprof-lib/src/test/cpp/elfparser_ut.cpp index 6d136991ba..c9d3afc920 100644 --- a/ddprof-lib/src/test/cpp/elfparser_ut.cpp +++ b/ddprof-lib/src/test/cpp/elfparser_ut.cpp @@ -104,6 +104,8 @@ class ElfReladyn : public ::testing::Test { if (_libs == nullptr) { return; // SetUp skipped (missing resource): nothing was parsed } + // parseLibraries skips inodes already in _parsed_inodes; clearing here + // lets the next parse (a later SetUp or a local array) reparse them. Symbols::clearParsingCaches(); _libs->freeNativeLibsForTest(); } @@ -118,6 +120,8 @@ TEST_F(ElfReladyn, resolveFromRela_plt) { ASSERT_THAT(sym, ::testing::NotNull()); } +// A local-array parse between fixture tests must not break the next SetUp +// reparse of the global library array. TEST_F(ElfReladyn, reinitializeAfterLocalLibraryParse) { const u64 reladyn_file_id = libreladyn()->fileId(); TearDown(); From 9533fcde7aab047c03ad839ce064ea8b440cf4f5 Mon Sep 17 00:00:00 2001 From: Paul Fournillon Date: Thu, 12 Nov 2026 18:35:14 +0100 Subject: [PATCH 11/15] fix(symbols): keep the whole-entry symbol copy from main in loadSymbolTable (PROF-16551) --- ddprof-lib/src/main/cpp/symbols_linux.cpp | 88 ++++++++++------------- ddprof-lib/src/test/cpp/elfparser_ut.cpp | 77 -------------------- 2 files changed, 36 insertions(+), 129 deletions(-) diff --git a/ddprof-lib/src/main/cpp/symbols_linux.cpp b/ddprof-lib/src/main/cpp/symbols_linux.cpp index 193d06d6a9..5320dc99bd 100644 --- a/ddprof-lib/src/main/cpp/symbols_linux.cpp +++ b/ddprof-lib/src/main/cpp/symbols_linux.cpp @@ -979,60 +979,44 @@ void ElfParser::loadSymbolTable(const char* symbols, size_t total_size, size_t e // to a small pointer that still compares <= end, walking off the image. The // `ent_size <= total_size - off` form keeps off <= total_size with no overflow. for (size_t off = 0; ent_size <= total_size - off; off += ent_size) { - const char* sym = symbols + off; - // Field copies instead of dereferencing a misaligned ElfSymbol*; - // the DT_SYMTAB caller does not extent-validate, so each field is - // read only when the entry survives the earlier checks. - uint32_t st_name = symbolNameAt(sym); - if (st_name == 0) { - continue; - } - uint64_t st_value; - memcpy(&st_value, sym + offsetof(ElfSymbol, st_value), sizeof(st_value)); - if (st_value == 0) { - continue; - } - // Resolve the name through the bounded string table; a bad st_name - // offset (or unterminated string) drops the symbol instead of reading - // out of bounds. - const char* sym_name = strAt(strings, strings_size, st_name); - if (sym_name == NULL) { - continue; - } - // Skip special AArch64 mapping symbols: $x and $d - uint64_t st_size; - memcpy(&st_size, sym + offsetof(ElfSymbol, st_size), sizeof(st_size)); - bool keep = st_size != 0; - if (!keep) { - // st_info is read only when st_size is zero, keeping the read - // conditional; both must be zero before the name decides. - uint8_t st_info; - memcpy(&st_info, sym + offsetof(ElfSymbol, st_info), sizeof(st_info)); - keep = st_info != 0 || sym_name[0] != '$'; - } - if (keep) { - const char* addr; - if (base != NULL) { - // Check for overflow when adding st_value to base - uintptr_t base_addr = (uintptr_t)base; - uint64_t symbol_value = st_value; - - // Skip this symbol if addition would overflow - // First check if symbol_value exceeds the address space - if (symbol_value > UINTPTR_MAX) { - continue; - } - // Then check if addition would overflow - if (base_addr > UINTPTR_MAX - (uintptr_t)symbol_value) { - continue; + // The section can start at any crafted file offset, so the entry address + // may violate Elf64_Sym's 8-byte alignment; copy the entry instead of + // aliasing mapped bytes through a typed pointer. + ElfSymbol entry; + memcpy(&entry, symbols + off, sizeof(entry)); + if (entry.st_name != 0 && entry.st_value != 0) { + // Resolve the name through the bounded string table; a bad st_name + // offset (or unterminated string) drops the symbol instead of reading + // out of bounds. + const char* sym_name = strAt(strings, strings_size, entry.st_name); + if (sym_name == NULL) { + continue; + } + // Skip special AArch64 mapping symbols: $x and $d + if (entry.st_size != 0 || entry.st_info != 0 || sym_name[0] != '$') { + const char* addr; + if (base != NULL) { + // Check for overflow when adding entry.st_value to base + uintptr_t base_addr = (uintptr_t)base; + uint64_t symbol_value = entry.st_value; + + // Skip this symbol if addition would overflow + // First check if symbol_value exceeds the address space + if (symbol_value > UINTPTR_MAX) { + continue; + } + // Then check if addition would overflow + if (base_addr > UINTPTR_MAX - (uintptr_t)symbol_value) { + continue; + } + + // Perform addition using integer arithmetic to avoid pointer overflow + addr = (const char*)(base_addr + (uintptr_t)symbol_value); + } else { + addr = (const char*)entry.st_value; } - - // Perform addition using integer arithmetic to avoid pointer overflow - addr = (const char*)(base_addr + (uintptr_t)symbol_value); - } else { - addr = (const char*)st_value; + _cc->add(addr, (int)entry.st_size, sym_name); } - _cc->add(addr, (int)st_size, sym_name); } } } diff --git a/ddprof-lib/src/test/cpp/elfparser_ut.cpp b/ddprof-lib/src/test/cpp/elfparser_ut.cpp index aae22e6f7a..5855abad5a 100644 --- a/ddprof-lib/src/test/cpp/elfparser_ut.cpp +++ b/ddprof-lib/src/test/cpp/elfparser_ut.cpp @@ -34,7 +34,6 @@ class ElfParser { public: static bool parseFile(CodeCache* cc, const char* base, const char* file_name, bool use_debug); - static void parseProgramHeaders(CodeCache* cc, const char* base, const char* end, bool relocate_dyn); }; // Test name for crash handler @@ -468,18 +467,6 @@ void parseElfBytes(const std::vector& bytes) { parseElfBytes(bytes, &cc, /*use_debug=*/false); } -// Releases a test mapping on every exit path, including the early returns -// that gtest ASSERT macros perform. -struct MappingGuard { - MappingGuard(void* mapping, size_t size) : addr(mapping), size(size) {} - ~MappingGuard() { munmap(addr, size); } - MappingGuard(const MappingGuard&) = delete; - MappingGuard& operator=(const MappingGuard&) = delete; - - void* addr; - size_t size; -}; - } // namespace // Regression test for the build-id parser hardening (found by fuzz_elf). @@ -722,68 +709,4 @@ TEST_F(ElfTest, symbolTableMisalignedEntries) { EXPECT_NE(cc.findSymbol("mis_helper@plt"), nullptr); } -// A DT_SYMTAB entry may straddle the end of the readable region (the dynamic -// path does not extent-validate). An entry with st_name == 0 must be dropped -// after the st_name read alone: a full-entry copy faults on the PROT_NONE page. -TEST_F(ElfTest, dynamicSymtabStraddlesReadableEnd) { - const size_t page = sysconf(_SC_PAGESIZE); - const size_t readable = 2 * page; - uint8_t* img = static_cast(mmap(NULL, 3 * page, PROT_READ | PROT_WRITE, - MAP_PRIVATE | MAP_ANONYMOUS, -1, 0)); - ASSERT_NE(img, MAP_FAILED); - MappingGuard mapping{img, 3 * page}; - memset(img, 0, readable); - - const size_t dyn_off = 0x200, hash_off = 0x2a0, str_off = 0x2b0; - const size_t sym_off = readable - sizeof(Elf64_Sym) - 8; // entry 1 straddles - - Elf64_Ehdr e = validEhdr(); - e.e_phoff = sizeof(Elf64_Ehdr); - e.e_phentsize = sizeof(Elf64_Phdr); - e.e_phnum = 2; - memcpy(img, &e, sizeof(e)); - - Elf64_Phdr ph[2]; - memset(ph, 0, sizeof(ph)); - ph[0].p_type = PT_LOAD; - ph[0].p_flags = PF_R | PF_X; - ph[0].p_memsz = readable; - ph[1].p_type = PT_DYNAMIC; - ph[1].p_vaddr = dyn_off; - ph[1].p_memsz = 7 * sizeof(Elf64_Dyn); - memcpy(img + sizeof(Elf64_Ehdr), ph, sizeof(ph)); - - // relent must be non-zero or parseDynamicSection() returns before the - // symbol-table loop; no relocation tables are present to consume it. - const Elf64_Dyn dyn[] = { - {DT_HASH, hash_off}, {DT_STRTAB, str_off}, {DT_SYMTAB, sym_off}, - {DT_STRSZ, 14}, {DT_SYMENT, sizeof(Elf64_Sym)}, {DT_RELAENT, sizeof(Elf64_Rela)}, - {DT_NULL, 0}, - }; - memcpy(img + dyn_off, dyn, sizeof(dyn)); - - const uint32_t hash[] = {1, 2}; // nbucket, nchain: two symbols - memcpy(img + hash_off, hash, sizeof(hash)); - - const char strtab[] = "\0strad_helper\0"; - memcpy(img + str_off, strtab, sizeof(strtab)); - - Elf64_Sym sym[2]; - memset(sym, 0, sizeof(sym)); - sym[0].st_name = 1; // "strad_helper" - sym[0].st_value = 0x1000; - sym[0].st_size = 4; - sym[1].st_name = 0; // straddling entry: skipped after the st_name read - memcpy(img + sym_off, sym, sizeof(sym)); - - ASSERT_EQ(mprotect(img, readable, PROT_READ), 0); - ASSERT_EQ(mprotect(img + readable, page, PROT_NONE), 0); - - CodeCache cc("regress"); - ElfParser::parseProgramHeaders(&cc, (const char*)img, (const char*)img + readable, false); - // Entry 0 resolved through the dynamic table; entry 1 was skipped without - // reading past the readable end. - EXPECT_NE(cc.findSymbol("strad_helper"), nullptr); -} - #endif //__linux__ From 86de471aa6cfdba57131e0edcb955eca97539d76 Mon Sep 17 00:00:00 2001 From: Paul Fournillon Date: Thu, 12 Nov 2026 18:44:48 +0100 Subject: [PATCH 12/15] test(elfparser): free parsed caches before asserting and tighten test helpers (PROF-16551) --- ddprof-lib/src/main/cpp/codeCache.h | 1 + ddprof-lib/src/main/cpp/libraries.h | 6 ++++-- ddprof-lib/src/main/cpp/symbols_linux.cpp | 2 ++ ddprof-lib/src/test/cpp/elfparser_ut.cpp | 18 ++++++++++++------ 4 files changed, 19 insertions(+), 8 deletions(-) diff --git a/ddprof-lib/src/main/cpp/codeCache.h b/ddprof-lib/src/main/cpp/codeCache.h index 10380c8f33..e2636b8cca 100644 --- a/ddprof-lib/src/main/cpp/codeCache.h +++ b/ddprof-lib/src/main/cpp/codeCache.h @@ -497,6 +497,7 @@ class CodeCacheArray { memset(_libs, 0, sizeof(_libs)); _reserved = 0; _count = 0; + _overflow_reported = false; } #endif }; diff --git a/ddprof-lib/src/main/cpp/libraries.h b/ddprof-lib/src/main/cpp/libraries.h index e3487eaf9b..5a22135330 100644 --- a/ddprof-lib/src/main/cpp/libraries.h +++ b/ddprof-lib/src/main/cpp/libraries.h @@ -95,8 +95,10 @@ class Libraries { // reading the loaded libraries; compiled only into gtest binaries. bool addLibraryForTest(CodeCache *lib) { return _native_libs.add(lib); } - // Frees every CodeCache published into the process-wide array by - // updateSymbols(). Test teardown only (see CodeCacheArray::freeForTest). + // Deletes every CodeCache in the process-wide array, including any passed + // to addLibraryForTest(), so it must not be combined with caller-owned + // caches in the same binary. Test teardown only (see + // CodeCacheArray::freeForTest). void freeNativeLibsForTest() { _native_libs.freeForTest(); } #endif diff --git a/ddprof-lib/src/main/cpp/symbols_linux.cpp b/ddprof-lib/src/main/cpp/symbols_linux.cpp index 5320dc99bd..7b1103ae80 100644 --- a/ddprof-lib/src/main/cpp/symbols_linux.cpp +++ b/ddprof-lib/src/main/cpp/symbols_linux.cpp @@ -7,6 +7,7 @@ #ifdef __linux__ #include +#include #include #include #include @@ -325,6 +326,7 @@ static char _debuginfod_cache_buf[PATH_MAX] = {0}; // Symbol-table addresses (file sh_offset or DT_SYMTAB) carry no alignment // guarantee; this copy avoids member access through a misaligned ElfSymbol*, // which is undefined behavior. +static_assert(offsetof(ElfSymbol, st_name) == 0, "symbolNameAt() reads st_name at offset 0"); static uint32_t symbolNameAt(const void* sym) { uint32_t name; memcpy(&name, sym, sizeof(name)); // st_name sits at offset 0 of Elf64_Sym diff --git a/ddprof-lib/src/test/cpp/elfparser_ut.cpp b/ddprof-lib/src/test/cpp/elfparser_ut.cpp index 5855abad5a..b4e2e851c8 100644 --- a/ddprof-lib/src/test/cpp/elfparser_ut.cpp +++ b/ddprof-lib/src/test/cpp/elfparser_ut.cpp @@ -213,8 +213,9 @@ TEST_F(ElfTest, invalidElf) { Symbols::parseLibraries(&cc_array, false); // The binary's own mapping is always collected and parsed. - ASSERT_GT(cc_array.count(), 0); + const int parsed = cc_array.count(); cc_array.freeForTest(); + EXPECT_GT(parsed, 0); munmap(addr, 4096); unlink("/tmp/invalid_elf"); @@ -244,8 +245,9 @@ TEST_F(ElfTest, nonElfFile) { // Call the parsing function with the non-ELF mapping Symbols::parseLibraries(&cc_array, false); - ASSERT_GT(cc_array.count(), 0); + const int parsed = cc_array.count(); cc_array.freeForTest(); + EXPECT_GT(parsed, 0); // we could add checks here, though I am mainly relying on asan to crash // if something is wrong @@ -275,8 +277,9 @@ TEST_F(ElfTest, invalidElfSmallMapping) { // Call the parsing function with the small invalid ELF mapping Symbols::parseLibraries(&cc_array, false); - ASSERT_GT(cc_array.count(), 0); + const int parsed = cc_array.count(); cc_array.freeForTest(); + EXPECT_GT(parsed, 0); munmap(addr, 16); unlink("/tmp/invalid_elf_small"); @@ -304,8 +307,9 @@ TEST_F(ElfTest, nonElfFileSmallMapping) { // Call the parsing function with the small non-ELF mapping Symbols::parseLibraries(&cc_array, false); - ASSERT_GT(cc_array.count(), 0); + const int parsed = cc_array.count(); cc_array.freeForTest(); + EXPECT_GT(parsed, 0); munmap(addr, contentSize); unlink("/tmp/non_elf_small"); } @@ -360,8 +364,9 @@ TEST_P(ElfTestParam, invalidElfSmallMappingAfterUnmap) { // Join the unmapper thread to ensure it has finished unmapper.join(); - ASSERT_GT(cc_array.count(), 0); + const int parsed = cc_array.count(); cc_array.freeForTest(); + EXPECT_GT(parsed, 0); } INSTANTIATE_TEST_SUITE_P( @@ -401,8 +406,9 @@ TEST_P(ElfTestParam, invalidElfSmallMappingAfterUnmap) { Symbols::parseLibraries(&cc_array, false); unmapper.join(); - ASSERT_GT(cc_array.count(), 0); + const int parsed = cc_array.count(); cc_array.freeForTest(); + EXPECT_GT(parsed, 0); } INSTANTIATE_TEST_SUITE_P( From 2a751e7b348c7037b4b98db18a6b95e73aa8e887 Mon Sep 17 00:00:00 2001 From: Paul Fournillon Date: Thu, 12 Nov 2026 19:28:29 +0100 Subject: [PATCH 13/15] test(elfparser): pin rela alignment and exact PLT address in misaligned symtab test (PROF-16551) --- ddprof-lib/src/main/cpp/libraries.h | 8 ++++++++ ddprof-lib/src/test/cpp/elfparser_ut.cpp | 15 +++++++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/ddprof-lib/src/main/cpp/libraries.h b/ddprof-lib/src/main/cpp/libraries.h index 5a22135330..8943c05d5a 100644 --- a/ddprof-lib/src/main/cpp/libraries.h +++ b/ddprof-lib/src/main/cpp/libraries.h @@ -99,6 +99,14 @@ class Libraries { // to addLibraryForTest(), so it must not be combined with caller-owned // caches in the same binary. Test teardown only (see // CodeCacheArray::freeForTest). + // + // LibraryPatcher's patch tables (_patched_entries, _sigaction_entries, + // _socket_entries) keep raw CodeCache pointers, and has_entry_for() matches + // them by address, so freeing would leave them dangling and let a later + // cache allocated at a reused address pass as already patched. Only call + // this in a binary that never patches: no LibraryPatcher::initialize() + // (patch_libraries() and patch_sigaction() return early without it), no + // install_socket_hooks(), and no refresher thread running. void freeNativeLibsForTest() { _native_libs.freeForTest(); } #endif diff --git a/ddprof-lib/src/test/cpp/elfparser_ut.cpp b/ddprof-lib/src/test/cpp/elfparser_ut.cpp index b4e2e851c8..5788747559 100644 --- a/ddprof-lib/src/test/cpp/elfparser_ut.cpp +++ b/ddprof-lib/src/test/cpp/elfparser_ut.cpp @@ -4,6 +4,7 @@ #include #include +#include "arch.h" #include "codeCache.h" #include "libraries.h" #include "symbols.h" @@ -636,6 +637,11 @@ TEST_F(ElfTest, symbolNameOffsetOutOfBounds) { // A .symtab misaligned by the 42-byte section-header string table (sh_offset // 490, not a multiple of alignof(Elf64_Sym)) must still resolve both symbols // and the PLT relocation name through loadSymbolTable()/addRelocationSymbols(). +// +// The misaligned reads are only detected by -fsanitize=alignment (the asan +// gtest config): with the alignment-safe copies reverted, that build aborts +// with "member access within misaligned address", while the debug build still +// passes because arm64 and x86-64 perform unaligned loads in hardware. TEST_F(ElfTest, symbolTableMisalignedEntries) { const uint16_t NSEC = 6; const uint64_t shoff = sizeof(Elf64_Ehdr); @@ -655,6 +661,10 @@ TEST_F(ElfTest, symbolTableMisalignedEntries) { const uint64_t str_off = sym_off + sizeof(sym); const char strtab[] = "\0mis_main\0mis_helper\0"; // 22 bytes: names at 1 and 10 const uint64_t rela_off = str_off + sizeof(strtab); + // ElfRelocation entries are still read through a cast pointer, so a + // misaligned .rela.plt would abort the asan build in addRelocationSymbols() + // before the symbol lookup under test; keep only .symtab misaligned. + ASSERT_EQ(rela_off % alignof(Elf64_Rela), 0u); Elf64_Rela rela; memset(&rela, 0, sizeof(rela)); rela.r_offset = 0x4000; @@ -711,8 +721,9 @@ TEST_F(ElfTest, symbolTableMisalignedEntries) { EXPECT_EQ(cc.findSymbol("mis_main"), (const void*)0x20000); EXPECT_EQ(cc.findSymbol("mis_helper"), (const void*)0x21000); // The PLT name came from the misaligned symbol-index lookup in - // addRelocationSymbols(): symbol 1 resolves to "mis_helper@plt". - EXPECT_NE(cc.findSymbol("mis_helper@plt"), nullptr); + // addRelocationSymbols(): symbol 1 resolves to "mis_helper@plt", placed at + // the first stub after the PLT header. + EXPECT_EQ(cc.findSymbol("mis_helper@plt"), (const void*)(0x3000 + PLT_HEADER_SIZE)); } #endif //__linux__ From 1b271e26280338a93313362552b1da7717db32a7 Mon Sep 17 00:00:00 2001 From: Paul Fournillon Date: Thu, 12 Nov 2026 19:55:39 +0100 Subject: [PATCH 14/15] docs: clarify freeForTest concurrency contract and patch-table matching (PROF-16551) --- ddprof-lib/src/main/cpp/codeCache.h | 5 +++-- ddprof-lib/src/main/cpp/libraries.h | 13 +++++++------ ddprof-lib/src/main/cpp/symbols_linux.cpp | 2 +- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/ddprof-lib/src/main/cpp/codeCache.h b/ddprof-lib/src/main/cpp/codeCache.h index e2636b8cca..d9a78d0e31 100644 --- a/ddprof-lib/src/main/cpp/codeCache.h +++ b/ddprof-lib/src/main/cpp/codeCache.h @@ -483,8 +483,9 @@ class CodeCacheArray { } #ifdef UNIT_TEST - // Deletes every registered CodeCache and resets the array to empty. Test - // teardown only: production arrays are append-only for process lifetime. + // Test teardown only: deletes every CodeCache and empties the array. + // Must not race add(): a publisher would spin on _count after the reset + // or store its pointer past the memset. void freeForTest() { for (int i = 0; i < count(); i++) { CodeCache *lib = at(i); diff --git a/ddprof-lib/src/main/cpp/libraries.h b/ddprof-lib/src/main/cpp/libraries.h index 8943c05d5a..25b0ac555a 100644 --- a/ddprof-lib/src/main/cpp/libraries.h +++ b/ddprof-lib/src/main/cpp/libraries.h @@ -101,12 +101,13 @@ class Libraries { // CodeCacheArray::freeForTest). // // LibraryPatcher's patch tables (_patched_entries, _sigaction_entries, - // _socket_entries) keep raw CodeCache pointers, and has_entry_for() matches - // them by address, so freeing would leave them dangling and let a later - // cache allocated at a reused address pass as already patched. Only call - // this in a binary that never patches: no LibraryPatcher::initialize() - // (patch_libraries() and patch_sigaction() return early without it), no - // install_socket_hooks(), and no refresher thread running. + // _socket_entries) keep raw CodeCache pointers, matched by address + // (has_entry_for() and the socket-entry scans), so freeing would leave them + // dangling and let a later cache allocated at a reused address pass as + // already patched. Only call this in a binary that never patches: no + // LibraryPatcher::initialize() (patch_libraries() and patch_sigaction() + // return early without it), no install_socket_hooks(), and no refresher + // thread running. void freeNativeLibsForTest() { _native_libs.freeForTest(); } #endif diff --git a/ddprof-lib/src/main/cpp/symbols_linux.cpp b/ddprof-lib/src/main/cpp/symbols_linux.cpp index 7b1103ae80..6b265e0508 100644 --- a/ddprof-lib/src/main/cpp/symbols_linux.cpp +++ b/ddprof-lib/src/main/cpp/symbols_linux.cpp @@ -329,7 +329,7 @@ static char _debuginfod_cache_buf[PATH_MAX] = {0}; static_assert(offsetof(ElfSymbol, st_name) == 0, "symbolNameAt() reads st_name at offset 0"); static uint32_t symbolNameAt(const void* sym) { uint32_t name; - memcpy(&name, sym, sizeof(name)); // st_name sits at offset 0 of Elf64_Sym + memcpy(&name, sym, sizeof(name)); return name; } From c3b5d8049c928416caa37d569aa0babce35ad672 Mon Sep 17 00:00:00 2001 From: Paul Fournillon Date: Thu, 12 Nov 2026 23:31:33 +0100 Subject: [PATCH 15/15] fix(symbols): copy ELF relocation entries out alignment-safely and bound relocation walks (PROF-16551) --- ddprof-lib/src/main/cpp/libraries.h | 16 +- ddprof-lib/src/main/cpp/symbols_linux.cpp | 66 ++++-- ddprof-lib/src/test/cpp/elfparser_ut.cpp | 273 ++++++++++++++++++++-- 3 files changed, 303 insertions(+), 52 deletions(-) diff --git a/ddprof-lib/src/main/cpp/libraries.h b/ddprof-lib/src/main/cpp/libraries.h index 25b0ac555a..78511d9e47 100644 --- a/ddprof-lib/src/main/cpp/libraries.h +++ b/ddprof-lib/src/main/cpp/libraries.h @@ -95,19 +95,9 @@ class Libraries { // reading the loaded libraries; compiled only into gtest binaries. bool addLibraryForTest(CodeCache *lib) { return _native_libs.add(lib); } - // Deletes every CodeCache in the process-wide array, including any passed - // to addLibraryForTest(), so it must not be combined with caller-owned - // caches in the same binary. Test teardown only (see - // CodeCacheArray::freeForTest). - // - // LibraryPatcher's patch tables (_patched_entries, _sigaction_entries, - // _socket_entries) keep raw CodeCache pointers, matched by address - // (has_entry_for() and the socket-entry scans), so freeing would leave them - // dangling and let a later cache allocated at a reused address pass as - // already patched. Only call this in a binary that never patches: no - // LibraryPatcher::initialize() (patch_libraries() and patch_sigaction() - // return early without it), no install_socket_hooks(), and no refresher - // thread running. + // Test teardown only; also deletes caches passed to addLibraryForTest(). + // LibraryPatcher's tables and updateBuildIds()' set match caches by address, + // so call it only in a binary that never patches nor updates build ids. void freeNativeLibsForTest() { _native_libs.freeForTest(); } #endif diff --git a/ddprof-lib/src/main/cpp/symbols_linux.cpp b/ddprof-lib/src/main/cpp/symbols_linux.cpp index 6b265e0508..d3f0345c36 100644 --- a/ddprof-lib/src/main/cpp/symbols_linux.cpp +++ b/ddprof-lib/src/main/cpp/symbols_linux.cpp @@ -323,9 +323,8 @@ typedef Elf64_Dyn ElfDyn; static char _debuginfod_cache_buf[PATH_MAX] = {0}; -// Symbol-table addresses (file sh_offset or DT_SYMTAB) carry no alignment -// guarantee; this copy avoids member access through a misaligned ElfSymbol*, -// which is undefined behavior. +// Nothing checks the alignment of sh_offset or DT_SYMTAB, so a crafted image can +// misalign the table; copying avoids UB from a misaligned ElfSymbol* access. static_assert(offsetof(ElfSymbol, st_name) == 0, "symbolNameAt() reads st_name at offset 0"); static uint32_t symbolNameAt(const void* sym) { uint32_t name; @@ -333,6 +332,15 @@ static uint32_t symbolNameAt(const void* sym) { return name; } +// Same for relocation tables (sh_offset, DT_JMPREL, DT_RELA/DT_REL). Copies the +// Elf64_Rel prefix, so it also serves Elf64_Rela; the caller must ensure +// sizeof(ElfRelocation) bytes remain in the table. +static ElfRelocation relocationAt(const void* rel) { + ElfRelocation r; + memcpy(&r, rel, sizeof(r)); + return r; +} + class ElfParser { private: CodeCache* _cc; @@ -686,37 +694,43 @@ void ElfParser::parseDynamicSection() { loadSymbolTable(symtab, syment * nsyms, syment, strtab, strsz); } - // base() is NULL for ET_EXEC (non-PIE) images - adding r->r_offset to it - // via pointer arithmetic is UB (base + r->r_offset on a null base), even + // base() is NULL for ET_EXEC (non-PIE) images - adding r.r_offset to it + // via pointer arithmetic is UB (base + r.r_offset on a null base), even // though the intent is just "sym addresses are already absolute". Do the // addition in integer space, same fix as the .plt case above. uintptr_t base_addr = (uintptr_t)this->base(); - if (jmprel != NULL && pltrelsz != 0) { + // Smaller entries cannot hold r_offset and r_info (hashImportLayout() + // skips them too). Walking only while a whole entry fits keeps offs + // within the table, so offs += relent cannot wrap. + if (jmprel != NULL && pltrelsz != 0 && relent >= sizeof(ElfRelocation)) { // Parse .rela.plt table - for (size_t offs = 0; offs < pltrelsz; offs += relent) { - ElfRelocation* r = (ElfRelocation*)(jmprel + offs); - uint32_t st_name = symbolNameAt(symtab + ELF_R_SYM(r->r_info) * syment); + for (size_t offs = 0; relent <= pltrelsz - offs; offs += relent) { + ElfRelocation r = relocationAt(jmprel + offs); + uint32_t st_name = symbolNameAt(symtab + ELF_R_SYM(r.r_info) * syment); if (st_name != 0) { const char* sym_name = strAt(strtab, strsz, st_name); if (sym_name != NULL) { - _cc->addImport((void**)(base_addr + r->r_offset), sym_name); + _cc->addImport((void**)(base_addr + r.r_offset), sym_name); } } } } - if (rel != NULL && relsz != 0) { + if (rel != NULL && relsz != 0 && relent >= sizeof(ElfRelocation)) { // Relocation entries for imports can be found in .rela.dyn, for example // if a shared library is built without PLT (-fno-plt). However, if both // entries exist, addImport saves them both. - for (size_t offs = relcount * relent; offs < relsz; offs += relent) { - ElfRelocation* r = (ElfRelocation*)(rel + offs); - if (ELF_R_TYPE(r->r_info) == R_GLOB_DAT || ELF_R_TYPE(r->r_info) == R_ABS64) { - uint32_t st_name = symbolNameAt(symtab + ELF_R_SYM(r->r_info) * syment); + // The leading DT_RELACOUNT entries are relative relocations, without + // a symbol; clamp the start as hashImportLayout() does. + size_t start = relcount * relent < relsz ? relcount * relent : relsz; + for (size_t offs = start; relent <= relsz - offs; offs += relent) { + ElfRelocation r = relocationAt(rel + offs); + if (ELF_R_TYPE(r.r_info) == R_GLOB_DAT || ELF_R_TYPE(r.r_info) == R_ABS64) { + uint32_t st_name = symbolNameAt(symtab + ELF_R_SYM(r.r_info) * syment); if (st_name != 0) { const char* sym_name = strAt(strtab, strsz, st_name); if (sym_name != NULL) { - _cc->addImport((void**)(base_addr + r->r_offset), sym_name); + _cc->addImport((void**)(base_addr + r.r_offset), sym_name); } } } @@ -1054,11 +1068,11 @@ void ElfParser::addRelocationSymbols(ElfSection* reltab, const char* plt) { // Offset-based iteration (see loadSymbolTable) so a huge rel_ent cannot // overflow the relocation pointer past the section end. for (size_t off = 0; rel_ent <= reltab_size - off; off += rel_ent, plt += PLT_ENTRY_SIZE) { - ElfRelocation* r = (ElfRelocation*)(relocations + off); - if (ELF_R_SYM(r->r_info) > max_sym_index) { + ElfRelocation r = relocationAt(relocations + off); + if (ELF_R_SYM(r.r_info) > max_sym_index) { continue; } - uint32_t st_name = symbolNameAt(symbols + (size_t)ELF_R_SYM(r->r_info) * sym_ent); + uint32_t st_name = symbolNameAt(symbols + (size_t)ELF_R_SYM(r.r_info) * sym_ent); char name[256]; if (st_name == 0) { @@ -1353,21 +1367,23 @@ class LoadedImage { // Hashes every relocation in rel (size bytes, entries relent apart) that names // a symbol: its offset, type and symbol index, and the symbol's name. Returns -// false if a symbol or its name lies outside the readable image. +// false if a symbol or its name lies outside the readable image. relent must be +// at least sizeof(ElfRelocation) (hashImportLayout() checks it), so every entry +// the walk visits holds a whole r_offset and r_info and the walk advances. bool hashSymbolRelocations(const LoadedImage& image, uintptr_t rel, size_t size, size_t relent, uintptr_t symtab, size_t syment, uintptr_t strtab, size_t strsz, u64& hash) { - for (size_t offs = 0; offs + sizeof(ElfRelocation) <= size; offs += relent) { - const ElfRelocation* r = (const ElfRelocation*)(rel + offs); - size_t index = ELF_R_SYM(r->r_info); + for (size_t offs = 0; relent <= size - offs; offs += relent) { + ElfRelocation r = relocationAt(reinterpret_cast(rel + offs)); + size_t index = ELF_R_SYM(r.r_info); if (index == 0) continue; // e.g. R_*_RELATIVE: no import uintptr_t sym = symtab + index * syment; if (!image.readable(sym, sizeof(ElfSymbol))) return false; size_t name = symbolNameAt(reinterpret_cast(sym)); if (name >= strsz) return false; const char* str = (const char*)(strtab + name); - hash = fnv1a(&r->r_offset, sizeof(r->r_offset), hash); - hash = fnv1a(&r->r_info, sizeof(r->r_info), hash); + hash = fnv1a(&r.r_offset, sizeof(r.r_offset), hash); + hash = fnv1a(&r.r_info, sizeof(r.r_info), hash); // Including the terminating NUL keeps adjacent names apart. hash = fnv1a(str, strnlen(str, strsz - name - 1) + 1, hash); } diff --git a/ddprof-lib/src/test/cpp/elfparser_ut.cpp b/ddprof-lib/src/test/cpp/elfparser_ut.cpp index 5788747559..a02dbedbc1 100644 --- a/ddprof-lib/src/test/cpp/elfparser_ut.cpp +++ b/ddprof-lib/src/test/cpp/elfparser_ut.cpp @@ -34,6 +34,7 @@ // Forward declaration for ElfParser functionality from symbols_linux.cpp class ElfParser { public: + static void parseProgramHeaders(CodeCache* cc, const char* base, const char* end, bool relocate_dyn); static bool parseFile(CodeCache* cc, const char* base, const char* file_name, bool use_debug); }; @@ -116,6 +117,7 @@ class ElfReladyn : public ::testing::Test { // lets the next parse (a later SetUp or a local array) reparse them. Symbols::clearParsingCaches(); _libs->freeNativeLibsForTest(); + _libreladyn = nullptr; // freed above; a mid-test TearDown() must not leave it dangling } CodeCache* libreladyn() { @@ -634,14 +636,9 @@ TEST_F(ElfTest, symbolNameOffsetOutOfBounds) { parseElfBytes(b); // must not crash: strAt() rejects st_name == strtab_size } -// A .symtab misaligned by the 42-byte section-header string table (sh_offset -// 490, not a multiple of alignof(Elf64_Sym)) must still resolve both symbols -// and the PLT relocation name through loadSymbolTable()/addRelocationSymbols(). -// -// The misaligned reads are only detected by -fsanitize=alignment (the asan -// gtest config): with the alignment-safe copies reverted, that build aborts -// with "member access within misaligned address", while the debug build still -// passes because arm64 and x86-64 perform unaligned loads in hardware. +// Misaligned .symtab and .rela.plt must still resolve symbols and the PLT name. +// Only -fsanitize=alignment (asan config) catches a regression: arm64 and +// x86-64 perform the unaligned loads in hardware. TEST_F(ElfTest, symbolTableMisalignedEntries) { const uint16_t NSEC = 6; const uint64_t shoff = sizeof(Elf64_Ehdr); @@ -660,11 +657,11 @@ TEST_F(ElfTest, symbolTableMisalignedEntries) { sym[1].st_size = 4; const uint64_t str_off = sym_off + sizeof(sym); const char strtab[] = "\0mis_main\0mis_helper\0"; // 22 bytes: names at 1 and 10 - const uint64_t rela_off = str_off + sizeof(strtab); - // ElfRelocation entries are still read through a cast pointer, so a - // misaligned .rela.plt would abort the asan build in addRelocationSymbols() - // before the symbol lookup under test; keep only .symtab misaligned. - ASSERT_EQ(rela_off % alignof(Elf64_Rela), 0u); + // 4 pad bytes push .rela.plt off its natural alignment too, so the + // relocation entry read in addRelocationSymbols() is misaligned as well. + const char rela_pad[4] = {0, 0, 0, 0}; + const uint64_t rela_off = str_off + sizeof(strtab) + sizeof(rela_pad); + ASSERT_NE(rela_off % alignof(Elf64_Rela), 0u); Elf64_Rela rela; memset(&rela, 0, sizeof(rela)); rela.r_offset = 0x4000; @@ -713,10 +710,11 @@ TEST_F(ElfTest, symbolTableMisalignedEntries) { app(shstrtab, sizeof(shstrtab)); app(sym, sizeof(sym)); app(strtab, sizeof(strtab)); + app(rela_pad, sizeof(rela_pad)); app(&rela, sizeof(rela)); CodeCache cc("regress"); - parseElfBytes(b, &cc, /*use_debug=*/true); + ASSERT_NO_FATAL_FAILURE(parseElfBytes(b, &cc, /*use_debug=*/true)); // Both misaligned .symtab entries resolved at their st_value addresses. EXPECT_EQ(cc.findSymbol("mis_main"), (const void*)0x20000); EXPECT_EQ(cc.findSymbol("mis_helper"), (const void*)0x21000); @@ -726,4 +724,251 @@ TEST_F(ElfTest, symbolTableMisalignedEntries) { EXPECT_EQ(cc.findSymbol("mis_helper@plt"), (const void*)(0x3000 + PLT_HEADER_SIZE)); } +// Loaded image with misaligned .dynsym, .rela.plt and .rela.dyn: both imports +// must resolve and both relocation tables must be hashed into the fingerprint. +// As above, only the asan config catches an alignment regression. +TEST_F(ElfTest, dynamicSectionMisalignedEntries) { +#if defined(__x86_64__) + const uint32_t glob_dat = R_X86_64_GLOB_DAT; +#elif defined(__aarch64__) + const uint32_t glob_dat = R_AARCH64_GLOB_DAT; +#else + GTEST_SKIP() << "R_GLOB_DAT imports are only recorded on x86-64 and aarch64"; + const uint32_t glob_dat = 0; +#endif + const size_t IMAGE_SIZE = 4096; + const uint64_t GOT_PLT_SLOT = 0x800; + const uint64_t GOT_DAT_SLOT = 0x808; + const int NPH = 2; + const int NDYN = 10; + + const uint64_t ph_off = sizeof(Elf64_Ehdr); + const uint64_t dyn_off = ph_off + NPH * sizeof(Elf64_Phdr); // ElfDyn stays aligned + const char strtab[] = "\0pthread_create\0pthread_setspecific"; // names at 1 and 16 + const uint64_t str_off = dyn_off + NDYN * sizeof(Elf64_Dyn); + const uint64_t sym_off = str_off + sizeof(strtab); + const uint64_t jmprel_off = sym_off + 3 * sizeof(Elf64_Sym); + const uint64_t rela_off = jmprel_off + sizeof(Elf64_Rela); + ASSERT_LE(rela_off + sizeof(Elf64_Rela), GOT_PLT_SLOT); + // Layout must stay misaligned for every table under test. + ASSERT_NE(sym_off % alignof(Elf64_Sym), 0u); + ASSERT_NE(jmprel_off % alignof(Elf64_Rela), 0u); + ASSERT_NE(rela_off % alignof(Elf64_Rela), 0u); + + // The image itself must be aligned so only the table offsets misalign. + std::unique_ptr mem(aligned_alloc(IMAGE_SIZE, IMAGE_SIZE), &free); + ASSERT_THAT(mem.get(), ::testing::NotNull()); + char* image = static_cast(mem.get()); + memset(image, 0, IMAGE_SIZE); + + Elf64_Ehdr e = validEhdr(); + e.e_phoff = ph_off; + e.e_phentsize = sizeof(Elf64_Phdr); + e.e_phnum = NPH; + memcpy(image, &e, sizeof(e)); + + Elf64_Phdr ph[NPH]; + memset(ph, 0, sizeof(ph)); + ph[0].p_type = PT_LOAD; + ph[0].p_filesz = IMAGE_SIZE; + ph[0].p_memsz = IMAGE_SIZE; + ph[1].p_type = PT_DYNAMIC; + ph[1].p_vaddr = dyn_off; + ph[1].p_offset = dyn_off; + ph[1].p_filesz = NDYN * sizeof(Elf64_Dyn); + ph[1].p_memsz = NDYN * sizeof(Elf64_Dyn); + memcpy(image + ph_off, ph, sizeof(ph)); + + // Unrelocated (vaddr) d_ptr values, as musl leaves them; both parsers + // rebase pointers that fall below the image. + const Elf64_Dyn dyn[NDYN] = { + {DT_SYMTAB, {sym_off}}, + {DT_STRTAB, {str_off}}, + {DT_SYMENT, {sizeof(Elf64_Sym)}}, + {DT_STRSZ, {sizeof(strtab)}}, + {DT_JMPREL, {jmprel_off}}, + {DT_PLTRELSZ, {sizeof(Elf64_Rela)}}, + {DT_RELA, {rela_off}}, + {DT_RELASZ, {sizeof(Elf64_Rela)}}, + {DT_RELAENT, {sizeof(Elf64_Rela)}}, + {DT_NULL, {0}}, + }; + memcpy(image + dyn_off, dyn, sizeof(dyn)); + memcpy(image + str_off, strtab, sizeof(strtab)); + + Elf64_Sym sym[3]; + memset(sym, 0, sizeof(sym)); + sym[1].st_name = 1; // "pthread_create" + sym[2].st_name = 16; // "pthread_setspecific" + memcpy(image + sym_off, sym, sizeof(sym)); + + Elf64_Rela jmprel; + memset(&jmprel, 0, sizeof(jmprel)); + jmprel.r_offset = GOT_PLT_SLOT; + jmprel.r_info = ELF64_R_INFO(1, 0); // .rela.plt entries are not type-checked + memcpy(image + jmprel_off, &jmprel, sizeof(jmprel)); + + Elf64_Rela rela; + memset(&rela, 0, sizeof(rela)); + rela.r_offset = GOT_DAT_SLOT; + rela.r_info = ELF64_R_INFO(2, glob_dat); + memcpy(image + rela_off, &rela, sizeof(rela)); + + CodeCache cc("inmem"); + ElfParser::parseProgramHeaders(&cc, image, image + IMAGE_SIZE, /*relocate_dyn=*/false); + + // parseDynamicSection(): both misaligned relocations resolved their + // misaligned symbols and recorded the GOT slot they name. + EXPECT_EQ(cc.peekImport(im_pthread_create), (void**)(image + GOT_PLT_SLOT)); + EXPECT_EQ(cc.peekImport(im_pthread_setspecific), (void**)(image + GOT_DAT_SLOT)); + + // hashSymbolRelocations(): a fingerprint of 0 means a table was rejected. + const Elf64_Phdr* phdrs = reinterpret_cast(image + ph_off); + const u64 fingerprint = Symbols::imageFingerprint(phdrs, NPH, (uintptr_t)image); + EXPECT_NE(fingerprint, 0u); + EXPECT_EQ(cc.imageFingerprint(), fingerprint); + + // Both misaligned relocation tables are part of the hash: moving either + // entry's GOT slot must change the fingerprint. + jmprel.r_offset = GOT_PLT_SLOT + 2 * sizeof(void*); + memcpy(image + jmprel_off, &jmprel, sizeof(jmprel)); + const u64 plt_moved = Symbols::imageFingerprint(phdrs, NPH, (uintptr_t)image); + EXPECT_NE(plt_moved, 0u); + EXPECT_NE(plt_moved, fingerprint); + + rela.r_offset = GOT_DAT_SLOT + 2 * sizeof(void*); + memcpy(image + rela_off, &rela, sizeof(rela)); + const u64 both_moved = Symbols::imageFingerprint(phdrs, NPH, (uintptr_t)image); + EXPECT_NE(both_moved, 0u); + EXPECT_NE(both_moved, plt_moved); + + // DT_RELASZ ends 8 bytes into a second entry whose GLOB_DAT r_info lies past + // the table; the in-table entry is not a GLOB_DAT, so any recorded import + // means the walk read past DT_RELASZ. + const int RELASZ_INDEX = 7; + ASSERT_EQ(dyn[RELASZ_INDEX].d_tag, DT_RELASZ); + ASSERT_LE(rela_off + sizeof(Elf64_Rela) + sizeof(Elf64_Rel), GOT_PLT_SLOT); + Elf64_Dyn truncated = {DT_RELASZ, {sizeof(Elf64_Rela) + sizeof(uint64_t)}}; + memcpy(image + dyn_off + RELASZ_INDEX * sizeof(Elf64_Dyn), &truncated, sizeof(truncated)); + rela.r_info = 0; + memcpy(image + rela_off, &rela, sizeof(rela)); + Elf64_Rel stray; + stray.r_offset = GOT_DAT_SLOT; + stray.r_info = ELF64_R_INFO(2, glob_dat); + memcpy(image + rela_off + sizeof(Elf64_Rela), &stray, sizeof(stray)); + CodeCache truncated_cc("truncated"); + ElfParser::parseProgramHeaders(&truncated_cc, image, image + IMAGE_SIZE, /*relocate_dyn=*/false); + EXPECT_EQ(truncated_cc.peekImport(im_pthread_setspecific), nullptr); +} + +// An undersized, a near-SIZE_MAX, and a table-truncating DT_RELAENT/PLTRELSZ: +// no walk may read an entry that does not fit in the table, record imports the +// fingerprint skips, or wrap its offset back before the table. +TEST_F(ElfTest, dynamicSectionInvalidRelocationEntrySize) { + const size_t IMAGE_SIZE = 4096; + const uint64_t GOT_PLT_SLOT = 0x800; + const int NPH = 2; + const int NDYN = 8; + + const uint64_t ph_off = sizeof(Elf64_Ehdr); + const uint64_t dyn_off = ph_off + NPH * sizeof(Elf64_Phdr); + const char strtab[] = "\0pthread_create"; + const uint64_t str_off = dyn_off + NDYN * sizeof(Elf64_Dyn); + const uint64_t sym_off = (str_off + sizeof(strtab) + 7) & ~(uint64_t)7; + const uint64_t jmprel_off = sym_off + 2 * sizeof(Elf64_Sym); + // Room for a full entry plus the 16 bytes the truncated-table case reads. + ASSERT_LE(jmprel_off + sizeof(Elf64_Rela) + sizeof(Elf64_Rel), GOT_PLT_SLOT); + + std::unique_ptr mem(aligned_alloc(IMAGE_SIZE, IMAGE_SIZE), &free); + ASSERT_THAT(mem.get(), ::testing::NotNull()); + char* image = static_cast(mem.get()); + memset(image, 0, IMAGE_SIZE); + + Elf64_Ehdr e = validEhdr(); + e.e_phoff = ph_off; + e.e_phentsize = sizeof(Elf64_Phdr); + e.e_phnum = NPH; + memcpy(image, &e, sizeof(e)); + + Elf64_Phdr ph[NPH]; + memset(ph, 0, sizeof(ph)); + ph[0].p_type = PT_LOAD; + ph[0].p_filesz = IMAGE_SIZE; + ph[0].p_memsz = IMAGE_SIZE; + ph[1].p_type = PT_DYNAMIC; + ph[1].p_vaddr = dyn_off; + ph[1].p_offset = dyn_off; + ph[1].p_filesz = NDYN * sizeof(Elf64_Dyn); + ph[1].p_memsz = NDYN * sizeof(Elf64_Dyn); + memcpy(image + ph_off, ph, sizeof(ph)); + + const int PLTRELSZ_INDEX = 5; + const int RELAENT_INDEX = 6; + const Elf64_Dyn dyn[NDYN] = { + {DT_SYMTAB, {sym_off}}, + {DT_STRTAB, {str_off}}, + {DT_SYMENT, {sizeof(Elf64_Sym)}}, + {DT_STRSZ, {sizeof(strtab)}}, + {DT_JMPREL, {jmprel_off}}, + {DT_PLTRELSZ, {sizeof(Elf64_Rela)}}, // PLTRELSZ_INDEX + {DT_RELAENT, {8}}, // RELAENT_INDEX: half an Elf64_Rel + {DT_NULL, {0}}, + }; + memcpy(image + dyn_off, dyn, sizeof(dyn)); + memcpy(image + str_off, strtab, sizeof(strtab)); + + Elf64_Sym sym[2]; + memset(sym, 0, sizeof(sym)); + sym[1].st_name = 1; // "pthread_create" + // The last 8 bytes before .rela.plt. Read as an r_info by a relocation walk + // that wraps back before the table, they name symbol 0xffffffff, which lies + // outside the image (see the huge-stride case below). + sym[1].st_size = 0xffffffff00000000ULL; + memcpy(image + sym_off, sym, sizeof(sym)); + + // A well-formed entry: with the 8-byte stride the first iteration still + // reads it whole, so only the entry-size check keeps it out. + Elf64_Rela jmprel; + memset(&jmprel, 0, sizeof(jmprel)); + jmprel.r_offset = GOT_PLT_SLOT; + jmprel.r_info = ELF64_R_INFO(1, 0); + memcpy(image + jmprel_off, &jmprel, sizeof(jmprel)); + + { + CodeCache cc("undersized"); + ElfParser::parseProgramHeaders(&cc, image, image + IMAGE_SIZE, /*relocate_dyn=*/false); + EXPECT_EQ(cc.peekImport(im_pthread_create), nullptr); + } + + // No entry fits, so nothing is recorded. A walk that wraps back before the + // table meets the index planted in sym[1].st_size and yields fingerprint 0. + Elf64_Dyn huge_relent = {DT_RELAENT, {SIZE_MAX - 7}}; + memcpy(image + dyn_off + RELAENT_INDEX * sizeof(Elf64_Dyn), &huge_relent, sizeof(huge_relent)); + { + CodeCache cc("huge"); + ElfParser::parseProgramHeaders(&cc, image, image + IMAGE_SIZE, /*relocate_dyn=*/false); + EXPECT_EQ(cc.peekImport(im_pthread_create), nullptr); + EXPECT_NE(cc.imageFingerprint(), 0u); + } + + // DT_PLTRELSZ ends 8 bytes into a second entry whose r_info lies past the + // table; the in-table entry names no symbol, so any recorded import means + // the walk read past DT_PLTRELSZ. + Elf64_Dyn relaent = {DT_RELAENT, {sizeof(Elf64_Rela)}}; + Elf64_Dyn truncated = {DT_PLTRELSZ, {sizeof(Elf64_Rela) + sizeof(uint64_t)}}; + memcpy(image + dyn_off + RELAENT_INDEX * sizeof(Elf64_Dyn), &relaent, sizeof(relaent)); + memcpy(image + dyn_off + PLTRELSZ_INDEX * sizeof(Elf64_Dyn), &truncated, sizeof(truncated)); + jmprel.r_info = 0; + memcpy(image + jmprel_off, &jmprel, sizeof(jmprel)); + Elf64_Rel stray; + stray.r_offset = GOT_PLT_SLOT; + stray.r_info = ELF64_R_INFO(1, 0); + memcpy(image + jmprel_off + sizeof(Elf64_Rela), &stray, sizeof(stray)); + { + CodeCache cc("truncated"); + ElfParser::parseProgramHeaders(&cc, image, image + IMAGE_SIZE, /*relocate_dyn=*/false); + EXPECT_EQ(cc.peekImport(im_pthread_create), nullptr); + } +} + #endif //__linux__