py-spy icon indicating copy to clipboard operation
py-spy copied to clipboard

Formalize linetable parsing and propagate out-of-bounds errors

Open noahbkim opened this issue 8 months ago • 8 comments

We've noticed a transient issue where py-spy will abort due to unmet assumptions about the contents of co_linetable (for example we've seen [231], which doesn't even satisfy the internal specification). To address this, I've patched the co_linetable parsing to propagate out-of-bounds errors so that the samples are simply ignored (and a warning is raised).

This change is not as minimal as it could be, for example it reorders a couple lines in python_interpreters.rs for clarity. If that's a problem, I'd be happy to go back and pare down such changes.

Taking a glance, I would guess this is the root cause for https://github.com/benfred/py-spy/issues/735 and https://github.com/benfred/py-spy/issues/569, although I have no explanation for why the co_linetable's appear invalid.

noahbkim avatar Apr 01 '25 20:04 noahbkim

Updated the PR to include a fix for an off-by-one introduced by https://github.com/benfred/py-spy/pull/635, trivially reproducible with the following:

# a.py
import b

# b.py
import time
time.sleep(10)

# Command
cargo run -F unwind --release -- record --native --rate 1 -- python3.12 a.py

noahbkim avatar Apr 04 '25 18:04 noahbkim

This change is not as minimal as it could be, for example it reorders a couple lines in python_interpreters.rs for clarity. If that's a problem, I'd be happy to go back and pare down such changes.

Let's change only what's needed for the fix, so that it's easier to cherry-pick & revert & reason about

korniltsev avatar Apr 11 '25 00:04 korniltsev

@benfred can approve this workflow so the build can run?

Otherwise this LGTM, and we've manually built it and confirmed it's fixed the issues we were seeing.

Hnasar avatar Apr 22 '25 14:04 Hnasar

@benfred can approve this workflow so the build can run?

I believe that this branch needs to be updated to the latest master first (both to get some CI fixes in - and to let me approve the workflow run) - and I don't seem to have permissions on this branch to update it myself =(

benfred avatar Jul 12 '25 20:07 benfred

I suggest we also fix int overflows either in the same PR or in a followup, so that debug builds of pyspy will not panic

korniltsev avatar Jul 14 '25 02:07 korniltsev

Updated! @benfred thanks for taking a look. @korniltsev can you point to the overflows to fix?

noahbkim avatar Jul 17 '25 17:07 noahbkim

@korniltsev can you point to the overflows to fix?

Basically every time some uncontrolled user-provided number (from the linetable) is added, subtracted, or shifted, etc. You can search for "checked" in this diff https://github.com/grafana/py-spy/pull/1/files Fuzzing is a great and easy way to find some of those overflows.

Btw, I am not a maintainer here, this is just a suggestion for improvements. Maybe even better to do as a separate change. Feel free to ignore.

Thank you for working on this!

korniltsev avatar Jul 18 '25 02:07 korniltsev

Updated the PR to include a fix for an off-by-one introduced by https://github.com/benfred/py-spy/pull/635, trivially reproducible with the following:

I believe this is fixed by https://github.com/benfred/py-spy/pull/751 - can you verify if this still happens with the latest code?

Also, There was also an alternative fix for the same linetable problem in https://github.com/benfred/py-spy/pull/736 - I've merged that one since it passed CI and I want to get a release out.

I like the unittests that you've added here though, any chance that you can merge in the other PR here so we can get those tests included?

benfred avatar Jul 31 '25 18:07 benfred