Fix incorrect file offset calculation in memory mapping

The current implementation incorrectly assumes that calculating a file
offset from a process memory address can be done using a simple
subtraction from the library's load address. This assumption doesn't
hold for binaries with non-standard ELF layouts, where PT_LOAD segments
may have different virtual address to file offset mappings.

Fix the issue by:
1. First converting the absolute process address to a library-relative
   offset by subtracting the library's load point in the process
2. Finding the PT_LOAD segment in the ELF file that contains this offset
3. Using the segment's p_vaddr and p_offset to calculate the correct
   file offset

To avoid performance penalties from repeatedly parsing ELF files, add
caching of PT_LOAD segments per library.

Example of what was wrong:
  old: file_offset = addr - lib_start
  new: file_offset = ((addr - lib_start) - segment->p_vaddr) + segment->p_offset

This fixes an issue where pystack would read from incorrect file offsets
when analyzing binaries compiled with non-standard layout options (e.g.,
when using the gold linker with custom flags).

Signed-off-by: Pablo Galindo <[email protected]>

Author: pablogsal

news/220.bugfix.rst

@@ -0,0 +1,5 @@
+Fix incorrect file offset calculation when analyzing ELF files with
+non-standard ELF layouts. Previously, pystack would fail to correctly analyze
+Python binaries that had non-standard ELF layouts (for example when compiled
+with certain linker options). The fix properly accounts for PT_LOAD segment
+mappings when calculating file offsets.

src/pystack/_pystack/mem.cpp

