gh-151022: Fix remote debugging linetable reads#151036
Conversation
34645da to
9b0f3ba
Compare
|
@maurycy can you review this? :) |
maurycy
left a comment
There was a problem hiding this comment.
@goutamadwant Woah! Thank you for jumping so quickly into this, and sorry for me jumping so slowly. :-) I've left some comments, with a small question mark to @pablogsal himself
| # ============================================================================ | ||
|
|
||
|
|
||
| class TestSelfStackTrace(RemoteInspectionTestBase): |
There was a problem hiding this comment.
@goutamadwant I think there's a missing @requires_remote_subprocess_debugging() decorator :-)
There was a problem hiding this comment.
Fixed @maurycy.. added the missing @requires_remote_subprocess_debugging() decorator to the new test class.
| self.assertEqual( | ||
| result.returncode, 0, | ||
| f"stdout: {result.stdout}\nstderr: {result.stderr}" |
There was a problem hiding this comment.
@goutamadwant It would be interesting to ensure that the returned linetable is also correct, not -1 :-)
There was a problem hiding this comment.
@maurycy Updated this too. The test now checks the returned frame for large_linetable.py and asserts the resolved line number matches the generated RemoteUnwinder call line, so it should catch the -1 fallback case.
|
|
||
| #include "_remote_debugging.h" | ||
|
|
||
| #define MAX_LINETABLE_SIZE (1024 * 1024) |
There was a problem hiding this comment.
@goutamadwant Why exactly 1024 * 1024? :-)
I know that's an arbitrary number but my hunch would be to try to check what sizes are we seeing in the wild, or at least in the stdlib. There's linetable_dist.py script in the original issue that should be helpful after a small modification!
There was a problem hiding this comment.
You are right @maurycy :) .. 1024 * 1024 was a conservative cap but not really data driven.
I ran a modified linetable scan against Lib. On current main I see max linetable size 37416 bytes, and the reported copyparty case was also lower, around 19536 bytes.
So I think 64 * 1024 may be a better cap for this PR. It covers the stdlib cases and the reported third party case, while still keeping the remote bytes read bounded much tighter than 1 MiB.
Does that sound reasonable to you ? I can make a change accordingly.
| #include "_remote_debugging.h" | ||
|
|
||
| #define MAX_LINETABLE_SIZE (1024 * 1024) | ||
| #define MAX_LINETABLE_ENTRIES 65536 |
There was a problem hiding this comment.
I did not check it but my guess would be that there's much more head room in the MAX_LINETABLE_SIZE now, than in MAX_LINETABLE_ENTRIES. If I'm reading the code right, it fallbacks to -1:
cpython/Modules/_remote_debugging/code_objects.c
Lines 495 to 500 in 29a920e
To be honest, I'd assert (proportionally) entries > size (exception is better than incorrect data) or get rid of MAX_LINETABLE_ENTRIES and entry_count. Either one depends on another, or one has to go...
Another solution would be along the lines of code_units (in CachedCodeMetadata?) but I think it's beyond the scope, and we still have to maintain some cap
There was a problem hiding this comment.
@maurycy I agree this needs a bit more thought.
From the same scan, the largest stdlib linetable I saw was Lib/html/entities.py with 37416 bytes and about 7465 parsed entries. So for the reported cases, the byte cap is the immediate problem. MAX_LINETABLE_ENTRIES still has enough headroom there.
my thinking is to keep the entry cap as a guard for malformed or pathological linetables and keep this PR focused on the too small byte read limit... I also strengthened the test so it checks a real line number instead of allowing -1.
Changing parse failure from unknown line to an exception feels like a separate behavior change to me. Happy to adjust if you and @pablogsal prefer that in this PR :)
Fixes gh-151022.
_remote_debugging reads a code object's line table from the target process
with a fixed 4096 byte limit. Valid code objects can exceed that. For example,
Lib/html/entities.py currently compiles to a line table of 37445 bytes, so
RemoteUnwinder.get_stack_trace() fails with RuntimeError: Invalid bytes length.
This keeps the generic remote bytes reader bounded, but gives code object line
tables their own larger limit before parsing. The parser already bounds the
number of line table entries it will scan.
Added a regression test that builds a code object with a line table larger than
4096 bytes and verifies that a same-process stack trace succeeds.
Tests:
./python.exe -m unittest -v test.test_external_inspection.TestSelfStackTrace.test_self_trace_with_large_linetable
./python.exe -m test test_external_inspection -v
make patchcheck