Fix handling duplicate symbols when libraries dlopen Python

When libraries like `llvmlite` `dlopen` the Python binary, it creates
duplicate mappings of the binary in the process memory. This caused
PyStack to use the wrong address for `_PyRuntime`, leading to
"Invalid address in remote process" errors.

The fix ensures we ignore any symbols which aren't actually located
within the mapped memory region. This works because when llvmlite makes
the call into `ffi_closure_alloc`, it doesn't map a full copy of the
interpreter, but only the portion of the text segment that it needs for
its trampoline, and so we can recognize that the mapping can't possibly
contain the symbol we're looking for.

Signed-off-by: Pablo Galindo Salgado <pablogsal@gmail.com>
Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>

Author: pablogsal

news/258.bugfix.rst

@@ -0,0 +1 @@
+Fix handling of duplicate ``_PyRuntime`` symbols when libraries like llvmlite dlopen the Python binary, which would cause "Invalid address in remote process" errors.

src/pystack/_pystack/unwinder.cpp

@@ -365,7 +365,13 @@ module_callback(
                    << module_arg->modulename;
         return DWARF_CB_OK;
     }
-    LOG(INFO) << "Attempting to find symbol '" << module_arg->symbol << "' in " << name;
+
+    Dwarf_Addr mapping_start;
+    Dwarf_Addr mapping_end;
+    dwfl_module_info(mod, nullptr, &mapping_start, &mapping_end, nullptr, nullptr, nullptr, nullptr);
+
+    LOG(INFO) << "Attempting to find symbol '" << module_arg->symbol << "' in " << name
+              << " mapping from " << std::hex << std::showbase << mapping_start << " to " << mapping_end;
     int n_syms = dwfl_module_getsymtab(mod);
     if (n_syms == -1) {
         return DWARF_CB_OK;
@@ -375,9 +381,15 @@ module_callback(
     for (int i = 0; i < n_syms; i++) {
         const char* sname = dwfl_module_getsym_info(mod, i, &sym, &addr, nullptr, nullptr, nullptr);
         if (strcmp(sname, module_arg->symbol) == 0) {
-            module_arg->addr = addr;
-            LOG(INFO) << "Symbol '" << sname << "' found at address " << std::hex << std::showbase
-                      << addr;
+            if (mapping_start <= addr && addr < mapping_end) {
+                module_arg->addr = addr;
+                LOG(INFO) << "Symbol '" << sname << "' found at address " << std::hex << std::showbase
+                          << addr;
+            } else {
+                LOG(INFO) << "Ignoring symbol address " << std::hex << std::showbase << addr
+                          << " outside the mapping [" << mapping_start << ", " << mapping_end << ")";
+                abort();
+            }
             break;
         }
     }

tests/integration/ctypes_program.py

@@ -0,0 +1,24 @@
+import ctypes
+import sys
+import time
+
+
+def first_func():
+    second_func()
+
+
+def second_func():
+    third_func()
+
+
+def third_func():
+    # Trigger libffi to re-import the Python binary
+    global gil_check
+    gil_check = ctypes.CFUNCTYPE(ctypes.c_int)(ctypes.CDLL(None).PyGILState_Check)
+
+    with open(sys.argv[1], "w") as fifo:
+        fifo.write("ready")
+    time.sleep(1000)
+
+
+first_func()

tests/integration/test_shenanigans.py

@@ -0,0 +1,35 @@
+import sys
+from pathlib import Path
+
+from pystack.engine import get_process_threads
+from tests.utils import spawn_child_process
+
+TEST_DUPLICATE_SYMBOLS_FILE = Path(__file__).parent / "ctypes_program.py"
+
+
+def test_duplicate_pyruntime_symbol_handling(tmpdir):
+    """Test that pystack correctly handles duplicate _PyRuntime symbols.
+
+    This can occur when ctypes uses libffi to dlopen the Python binary
+    in order to create a trampoline (which it only does if the Python binary
+    was statically linked against libpython).
+    """
+    # GIVEN
+    with spawn_child_process(
+        sys.executable, TEST_DUPLICATE_SYMBOLS_FILE, tmpdir
+    ) as child_process:
+        # WHEN
+        threads = list(get_process_threads(child_process.pid, stop_process=True))
+
+    # THEN
+    # We should have successfully resolved threads without "Invalid address" errors
+    assert threads is not None
+    assert len(threads) > 0
+
+    # Verify we can get stack traces (which requires correct _PyRuntime)
+    for thread in threads:
+        # Just ensure we can get frames without crashing
+        frames = list(thread.frames)
+        # The main thread should have at least one frame
+        if thread.tid == child_process.pid:
+            assert len(frames) > 0