@@ -16,6 +16,9 @@
 
 namespace pystack {
 
+using elf_unique_ptr = std::unique_ptr<Elf, std::function<void(Elf*)>>;
+using file_unique_ptr = std::unique_ptr<FILE, std::function<int(FILE*)>>;
+
 static ssize_t
 _process_vm_readv(
         pid_t pid,
@@ -398,17 +401,62 @@ CorefileRemoteMemoryManager::StatusCode
 CorefileRemoteMemoryManager::getMemoryLocationFromCore(remote_addr_t addr, off_t* offset_in_file) const
 {
     auto corefile_it = std::find_if(d_vmaps.cbegin(), d_vmaps.cend(), [&](auto& map) {
-        return (map.Start() <= addr && addr < map.End()) && (map.FileSize() != 0 && map.Offset() != 0);
+        // When considering if the data is in the core file, we need to check if the address is
+        // within the chunk of the segment in the core file. map.End() corresponds
+        // to the end of the segment in memory when the process was alive but when the core was
+        // created not all that data will be in the core, so we need to use map.FileSize()
+        // to get the end of the segment in the core file.
+        uintptr_t fileEnd = map.Start() + map.FileSize();
+        return (map.Start() <= addr && addr < fileEnd) && (map.FileSize() != 0 && map.Offset() != 0);
     });
     if (corefile_it == d_vmaps.cend()) {
         return StatusCode::ERROR;
     }
 
-    unsigned long base = corefile_it->Offset() - corefile_it->Start();
+    off_t base = corefile_it->Offset() - corefile_it->Start();
     *offset_in_file = base + addr;
     return StatusCode::SUCCESS;
 }
 
+CorefileRemoteMemoryManager::StatusCode
+CorefileRemoteMemoryManager::initLoadSegments(const std::string& filename) const
+{
+    file_unique_ptr file(fopen(filename.c_str(), "r"), fclose);
+    if (!file || fileno(file.get()) == -1) {
+        return StatusCode::ERROR;
+    }
+
+    auto elf = elf_unique_ptr(elf_begin(fileno(file.get()), ELF_C_READ_MMAP, nullptr), elf_end);
+    if (!elf) {
+        return StatusCode::ERROR;
+    }
+
+    std::vector<ElfLoadSegment> segments;
+    size_t phnum;
+    if (elf_getphdrnum(elf.get(), &phnum) == 0) {
+        for (size_t i = 0; i < phnum; i++) {
+            GElf_Phdr phdr_mem;
+            GElf_Phdr* phdr = gelf_getphdr(elf.get(), i, &phdr_mem);
+            if (phdr == nullptr) {
+                LOG(WARNING) << "Failed to read program header " << i << " from " << filename.c_str()
+                             << " (" << elf_errmsg(elf_errno()) << ")";
+                continue;
+            }
+
+            if (phdr->p_type == PT_LOAD) {
+                segments.push_back(
+                        {.vaddr = phdr->p_vaddr, .offset = phdr->p_offset, .size = phdr->p_filesz});
+            }
+        }
+    }
+
+    if (!segments.empty()) {
+        d_elf_load_segments_cache[filename] = std::move(segments);
+        return StatusCode::SUCCESS;
+    }
+    return StatusCode::ERROR;
+}
+
 CorefileRemoteMemoryManager::StatusCode
 CorefileRemoteMemoryManager::getMemoryLocationFromElf(
         remote_addr_t addr,
@@ -418,12 +466,42 @@ CorefileRemoteMemoryManager::getMemoryLocationFromElf(
     auto shared_libs_it = std::find_if(d_shared_libs.cbegin(), d_shared_libs.cend(), [&](auto& map) {
         return map.start <= addr && addr <= map.end;
     });
+
     if (shared_libs_it == d_shared_libs.cend()) {
         return StatusCode::ERROR;
     }
+
     *filename = &shared_libs_it->filename;
-    *offset_in_file = addr - shared_libs_it->start;
-    return StatusCode::SUCCESS;
+
+    // Check if we have cached segments for this file
+    auto cache_it = d_elf_load_segments_cache.find(**filename);
+    if (cache_it == d_elf_load_segments_cache.end()) {
+        // Initialize segments if not in cache
+        if (initLoadSegments(**filename) != StatusCode::SUCCESS) {
+            return StatusCode::ERROR;
+        }
+        cache_it = d_elf_load_segments_cache.find(**filename);
+    }
+
+    // Get the load address of the elf file from its first segment
+    remote_addr_t elf_load_addr = cache_it->second[0].vaddr;
+
+    // Now relocate the address to the elf file
+    remote_addr_t symbol_vaddr = addr - shared_libs_it->start + elf_load_addr;
+
+    // Find the segment containing this address
+    for (const auto& segment : cache_it->second) {
+        if (symbol_vaddr >= segment.vaddr && symbol_vaddr < segment.vaddr + segment.size) {
+            *offset_in_file = (symbol_vaddr - segment.vaddr) + segment.offset;
+            return StatusCode::SUCCESS;
+        }
+    }
+
+    LOG(ERROR) << "Failed to find the correct segment for address " << std::hex << std::showbase << addr
+               << " (with vaddr offset " << symbol_vaddr << ")"
+               << " in file " << **filename;
+
+    return StatusCode::ERROR;
 }
 
 bool

src/pystack/_pystack/mem.h

@@ -207,6 +207,15 @@ class CorefileRemoteMemoryManager : public AbstractRemoteMemoryManager
         ERROR,
     };
 
+    struct ElfLoadSegment
+    {
+        GElf_Addr vaddr;
+        GElf_Off offset;
+        GElf_Xword size;
+    };
+    // Cache for PT_LOAD segments
+    mutable std::unordered_map<std::string, std::vector<ElfLoadSegment>> d_elf_load_segments_cache;
+
     // Data members
     std::shared_ptr<CoreFileAnalyzer> d_analyzer;
     std::vector<VirtualMap> d_vmaps;
@@ -220,5 +229,6 @@ class CorefileRemoteMemoryManager : public AbstractRemoteMemoryManager
             remote_addr_t addr,
             const std::string** filename,
             off_t* offset_in_file) const;
+    StatusCode initLoadSegments(const std::string& filename) const;
 };
 }  // namespace pystack

src/pystack/_pystack/process.cpp

@@ -750,6 +750,23 @@ AbstractProcessManager::findPythonVersion() const
     }
     int major = (version >> 24) & 0xFF;
     int minor = (version >> 16) & 0xFF;
+    int level = (version >> 4) & 0x0F;
+
+    if (major == 0 && minor == 0) {
+        LOG(DEBUG) << "Failed to determine Python version from symbols: empty data copied";
+        return {-1, -1};
+    }
+
+    if (major != 2 && major != 3) {
+        LOG(DEBUG) << "Failed to determine Python version from symbols: invalid major version";
+        return {-1, -1};
+    }
+
+    if (level != 0xA && level != 0xB && level != 0xC && level != 0xF) {
+        LOG(DEBUG) << "Failed to determine Python version from symbols: invalid release level";
+        return {-1, -1};
+    }
+
     LOG(DEBUG) << "Python version determined from symbols: " << major << "." << minor;
     return {major, minor};
 }