gh-134876: Add fallback for when process_vm_readv fails with ENOSYS #134878

cakemanny · 2025-05-29T10:57:02Z

This adds a fallback /proc/[pid]/mem from the proc(5) filesystem when process_vm_readv and process_vm_writev are not compiled into the kernel.

Regarding tests, these are covered by ./python -m test --match 'test_remote_pdb', but only on affected systems.
Do say if this merits a NEWS entry, I wasn't sure because it's a fix for not yet released python versions.

Issue: python -m pdb -p fails when CONFIG_CROSS_MEMORY_ATTACH not set in kernel config #134876

python-cla-bot · 2025-05-29T10:57:05Z

All commit authors signed the Contributor License Agreement.

bedevere-app · 2025-05-29T10:57:08Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

cakemanny · 2025-05-29T11:12:18Z

I think the those tests are only failing because compilation fails on gcc-10. I'll have a look and see if I can obtain a copy and will update accordingly.

bedevere-app · 2025-05-29T12:28:47Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

pablogsal · 2025-05-29T16:54:15Z

Python/remote_debug.h

@@ -952,6 +955,38 @@ _Py_RemoteDebug_ReadRemoteMemory(proc_handle_t *handle, uintptr_t remote_address
        result += read_bytes;
    } while ((size_t)read_bytes != local[0].iov_len);
    return 0;
+fallback:
+    ; // statement after label needed by gcc 10 and earlier


Can you factor the fallback out into a different function. I think this makes the normal path more difficult to read

pablogsal · 2025-05-29T16:54:40Z

Python/remote_debug.h

+    char mem_file_path[64];
+    off_t offset = 0;
+    sprintf(mem_file_path, "/proc/%d/mem", handle->pid);
+    int fd = open(mem_file_path, O_RDONLY);


This is going to be tremendously inefficient for a profiler because it keeps opening the pseudo file all the time

I could add the file descriptor to proc_handle_t with an initial value of -1 and then open only on the first ENOSYS. Any thoughts?

You can guard it with macros for process_vm_readv but we are already checking for they to compile all of this in so you still need to deal with that problem

Ah no my bad. The problem here is that the function IS defined but the kernel refuses.... alright in that case yeah add it with a guard for only Linux

pablogsal · 2025-05-29T16:54:49Z

Python/remote_debug.h

+
+        result += read_bytes;
+    } while ((size_t)read_bytes != local[0].iov_len);
+    close(fd);


Same as before

pablogsal · 2025-05-29T16:55:13Z

Python/remote_debug.h

+        local[0].iov_len = len - result;
+        offset = remote_address + result;
+
+        read_bytes = preadv(fd, local, 1, offset);


You need to check for preadv and gate it by a macro as this is not available in all platforms IIRC

I gave this a go, but I think it's making the code a lot more complicated.

I now took a look at the histories of glibc and musl.
In musl, both both preadv and process_vm_readv were added in 2012 (searches: preadv, process_vm_readv)

In glibc preadv was added in glibc 2.10 and process_vm_readv in glibc 2.15 (man pages: preadv, process_vm_readv). Those were released in 2009 and 2012 respectively. So process_vm_readv is good enough as a guard here.

Seems Android is the one you are thinking of, https://android.googlesource.com/platform/bionic/+/refs/heads/main/libc/include/sys/uio.h
It states preadv is available at API level 24 and process_vm_readv at API level 23, which are from Android 7 (2016) and Android 6 (2015) respectively. I wasn't able to find somewhere stating which versions we support.

I think I will switch to pread and pwrite which have been available a lot longer. I tried to do that but I was forever unhappy with the name for the loop-inner count/len variable. I tried to_read, count, len_remaining, bytes_remaining, ... I think I will come back to this tomorrow.

preadv is available at API level 24 and process_vm_readv at API level 23, which are from Android 7 (2016) and Android 6 (2015) respectively. I wasn't able to find somewhere stating which versions we support.

This is defined in Android/android-env.sh. Python 3.13 requires API level 21 or greater, and Python 3.14 increases that to 24.

Given, this, I think is fine, just drop a comment so we know why are we not gating it :)

pablogsal · 2025-05-29T16:55:24Z

Python/remote_debugging.c

+        result += written;
+    } while ((size_t)written != local[0].iov_len);
+    return 0;
+fallback:


Same feedback as the read path

bedevere-app · 2025-05-29T16:55:39Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

bedevere-app · 2025-05-29T21:29:26Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

bedevere-app · 2025-05-29T21:44:50Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

bedevere-app · 2025-05-29T22:05:35Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

bedevere-app · 2025-05-29T22:37:17Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

pablogsal · 2025-05-30T12:54:06Z

@cakemanny one small comment: we squash merge so there is no need to rebase. It's easier to review if you just add commits on top so we don't need to review the full thing every iteration :)

cakemanny · 2025-05-31T13:09:22Z

I have made the requested changes; please review again

bedevere-app · 2025-05-31T13:09:26Z

Thanks for making the requested changes!

@pablogsal: please review the changes made to this pull request.

cakemanny requested a review from pablogsal as a code owner May 29, 2025 10:57

bedevere-app bot added the awaiting review label May 29, 2025

bedevere-app bot mentioned this pull request May 29, 2025

python -m pdb -p fails when CONFIG_CROSS_MEMORY_ATTACH not set in kernel config #134876

Open

cakemanny force-pushed the fallback-to-proc-mem branch from 938b91c to 90b08e6 Compare May 29, 2025 12:28

pablogsal requested changes May 29, 2025

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels May 29, 2025

cakemanny force-pushed the fallback-to-proc-mem branch from 90b08e6 to cbea7a2 Compare May 29, 2025 21:29

cakemanny force-pushed the fallback-to-proc-mem branch from cbea7a2 to 5fd8ff6 Compare May 29, 2025 21:44

cakemanny force-pushed the fallback-to-proc-mem branch from 5fd8ff6 to ef8ae45 Compare May 29, 2025 22:05

Add fallback for when process_vm_readv fails with ENOSYS

f1eb6c0

cakemanny force-pushed the fallback-to-proc-mem branch from ef8ae45 to f1eb6c0 Compare May 29, 2025 22:37

cakemanny added 2 commits May 31, 2025 10:05

Subsume guards for preadv into those for process_vm_readv

26b8647

Add news entry

2ebfc19

bedevere-app bot added awaiting change review and removed awaiting changes labels May 31, 2025

bedevere-app bot requested a review from pablogsal May 31, 2025 13:09

mostafaammer added this to lavitaconnect@MOSTAFAAMMER May 31, 2025

Uh oh!

gh-134876: Add fallback for when process_vm_readv fails with ENOSYS #134878

Are you sure you want to change the base?

gh-134876: Add fallback for when process_vm_readv fails with ENOSYS #134878

Conversation

cakemanny commented May 29, 2025 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

python-cla-bot bot commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-app bot commented May 29, 2025

Uh oh!

cakemanny commented May 29, 2025

Uh oh!

bedevere-app bot commented May 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-app bot commented May 29, 2025

Uh oh!

bedevere-app bot commented May 29, 2025

Uh oh!

bedevere-app bot commented May 29, 2025

Uh oh!

bedevere-app bot commented May 29, 2025

Uh oh!

bedevere-app bot commented May 29, 2025

Uh oh!

pablogsal commented May 30, 2025

Uh oh!

cakemanny commented May 31, 2025

Uh oh!

bedevere-app bot commented May 31, 2025

Uh oh!

Uh oh!

cakemanny commented May 29, 2025 •

edited by bedevere-app bot

Loading

python-cla-bot bot commented May 29, 2025 •

edited

Loading