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.

Sign up for GitHub

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

Jump to bottom

Handle offsetted Elf segment case + FreeBSD CI #433

Merged
merged 2 commits into from
Aug 18, 2021
Merged

Handle offsetted Elf segment case + FreeBSD CI #433

merged 2 commits into from
Aug 18, 2021

Conversation

akhramov
Copy link
Contributor

@akhramov akhramov commented Aug 8, 2021 •
edited
Loading

An Elf segment may have a non-zero offset and that may result in
subtraction overflow when we calculating the symbol addresses.

Apart from that, CI workflows are run within GH Actions with FreeBSD
being notable exception. While FreeBSD is not supported on actions,
the conventional workaround is to run builds on macos runner using a
vagrant box.

This change

  • Handles the subtraction overflow case by defaulting to zero.

  • removes cirrus config and sets up a FreeBSD GH action job.

@akhramov akhramov changed the title Setup GitHub actions for FreeBSD [Spike] Setup GitHub actions for FreeBSD Aug 8, 2021
@akhramov
Copy link
Contributor Author

akhramov commented Aug 8, 2021

I just want to evaluate the viability of keeping CI entirely within GH Actions

@akhramov akhramov force-pushed the feature/github-actions-for-freebsd branch 4 times, most recently from 2fe68ce to d5113b0 Compare August 8, 2021 21:57
@akhramov akhramov changed the title [Spike] Setup GitHub actions for FreeBSD Handle offsetted Elf segment case + FreeBSD CI Aug 8, 2021
@akhramov akhramov mentioned this pull request Aug 8, 2021
Closed
@akhramov akhramov force-pushed the feature/github-actions-for-freebsd branch 8 times, most recently from a4e1a1a to 25b4fda Compare August 11, 2021 12:56
@benfred benfred mentioned this pull request Aug 11, 2021
Closed
benfred
benfred reviewed Aug 11, 2021
View reviewed changes
Copy link
Owner

@benfred benfred left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for looking into this!

I tried to move the freebsd CI to github actions in #409 - but got stuck with some weird errors. I'm glad that you got this working.

