diff --git a/news/guard-page-cache.bugfix b/news/guard-page-cache.bugfix new file mode 100644 index 00000000..fc30c95f --- /dev/null +++ b/news/guard-page-cache.bugfix @@ -0,0 +1,2 @@ +Avoid repeated whole-map reads when a readable memory map contains an inaccessible guard page. +Reads of accessible addresses in the same map now bypass the cache after the first failed cache fill. diff --git a/src/pystack/_pystack.pyi b/src/pystack/_pystack.pyi index 64545320..3363f8bb 100644 --- a/src/pystack/_pystack.pyi +++ b/src/pystack/_pystack.pyi @@ -84,6 +84,12 @@ def get_process_threads_for_core( def get_bss_info(binary: Union[str, pathlib.Path]) -> Optional[Dict[str, Any]]: ... def _check_interpreter_shutdown(manager: ProcessManager) -> None: ... def is_eval_frame(symbol: str, python_version: Tuple[int, int]) -> bool: ... +def _copy_memory_for_testing( + pid: int, + map_start: int, + map_end: int, + reads: List[Tuple[int, int]], +) -> List[bytes]: ... def _normalize_threads_for_testing( thread_descs: List[Dict[str, Any]], native_mode: NativeReportingMode, diff --git a/src/pystack/_pystack/bindings.cpp b/src/pystack/_pystack/bindings.cpp index 562e38ae..98a64879 100644 --- a/src/pystack/_pystack/bindings.cpp +++ b/src/pystack/_pystack/bindings.cpp @@ -1046,6 +1046,28 @@ NB_MODULE(_pystack, m) "python_version"_a, "Return True if the symbol is a CPython eval frame function"); + m.def( + "_copy_memory_for_testing", + [](pid_t pid, + pystack::remote_addr_t map_start, + pystack::remote_addr_t map_end, + const std::vector>& reads) { + std::vector maps; + maps.emplace_back(map_start, map_end, map_end - map_start, "rw-p", 0, "00:00", 0, ""); + pystack::ProcessMemoryManager manager(pid, maps); + nb::list result; + for (const auto& [address, size] : reads) { + std::vector buffer(size); + manager.copyMemoryFromProcess(address, size, buffer.data()); + result.append(nb::bytes(buffer.data(), buffer.size())); + } + return result; + }, + "pid"_a, + "map_start"_a, + "map_end"_a, + "reads"_a); + m.def( "_normalize_threads_for_testing", [](nb::list thread_descs, diff --git a/src/pystack/_pystack/mem.cpp b/src/pystack/_pystack/mem.cpp index 0b6f0473..7d873bca 100644 --- a/src/pystack/_pystack/mem.cpp +++ b/src/pystack/_pystack/mem.cpp @@ -256,7 +256,9 @@ ProcessMemoryManager::copyMemoryFromProcess(remote_addr_t addr, size_t len, void return vmap.containsAddr(addr) && vmap.containsAddr(addr + len - 1); }); - if (vmap == d_vmaps.end() || !d_lru_cache.can_fit(vmap->Size())) { + if (vmap == d_vmaps.end() || !d_lru_cache.can_fit(vmap->Size()) + || d_uncacheable_vmaps.contains(vmap->Start())) + { return readChunk(addr, len, reinterpret_cast(dst)); } @@ -272,7 +274,8 @@ ProcessMemoryManager::copyMemoryFromProcess(remote_addr_t addr, size_t len, void d_lru_cache.put(key, std::move(buf)); } catch (const InvalidRemoteAddress&) { // The full vmap read failed (e.g. guard pages in JIT mappings). - // Fall back to reading just the requested bytes directly. + // Future reads from this vmap should avoid the same failing cache fill. + d_uncacheable_vmaps.insert(key); return readChunk(addr, len, reinterpret_cast(dst)); } } diff --git a/src/pystack/_pystack/mem.h b/src/pystack/_pystack/mem.h index 64409ebc..1d31a1ba 100644 --- a/src/pystack/_pystack/mem.h +++ b/src/pystack/_pystack/mem.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include "elf_common.h" @@ -174,6 +175,7 @@ class ProcessMemoryManager : public AbstractRemoteMemoryManager pid_t d_pid; std::vector d_vmaps; mutable LRUCache d_lru_cache; + mutable std::unordered_set d_uncacheable_vmaps; mutable file_unique_ptr d_memfile; // Methods diff --git a/tests/unit/test_memory_manager.py b/tests/unit/test_memory_manager.py new file mode 100644 index 00000000..c0e9c453 --- /dev/null +++ b/tests/unit/test_memory_manager.py @@ -0,0 +1,37 @@ +import ctypes +import mmap +import os + +import pytest + +from pystack._pystack import _copy_memory_for_testing + + +def test_reads_accessible_memory_from_map_with_guard_page( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.delenv("_PYSTACK_NO_PROCESS_VM_READV", raising=False) + page_size = mmap.PAGESIZE + mapping = mmap.mmap(-1, page_size * 3) + address = ctypes.addressof(ctypes.c_char.from_buffer(mapping)) + expected = b"pthread data" + requested_address = address + page_size + mapping[page_size : page_size + len(expected)] = expected + + libc = ctypes.CDLL(None, use_errno=True) + mprotect = libc.mprotect + mprotect.argtypes = [ctypes.c_void_p, ctypes.c_size_t, ctypes.c_int] + mprotect.restype = ctypes.c_int + assert mprotect(address, page_size, 0) == 0 + try: + result = _copy_memory_for_testing( + os.getpid(), + address, + address + len(mapping), + [(requested_address, len(expected)), (requested_address, len(expected))], + ) + finally: + assert mprotect(address, page_size, mmap.PROT_READ | mmap.PROT_WRITE) == 0 + mapping.close() + + assert result == [expected, expected]