fix: alignment-safe symbol and relocation reads in the ELF parser (PROF-16551)

.gitignore

@@ -32,6 +32,7 @@ datadog/maven/resources
 **/launcher*
 /gradle.properties
 **/hs_err*
+*.core
 
 # cursor AI history
 .history

ddprof-lib/src/main/cpp/codeCache.h

@@ -481,6 +481,26 @@ class CodeCacheArray {
     }
     return total;
   }
+
+#ifdef UNIT_TEST
+  // 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);
+      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;
+    _count = 0;
+    _overflow_reported = false;
+  }
+#endif
 };
 
 #endif // _CODECACHE_H

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,11 @@ 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); }
+
+  // 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
 
   // Delete copy constructor and assignment operator to prevent copies

ddprof-lib/src/main/cpp/symbols_linux.cpp

@@ -7,6 +7,7 @@
 #ifdef __linux__
 
 #include <dlfcn.h>
+#include <stddef.h>
 #include <unordered_map>
 #include <unordered_set>
 #include <stdio.h>
@@ -322,6 +323,24 @@ typedef Elf64_Dyn  ElfDyn;
 
 static char _debuginfod_cache_buf[PATH_MAX] = {0};
 
+// 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;
+    memcpy(&name, sym, sizeof(name));
+    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;
@@ -675,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);
-                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);
+            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) {
-                    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);
+            // 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);
                         }
                     }
                 }
@@ -1043,17 +1068,17 @@ 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;
         }
-        ElfSymbol* sym = (ElfSymbol*)(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
             }
@@ -1342,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<const void*>(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 = ((const ElfSymbol*)sym)->st_name;
+        size_t name = symbolNameAt(reinterpret_cast<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);
-        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);
     }

ddprof-lib/src/test/cpp/codeCache_ut.cpp

@@ -6,6 +6,7 @@
 #include <gtest/gtest.h>
 #include "codeCache.h"
 
+#include <memory>
 #include <stdio.h>
 
 // memoryUsage() must reflect the actual per-symbol name length, not a fixed
@@ -81,25 +82,76 @@ 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<CodeCache>("liba");
+    auto b = std::make_unique<CodeCache>("libb");
     // Populate before registering: add() must run pre-publication.
     a->add(reinterpret_cast<const void *>(0x1000), 16, "alpha_symbol");
     b->add(reinterpret_cast<const void *>(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;
+// 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);
+    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<CodeCache>("cleanup_liba");
+        auto b = std::make_unique<CodeCache>("cleanup_libb_with_longer_name");
+        a->add(reinterpret_cast<const void *>(0x1000), 16, "alpha_symbol");
+        b->add(reinterpret_cast<const void *>(0x2000), 32, "beta_symbol_longer_name");
+        b->add(reinterpret_cast<const void *>(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<long long>(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