Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use /proc/pid/exe for the Python binary in Linux #364

Merged
merged 4 commits into from
Mar 21, 2021

Conversation

Jongy
Copy link
Contributor

@Jongy Jongy commented Mar 20, 2021

Instead of "/proc/pid/root/$(readlink /proc/pid/exe)", we'll just use /proc/pid/exe
which provides access to the file even if it was deleted on-disk.

This is not uncommon - for example, Python may be upgraded and thus long-running
processes no longer have the correct file at the specified path.
After this change we can profile/dump those processes (only if they are Python-binary
based and not libpython-based; the latter is inaccessible if deleted... perhaps only
partially accessible via /proc/pid/map_files).

Note: this depends on rbspy/proc-maps#8 because currently we don't extract the full filename from /proc/pid/maps, so the comparison in is_python_bin (matching readlink("/proc/pid/exe") to filenames in /proc/pid/maps) always fails.

With this PR (and the proc-maps fix) I managed to py-spy dump Python processes that were started before I upgraded Python on my local box :) (so they now run /usr/bin/python3.8 (deleted))

Instead of "/proc/pid/root/$(readlink /proc/pid/exe)", we'll just use /proc/pid/exe
which provides access to the file even if it was deleted on-disk.

This is not uncommon - for example, Python may be upgraded and thus long-running
processes no longer have the correct file at the specified path.
After this change we can profile/dump those processes (only if they are Python-binary
based and not libpython-based; the latter is inaccessible if deleted... perhaps only
partially accessible via /proc/pid/map_files).
src/binary_parser.rs Outdated Show resolved Hide resolved
src/binary_parser.rs Outdated Show resolved Hide resolved
@benfred
Copy link
Owner

benfred commented Mar 21, 2021

thanks for this! I think this will fix #109 (and possibly even #360)

src/binary_parser.rs Outdated Show resolved Hide resolved
@Jongy
Copy link
Contributor Author

Jongy commented Mar 21, 2021

thanks for this! I think this will fix #109 (and possibly even #360)

#109 surely, yeah, that's exactly the issue.

EDIT: reading #360 thoroughly, I think it'll fix it as well.

@benfred benfred linked an issue Mar 21, 2021 that may be closed by this pull request
@benfred benfred merged commit e905a07 into benfred:master Mar 21, 2021
@Jongy Jongy deleted the deletes-exes branch March 21, 2021 22:11
@Jongy
Copy link
Contributor Author

Jongy commented Mar 21, 2021

@benfred note that we need to bump the version of proc_maps to accommodate for rbspy/proc-maps#8

Jongy added a commit to Granulate/gprofiler that referenced this pull request May 1, 2021
There's no need to move the the target PID namespace.

The link can be passed to execve itself. It has the benefit of running even if
the file was deleted (for example - if the Java installation was upgraded then
readlink("/proc/pid/exe") returns the path with " (deleted)", but executing
/proc/pid/exe will run the "old" version just fine (as long as all libs exist, etc).

This matches the fix I posted for py-spy: benfred/py-spy#364
Jongy added a commit to Granulate/gprofiler that referenced this pull request May 2, 2021
There's no need to move the the target PID namespace.

The link can be passed to execve itself. It has the benefit of running even if
the file was deleted (for example - if the Java installation was upgraded then
readlink("/proc/pid/exe") returns the path with " (deleted)", but executing
/proc/pid/exe will run the "old" version just fine (as long as all libs exist, etc).

This matches the fix I posted for py-spy: benfred/py-spy#364
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot attach to long running process
2 participants