Fallback to /proc/PID/mem when process_vm_readv not in kernel

[cakemanny]

Resolves #238

Describe your changes
I've added an alternative implementation of ProcessMemoryManager::readChunk that uses the relevant /proc/PID/mem file and is fallen back to when process_vm_readv fails with ENOSYS.
Once the /proc/PID/mem file is open, subsequent reads are done only from there.

Testing performed

On my dev instance where my kernel does have CONFIG_CROSS_MEMORY_ATTACH set, I did various tests with small source changes. Such as forcing the fallback implementation to always be used.

Details

diff --git a/src/pystack/_pystack/mem.cpp b/src/pystack/_pystack/mem.cpp
index e25c69f..5deda3d 100644
--- a/src/pystack/_pystack/mem.cpp
+++ b/src/pystack/_pystack/mem.cpp
@@ -229,11 +229,15 @@ ProcessMemoryManager::ProcessMemoryManager(pid_t pid)
 ssize_t
 ProcessMemoryManager::readChunk(remote_addr_t addr, size_t len, char* dst) const
 {
+#if 0
     if (d_memfile.is_open()) {
         return readChunkThroughMemFile(addr, len, dst);
     } else {
         return readChunkDirect(addr, len, dst);
     }
+#else
+        return readChunkThroughMemFile(addr, len, dst);
+#endif
 }
 
 ssize_t

dan:~/src/pystack (support-missing-process_vm_readv) % python3 -c $'import time\nwhile True: time.sleep(1)' &
[1] 27161
dan:~/src/pystack (support-missing-process_vm_readv) % pystack remote $(pgrep python3)                       
Traceback for thread 27161 (python3) [] (most recent call last):
    (Python) File "<string>", line 2, in <module>

dan:~/src/pystack (support-missing-process_vm_readv) % pytest
===================================== test session starts =====================================
platform linux -- Python 3.11.2, pytest-8.3.5, pluggy-1.6.0
rootdir: /home/dan/src/pystack
...
=============================== 459 passed, 3 skipped in 15.75s ===============================

I also checked what the various errors might look like, say if the file didn't exist

@@ -273,7 +277,7 @@ ssize_t
 ProcessMemoryManager::readChunkThroughMemFile(remote_addr_t addr, size_t len, char* dst) const
 {
     if (!d_memfile.is_open()) {
-        std::string filepath = "/proc/" + std::to_string(d_pid) + "/mem";
+        std::string filepath = "/proc/" + std::to_string(d_pid) + "/memXXX";
         d_memfile.open(filepath, std::ifstream::binary);
         if (!d_memfile) {
             LOG(ERROR) << "Failed to open file " << filepath;

dan:~/src/pystack (support-missing-process_vm_readv) % pystack remote $(pgrep python3)
ERROR(process_remote): Failed to open file /proc/27161/memXXX
💀 Engine error: No such file or directory 💀

dan:~/src/pystack (support-missing-process_vm_readv) %

Once I was pretty happy that it wasn't too awful, I spun up a flatcar VM and tested the happy path there.

Details (Probably too many)

Per https://www.flatcar.org/docs/latest/setup/releases/verify-images/

curl -L -O https://www.flatcar.org/security/image-signing-key/Flatcar_Image_Signing_Key.asc
gpg --import --keyid-format LONG Flatcar_Image_Signing_Key.asc

cat  << 'EOF' > ssh_config
Host flatcar
HostName localhost
Port 2222
User core
StrictHostKeyChecking no
UserKnownHostsFile /dev/null
EOF
cat << 'EOF' > acquire.sh 
#!/bin/sh

set -eu

#current=4152.2.3
current=current

base_name=flatcar_production_qemu_uefi

for suff in .sh _image.img _efi_code.fd _efi_vars.fd; do
  wget https://lts.release.flatcar-linux.net/arm64-usr/$current/${base_name}${suff}
  wget https://lts.release.flatcar-linux.net/arm64-usr/$current/${base_name}${suff}.sig
  gpg --verify ${base_name}${suff}.sig
done

chmod +x ${base_name}.sh
EOF

sh ./acquire.sh

patch <<'EOF'
diff --git a/flatcar_production_qemu_uefi.sh b/flatcar_production_qemu_uefi.sh
index e97ba31..0e31ba9 100755
--- a/flatcar_production_qemu_uefi.sh
+++ b/flatcar_production_qemu_uefi.sh
@@ -220,6 +220,8 @@ else
             set -- -machine q35 -cpu kvm64 -smp 1 -nographic "$@" ;;
         arm64-usr+aarch64)
             set -- -machine virt,accel=kvm,gic-version=3 -cpu host -smp "${VM_NCPUS}" -nographic "$@" ;;
+        arm64-usr+arm64)
+            set -- -machine virt,accel=hvf -cpu host -smp "${VM_NCPUS}" -nographic "$@" ;;
         arm64-usr+*)
             if test "${VM_NCPUS}" -gt 4 ; then
                 VM_NCPUS=4