src/python_spy.rs Outdated
@@ -724,6 +724,7 @@ impl PythonProcessInfo {
let filename = process.exe()
.context("Failed to get process executable name. Check that the process is running.")?;

println!("{:?}", filename);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this line?

Suggested change
println!("{:?}", filename);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, thanks.

Vagrantfile Outdated
@@ -0,0 +1,35 @@
# This code is taken from the Vagrantfile from libjail-rs
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move this file to the 'ci' subfolder?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, good suggestion.

src/binary_parser.rs
let offset = offset - program_header.p_vaddr;
// p_vaddr may be larger than the map address in case when the header has an offset and
// the map address is relatively small. In this case we can default to 0.
let offset = offset.checked_sub(program_header.p_vaddr).unwrap_or(0);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we set offset to 0 - that seems to mean we disregard the address that virtual memory maps address entirely. Is this the right thing to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a corner case for the very first memory map.

In my python binary, p_vaddr, while in the map, is not aligned to the beginning. Therefore, it is slightly larger than the map address, so the overflow happens.

This was not the case in FreeBSD 11 python, and p_vaddr was aligned to the beginning of the map, so offset was 0.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm - I see. Is the first memory map here the main python binary?

I'm wondering if instead of setting to 0 - we should adjust lines 110/114/118 (so like sym.st_value + offset - program_header.p_vaddr etc). It seems like the results here for the BSS section and the symbols will be wrong with just setting the address to 0.

Can you paste a log with RUST_LOG=INFO here? I'm interested to see if we can use the symbols returned from this successfully.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if instead of setting to 0 - we should adjust lines 110/114/118 (so like sym.st_value + offset - program_header.p_vaddr etc)

Tried this, still getting the overflow.

Can you paste a log with RUST_LOG=INFO here? I'm interested to see if we can use the symbols returned from this successfully.

Sure

Running `target/debug/py-spy record --pid 1663`
[2021-08-17T05:56:38.663688072Z INFO  py_spy::config] Command line args: ArgMatches { args: {}, subcommand: Some(SubCommand { name: "record", matches: ArgMatches { args: {"format": MatchedArg { occurs: 0, indices: [3], vals: ["flamegraph"] }, "rate": MatchedArg { occurs: 0, indices: [5], vals: ["100"] }, "duration": MatchedArg { occurs: 0, indices: [4], vals: ["unlimited"] }, "pid": MatchedArg { occurs: 1, indices: [2], vals: ["1663"] }}, subcommand: None, usage: Some("USAGE:\n    py-spy record [OPTIONS] --pid <pid> [python_program]...") } }), usage: Some("USAGE:\n    py-spy <SUBCOMMAND>") }
[2021-08-17T05:56:38.664286756Z INFO  py_spy::python_spy] Got virtual memory maps from pid 1663:
[2021-08-17T05:56:38.666193438Z INFO  py_spy::python_spy] Found libpython binary @ /usr/local/lib/libpython3.8.so.1.0
[2021-08-17T05:56:38.681141446Z INFO  py_spy::python_spy] Getting version from python binary BSS
[2021-08-17T05:56:38.685870855Z INFO  py_spy::python_spy] Failed to get version from BSS section: failed to find version string
[2021-08-17T05:56:38.685895646Z INFO  py_spy::python_spy] Getting version from libpython BSS
[2021-08-17T05:56:38.691449682Z INFO  py_spy::version] Found matching version string '3.8.10 (default, May 18 2021, 01:14:29) '
[2021-08-17T05:56:38.691474713Z INFO  py_spy::python_spy] python version 3.8.10 detected
[2021-08-17T05:56:38.691492852Z INFO  py_spy::python_spy] got symbol _PyRuntime (0x0000000800587b38) from libpython binary
[2021-08-17T05:56:38.691520965Z WARN  py_spy::python_spy] Interpreter address from _PyRuntime symbol is invalid 0000000000000000
[2021-08-17T05:56:38.691541874Z INFO  py_spy::python_spy] Failed to get interp_head from symbols, scanning BSS section from main binary
[2021-08-17T05:56:38.691571414Z INFO  py_spy::python_spy] Failed to get interpreter from binary BSS, scanning libpython BSS
[2021-08-17T05:56:38.739178491Z INFO  py_spy::python_spy] Found interpreter at 0x0000000800e54000
[2021-08-17T05:56:38.739201332Z INFO  py_spy::python_spy] got symbol _PyRuntime (0x0000000800587b38) from libpython binary
[2021-08-17T05:56:38.739217883Z INFO  py_spy::python_spy] Found _PyRuntime @ 0x0000000800587b38, getting gilstate.tstate_current from offset 0x4c8

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks like its picking up the python symbols from the libpython library . I'm still wondering if the default '0' value is correct here, but since this is better than panic'ing like it will now - I think we should merge this.

(though I would be interested to see what happens if there is no libpython binary and we need to read symbols from the main python executable (like building python without --enable-shared https://docs.python.org/3.11/using/configure.html#linker-options ) )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me actually check this -- will get back to you shortly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like everything is okay

     Running `target/debug/py-spy record --pid 32133`
[2021-08-18T16:00:03.402788857Z INFO  py_spy::config] Command line args: ArgMatches { args: {}, subcommand: Some(SubCommand { name: "record", matches: ArgMatches { args: {"duration": MatchedArg { occurs: 0, indices: [4], vals: ["unlimited"] }, "format": MatchedArg { occurs: 0, indices: [3], vals: ["flamegraph"] }, "pid": MatchedArg { occurs: 1, indices: [2], vals: ["32133"] }, "rate": MatchedArg { occurs: 0, indices: [5], vals: ["100"] }}, subcommand: None, usage: Some("USAGE:\n    py-spy record [OPTIONS] --pid <pid> [python_program]...") } }), usage: Some("USAGE:\n    py-spy <SUBCOMMAND>") }
[2021-08-18T16:00:03.403722389Z INFO  py_spy::python_spy] Got virtual memory maps from pid 32133:
[2021-08-18T16:00:03.452896463Z INFO  py_spy::python_spy] got symbol Py_GetVersion.version (0x00000000004c66d0) from python binary
[2021-08-18T16:00:03.452937099Z INFO  py_spy::python_spy] Getting version from symbol address
[2021-08-18T16:00:03.460439322Z INFO  py_spy::version] Found matching version string '3.8.0a1 (tags/v3.8.0a1:e75eeb00b5, Aug 18 2021, 18:58:38) '
[2021-08-18T16:00:03.460465918Z INFO  py_spy::python_spy] python version 3.8.0a1 detected
[2021-08-18T16:00:03.460484420Z INFO  py_spy::python_spy] got symbol _PyRuntime (0x00000000004c49c0) from python binary
[2021-08-18T16:00:03.460538355Z INFO  py_spy::python_spy] Found interpreter at 0x0000000800c65000
[2021-08-18T16:00:03.460557038Z INFO  py_spy::python_spy] got symbol _PyRuntime (0x00000000004c49c0) from python binary
[2021-08-18T16:00:03.460574626Z INFO  py_spy::python_spy] Found _PyRuntime @ 0x00000000004c49c0, getting gilstate.tstate_current from offset 0x4d8
py-spy> Sampling process 100 times a second. Press Control-C to exit.

Offset would overflow in this case too (offset=2953216 and program_header.p_vaddr=2957168).

FWIW the maps of the process (procstat vm PID)

32133           0x200000           0x2d1000 r--  209 2633   4   0 CN--- vn /usr/home/akhramov/work/bsd/cpython/python
32133           0x2d1000           0x441000 r-x  368 2633   4   0 CN--- vn /usr/home/akhramov/work/bsd/cpython/python
32133           0x441000           0x442000 r--    1    0   1   0 C---- vn /usr/home/akhramov/work/bsd/cpython/python
32133           0x442000           0x4a8000 rw-  102    0   1   0 C---- vn /usr/home/akhramov/work/bsd/cpython/python
32133           0x4a8000           0x4c8000 rw-   32   32   1   0 ----- df 
32133        0x800442000        0x800448000 r--    6   28 190   0 CN--- vn /libexec/ld-elf.so.1
32133        0x800448000        0x80045f000 r-x   23   28 190   0 CN--- vn /libexec/ld-elf.so.1
32133        0x80045f000        0x800460000 r--    1    0   1   0 C---- vn /libexec/ld-elf.so.1
32133        0x800460000        0x800481000 rw-   29   29   1   0 ----- df 
32133        0x800484000        0x800488000 r--    4   13  32   0 CN--- vn /lib/libcrypt.so.5
32133        0x800488000        0x800491000 r-x    9   13  32   0 CN--- vn /lib/libcrypt.so.5
32133        0x800491000        0x800492000 r--    1    0   1   0 C---- vn /lib/libcrypt.so.5
32133        0x800492000        0x800494000 rw-    2    0   1   0 C---- vn /lib/libcrypt.so.5
32133        0x800494000        0x8004a5000 rw-    0    0   0   0 ----- -- 
32133        0x8004a5000        0x8004a6000 r--    1    1  64   0 CN--- vn /usr/lib/libdl.so.1
32133        0x8004a6000        0x8004a7000 r-x    1    1  64   0 CN--- vn /usr/lib/libdl.so.1
32133        0x8004a7000        0x8004a8000 r--    1    0   1   0 C---- vn /usr/lib/libdl.so.1
32133        0x8004a8000        0x8004a9000 rw-    1    0   1   0 C---- vn /usr/lib/libdl.so.1
32133        0x8004a9000        0x8004b1000 r--    8   20 153   0 CN--- vn /lib/libutil.so.9
32133        0x8004b1000        0x8004bd000 r-x   12   20 153   0 CN--- vn /lib/libutil.so.9
32133        0x8004bd000        0x8004be000 r--    1    0   1   0 C---- vn /lib/libutil.so.9
32133        0x8004be000        0x8004bf000 rw-    1    0   1   0 C---- vn /lib/libutil.so.9
32133        0x8004bf000        0x8004c1000 rw-    0    0   0   0 ----- -- 
32133        0x8004c1000        0x8004d3000 r--   17   47 100   0 CN--- vn /lib/libm.so.5
32133        0x8004d3000        0x8004f1000 r-x   30   47 100   0 CN--- vn /lib/libm.so.5
32133        0x8004f1000        0x8004f2000 r--    1    0   1   0 C---- vn /lib/libm.so.5
32133        0x8004f2000        0x8004f4000 rw-    2    0   1   0 C---- vn /lib/libm.so.5
32133        0x8004f4000        0x800501000 r--   13   31 152   0 CN--- vn /lib/libthr.so.3
32133        0x800501000        0x800513000 r-x   18   31 152   0 CN--- vn /lib/libthr.so.3
32133        0x800513000        0x800514000 r--    1    0   1   0 C---- vn /lib/libthr.so.3
32133        0x800514000        0x800516000 rw-    2    0   1   0 C---- vn /lib/libthr.so.3
32133        0x800516000        0x800521000 rw-   11   11   1   0 ----- df 
32133        0x800521000        0x8005a5000 r--  113  449 248   0 CN--- vn /lib/libc.so.7
32133        0x8005a5000        0x8006f7000 r-x  322  449 248   0 CN--- vn /lib/libc.so.7
32133        0x8006f7000        0x800701000 r--   10    0   1   0 C---- vn /lib/libc.so.7
32133        0x800701000        0x800708000 rw-    7    0   1   0 C---- vn /lib/libc.so.7
32133        0x800708000        0x8009d4000 rw-  151  151   1   0 ----- df 
32133        0x8009d4000        0x8009d8000 r--    4   20   4   0 CN--- vn /usr/home/akhramov/work/bsd/cpython/build/lib.freebsd-13.0-STABLE-amd64-3.8/readline.so
32133        0x8009d8000        0x8009da000 r-x    2   20   4   0 CN--- vn /usr/home/akhramov/work/bsd/cpython/build/lib.freebsd-13.0-STABLE-amd64-3.8/readline.so
32133        0x8009da000        0x8009db000 r--    1    0   1   0 C---- vn /usr/home/akhramov/work/bsd/cpython/build/lib.freebsd-13.0-STABLE-amd64-3.8/readline.so
32133        0x8009db000        0x8009dd000 rw-    2    0   1   0 C---- vn /usr/home/akhramov/work/bsd/cpython/build/lib.freebsd-13.0-STABLE-amd64-3.8/readline.so
32133        0x800a00000        0x801300000 rw-  639  639   1   0 ----- df 
32133        0x801300000        0x801321000 r--   33   87   4   0 CN--- vn /usr/local/lib/libreadline.so.8.1
32133        0x801321000        0x801350000 r-x   47   87   4   0 CN--- vn /usr/local/lib/libreadline.so.8.1
32133        0x801350000        0x801352000 r--    2    0   1   0 C---- vn /usr/local/lib/libreadline.so.8.1
32133        0x801352000        0x801359000 rw-    7    0   1   0 C---- vn /usr/local/lib/libreadline.so.8.1
32133        0x801359000        0x80135a000 rw-    1    1   1   0 ----- df 
32133        0x80135a000        0x801389000 r--   47   96  56   0 CN--- vn /lib/libncursesw.so.9
32133        0x801389000        0x8013c7000 r-x   46   96  56   0 CN--- vn /lib/libncursesw.so.9
32133        0x8013c7000        0x8013cb000 r--    4    0   1   0 C---- vn /lib/libncursesw.so.9
32133        0x8013cb000        0x8013cd000 rw-    2    0   1   0 C---- vn /lib/libncursesw.so.9
32133        0x8013cd000        0x8016b2000 rw-   74   74   1   0 ----- df 
32133     0x7fffdfffe000     0x7fffdffff000 ---    0    0   0   0 ----- -- 
32133     0x7fffdffff000     0x7ffffffdf000 ---    0    0   0   0 ----- gd 
32133     0x7ffffffdf000     0x7ffffffff000 rw-    8    8   1   0 ---D- df 
32133     0x7ffffffff000     0x800000000000 r-x    1    1  69   0 ----- ph

@akhramov akhramov force-pushed the feature/github-actions-for-freebsd branch 4 times, most recently from cf0ccf7 to 782c21b Compare August 12, 2021 03:15
@akhramov
benfred#431 Handle offsetted Elf segment case + FreeBSD CI
a0569bb 
An Elf segment may have a non-zero offset and that may result in
subtraction overflow when we calculating the symbol addresses.

Apart from that, CI workflows are run within GH Actions with FreeBSD
being notable exception. While FreeBSD is not supported on actions,
the conventional workaround is to run builds on macos runner using a
vagrant box.

This change
- Handles the subtraction overflow case by defaulting to zero.
- removes cirrus config and sets up a FreeBSD GH action job.
- Also bumps proc-maps version in attempt to resolve benfred#431
@akhramov akhramov force-pushed the feature/github-actions-for-freebsd branch from 782c21b to a0569bb Compare August 12, 2021 03:40
@benfred benfred merged commit b222f0b into benfred:master Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benfred benfred benfred approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@akhramov @benfred