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.

It happens to work if we stop searching as soon as we find the symbol,
rather than continuing to iterate over other loaded modules searching
for the symbol. This counts on the earliest mapping for the interpreter
binary being the initial mapping made by the loader.

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

Author: pablogsal

news/258.bugfix.rst

@@ -0,0 +1 @@
+Fix handling of duplicate ``_PyRuntime`` symbols when ctypes mmaps a statically linked Python interpreter's binary in order to create a trampoline. Previously this led to "Invalid address in remote process" errors.

src/pystack/_pystack/unwinder.cpp

@@ -378,7 +378,7 @@ module_callback(
             module_arg->addr = addr;
             LOG(INFO) << "Symbol '" << sname << "' found at address " << std::hex << std::showbase
                       << addr;
-            break;
+            return DWARF_CB_ABORT;
         }
     }
     return DWARF_CB_OK;
@@ -389,7 +389,7 @@ AbstractUnwinder::getAddressforSymbol(const std::string& symbol, const std::stri
 {
     LOG(DEBUG) << "Trying to find address for symbol " << symbol;
     ModuleArg arg = {symbol.c_str(), modulename.c_str(), 0};
-    if (dwfl_getmodules(Dwfl(), module_callback, &arg, 0) != 0) {
+    if (dwfl_getmodules(Dwfl(), module_callback, &arg, 0) == -1) {
         throw UnwinderError("Failed to fetch modules!");
     }
     LOG(DEBUG) << "Address for symbol " << symbol << " resolved to: " << std::hex << std::showbase

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