EOF

./flatcar_production_qemu_uefi.sh -nographic

ssh -F ssh_config flatcar

core@localhost ~ $ docker run -it --rm python:3.12 bash
Unable to find image 'python:3.12' locally
...
Status: Downloaded newer image for python:3.12
root@946ac0b130f3:/# # fix broken dns
root@946ac0b130f3:/# newresolve=$(sed $'/nameserver/i\\\nnameserver 1.1.1.1' /etc/resolv.conf); echo "$newresolve" > /etc/resolv.conf
root@946ac0b130f3:/# apt-get update && apt-get install libdw-dev libelf-dev pkg-config
...
root@946ac0b130f3:/# python -m pip install "pystack @ git+https://github.com/cakemanny/pystack.git@support-missing-process_vm_readv"
...
root@dce305e0c604:/# pystack
usage: pystack [-h] [-v] [--version] [--no-color] {remote,core} ...
pystack: error: the following arguments are required: command
root@dce305e0c604:/# python3 -c $'import time\nwhile True: time.sleep(1)' &
[1] 260
root@dce305e0c604:/# pystack remote 260
Traceback for thread 260 (python3) [] (most recent call last):
    (Python) File "<string>", line 2, in <module>

root@dce305e0c604:/# pystack remote 260 -vv 2>&1| grep falling
DEBUG(process_remote): process_vm_readv not compiled in kernel, falling back to /proc/PID/mem
root@dce305e0c604:/#

Additional context
I've not written any C++ for about 14 years, so I've probably missed out on some cleverer or prettier ways of doing this.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 68.96552% with 9 lines in your changes missing coverage. Please review.

Project coverage is 82.66%. Comparing base (d86a7c3) to head (769ac62).

Files with missing lines	Patch %	Lines
src/pystack/_pystack/mem.cpp	60.86%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #240      +/-   ##
==========================================
- Coverage   82.73%   82.66%   -0.07%     
==========================================
  Files          46       46              
  Lines        6272     6300      +28     
  Branches      461      467       +6     
==========================================
+ Hits         5189     5208      +19     
- Misses       1083     1092       +9     

Flag	Coverage Δ
cpp	82.66% <68.96%> (-0.07%)	⬇️
python_and_cython	82.66% <68.96%> (-0.07%)	⬇️

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.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[cakemanny]

Hey @godlygeek, did you have any feedback for my PR?
I think maybe you ran the workflows before but they failed a clang-format check, which has since been fixed.

[godlygeek]

Hey @godlygeek, did you have any feedback for my PR?

Sorry for the delay @cakemanny, just circling back to this now.

What you had looked pretty good to me, and I very much appreciate how readable the diff was.

I didn't like the need to fall back to open if std::ifstream::open failed, just to get the errno. That was overly complex, and also you leaked the fd if somehow the fallback open call succeeded (which could happen if, for instance, the pid didn't exist when std::ifstream::open was called but did exist by the time open was called).

Instead of hopping from the highest level interface to the lowest level one just to get the failure reason, we can do everything with a medium-level interface that gives us everything we need - switching to the C style stdio functions lets us get the cause of any errors (they're guaranteed to set errno), while still letting us work at a slightly higher level of abstraction than the raw syscall wrappers.

I also added an environment variable to force us down the /proc/PID/mem path even on kernels that do support process_vm_readv, so that we're able to exercise this path with a test.

Since I made some non-trivial changes here, I'm not going to merge this until either @sarahmonod or @pablogsal check my work.

[godlygeek]

Ah, and also, if you still have access to the system that was compiled without CONFIG_CROSS_MEMORY_ATTACH, can you confirm that things still work after my changes, @cakemanny?

I can't imagine why they wouldn't, since my new test is exercising everything except the errno == ENOSYS case in readChunkDirect which I didn't touch, but just in case...