Fix incorrect file offset calculation in memory mapping #220

pablogsal · 2025-01-21T16:36:01Z

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:

First converting the absolute process address to a library-relative
offset by subtracting the library's load point in the process
Finding the PT_LOAD segment in the ELF file that contains this offset
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).

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]>

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

codecov-commenter · 2025-01-21T21:37:58Z

Codecov Report

Attention: Patch coverage is 67.34694% with 16 lines in your changes missing coverage. Please review.

Project coverage is 83.41%. Comparing base (19b9759) to head (a2bd7ae).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/pystack/_pystack/mem.cpp	76.74%	10 Missing ⚠️
src/pystack/_pystack/process.cpp	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #220      +/-   ##
==========================================
- Coverage   83.51%   83.41%   -0.11%     
==========================================
  Files          46       46              
  Lines        6201     6246      +45     
  Branches      134      459     +325     
==========================================
+ Hits         5179     5210      +31     
- Misses       1020     1036      +16     
+ Partials        2        0       -2

Flag	Coverage Δ
cpp	`83.41% <67.34%> (+18.97%)`	⬆️
python_and_cython	`83.41% <67.34%> (-15.66%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

src/pystack/_pystack/mem.cpp

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

godlygeek

The commit messages for the fighting-the-CI commits need rewording to explain the rationale for the changes. Other than that...

src/pystack/_pystack/mem.cpp

godlygeek · 2025-01-24T23:29:43Z

src/pystack/_pystack/mem.cpp

+        // withing 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();


Does this imply that fileEnd <= map.End()? If so, should we be checking that condition as well?

Yep, do you want an assert for this?

I was mostly checking my understanding, but I wouldn't be opposed to an assert

godlygeek · 2025-01-24T23:33:38Z

src/pystack/_pystack/process.cpp

+    if (major != 2 && major != 3) {
+        LOG(DEBUG) << "Failed to determine Python version from symbols: invalid major version";
+        return {-1, -1};
+    }


Should we also check whether PY_RELEASE_LEVEL is in {0xA, 0xB, 0xC, 0xF} while we're at it? The valid values for that are almost as constrained as for the major version.

godlygeek · 2025-01-24T23:38:12Z

src/pystack/_pystack/mem.h

+        GElf_Xword size;
+    };
+    // Cache for PT_LOAD segments
+    mutable std::unordered_map<std::string, std::vector<ElfLoadSegment>> d_elf_load_segments_cache;


I don't love this being mutable, but, fine... I guess you're delaying populating this cache instead of doing it when the memory manager is constructed to avoid paying the cost until the first time memory is actually read from a given shared library?

godlygeek · 2025-01-24T23:41:21Z

src/pystack/_pystack/mem.cpp

+    }
+
+    elf_end(elf);
+    close(fd);


Doesn't elf_end close the file descriptor? I thought a successful elf_begin takes over ownership of it, so this seems like a double close to me. Should be easy to confirm that by checking the return code...

It doesn;t: https://github.com/roolebo/elfutils/blob/cff53f1784c9a4344604bedf41b7d499b3eb30d5/libelf/elf_end.c#L42-L238

Ah, you're right. I misread some of our other code, which uses elf_unique_ptr to guarantee that elf_end gets called, and then doesn't call close after that point - but that's because it also uses file_unique_ptr, which always calls close, and I missed it because it was hidden in a destructor.

That said: maybe we should be reusing those two helper types, since we use them elsewhere.

godlygeek · 2025-01-24T23:41:59Z

src/pystack/_pystack/mem.cpp

+        return StatusCode::ERROR;
+    }
+
+    Elf* elf = elf_begin(fd, ELF_C_READ, nullptr);


Why ELF_C_READ? We use ELF_C_READ_MMAP everywhere else.

godlygeek · 2025-01-24T23:45:35Z

src/pystack/_pystack/mem.cpp

+        for (size_t i = 0; i < phnum; i++) {
+            GElf_Phdr phdr_mem;
+            GElf_Phdr* phdr = gelf_getphdr(elf, i, &phdr_mem);
+            if (phdr == nullptr) continue;


Isn't this just silently ignoring a legitimate error?

godlygeek · 2025-01-24T23:50:59Z

src/pystack/_pystack/mem.cpp

+    if (cache_it->second.empty()) {
+        return StatusCode::ERROR;
+    }


This case can never happen. Only initLoadSegments can add a key/value pair to the cache, and it returns an error if the value it would add is empty.

godlygeek · 2025-01-25T00:00:56Z

src/pystack/_pystack/mem.cpp

+    // Find the correct segment
+    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;
+        }
+    }


So we've got d_vmaps which is a vector of files and their offsets in virtual memory, and we iterate over it to find the file that contains a given virtual address, then we look up the vector of segments contained in that file, and iterate over it to find the segment that contains the given virtual address.

Wouldn't everything be simpler if, instead of introducing a second level of vectors, we flattened things so that we keep a vector of segments as a member variable instead of a vector of ELF files?

The only advantage I can think of for how you've done it is that it's lazier and allows delaying reading the segments for an ELF file until the first time they're needed. But, isn't reading the program headers quite fast? At least there's not much seeking involved, nor copying of strings, just reading a bunch of integers.

I don't think I follow. These are different things:

d_vmaps is not a vector of files and their offsets: is a vector of memory segments (that some are backed by files) as seen by the core: this is what the core has mapped inside of it. Some data may be missing.

What we are storing in the cache bt initLoadSegments and we are iterating here are the segments in the ELF files that have been mapped to the process when it was alive. Some of the data in these segments may be in the core and some may not. The addresses in these segments are not relocated to the process.

First, we look at d_vmaps and we look for the file that contains a given virtual address that'a absolute (an address in the process memory), and then we look up the ELF segments for that file but what we search for is an address that's not absolute anymore because we have subtracted the load point of the library.

So not sure I understand the proposal to flatten. There is nothing obvious that I can think of that we can flatten.

We just talked offline, but for the sake of anyone who finds the PR later:

The thing that we could represent as a flat structure is a list of virtual address ranges, and, for each range, the file that should be read to find its contents and the offset to begin reading at to find the start of the range. This would let us stop treating from-core and from-executable as two separate cases, and use exactly the same code to handle them, at the cost of extra time in the constructor to build this mapping.

We argued back and forth a bit about the pros and cons of the two approaches, and agreed to try to pair program the other approach on Monday to see if it's as clean as I think it'll be or as slow and ugly as Pablo thinks it will be 😆

Co-authored-by: Matt Wozniski <[email protected]> Signed-off-by: Pablo Galindo Salgado <[email protected]>

pablogsal changed the title ~~elf fix core~~ Fix incorrect file offset calculation in memory mapping Jan 21, 2025

pablogsal mentioned this pull request Jan 21, 2025

Version check makes potentially invalid assumptions about ELF layout #174

Open

1 task

pablogsal force-pushed the elf_fix_core branch 6 times, most recently from 1ec33d2 to a74a66e Compare January 21, 2025 18:54

pablogsal added 2 commits January 21, 2025 19:20

Fix mypy errors

09efbcd

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

pablogsal force-pushed the elf_fix_core branch 3 times, most recently from 1b17fec to 3b04fd4 Compare January 21, 2025 21:34

pablogsal force-pushed the elf_fix_core branch from 3b04fd4 to 5644a23 Compare January 21, 2025 21:58

Unpin pyinstaller

813c89f

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

pablogsal force-pushed the elf_fix_core branch from 5644a23 to 813c89f Compare January 21, 2025 23:12

pablogsal added 2 commits January 24, 2025 22:19

fixup! Unpin pyinstaller

d1e586c

fixup! fixup! Unpin pyinstaller

a682000

pablogsal commented Jan 24, 2025

View reviewed changes

src/pystack/_pystack/mem.cpp Outdated Show resolved Hide resolved

Update src/pystack/_pystack/mem.cpp

f3f8a62

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

godlygeek requested changes Jan 25, 2025

View reviewed changes

pablogsal and others added 2 commits January 25, 2025 00:48

Update src/pystack/_pystack/mem.cpp

eb2d045

Co-authored-by: Matt Wozniski <[email protected]> Signed-off-by: Pablo Galindo Salgado <[email protected]>

Update src/pystack/_pystack/mem.cpp

a2bd7ae

Co-authored-by: Matt Wozniski <[email protected]> Signed-off-by: Pablo Galindo Salgado <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect file offset calculation in memory mapping #220

Fix incorrect file offset calculation in memory mapping #220

pablogsal commented Jan 21, 2025 •

edited

Loading

codecov-commenter commented Jan 21, 2025 •

edited

Loading

godlygeek left a comment

godlygeek Jan 24, 2025

pablogsal Jan 25, 2025

godlygeek Jan 25, 2025

godlygeek Jan 24, 2025

godlygeek Jan 24, 2025

godlygeek Jan 24, 2025

pablogsal Jan 25, 2025

godlygeek Jan 25, 2025

godlygeek Jan 24, 2025

godlygeek Jan 24, 2025

godlygeek Jan 24, 2025

godlygeek Jan 25, 2025 •

edited

Loading

godlygeek Jan 25, 2025 •

edited

Loading

pablogsal Jan 25, 2025

godlygeek Jan 25, 2025

Fix incorrect file offset calculation in memory mapping #220

Are you sure you want to change the base?

Fix incorrect file offset calculation in memory mapping #220

Conversation

pablogsal commented Jan 21, 2025 • edited Loading

codecov-commenter commented Jan 21, 2025 • edited Loading

Codecov Report

godlygeek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

godlygeek Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

godlygeek Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablogsal commented Jan 21, 2025 •

edited

Loading

codecov-commenter commented Jan 21, 2025 •

edited

Loading

godlygeek Jan 25, 2025 •

edited

Loading

godlygeek Jan 25, 2025 •

edited

Loading