Repository navigation
gh-112564: Fix UAF for gc debug print - #160662
Conversation
In both uncollectable and collectables we would iterate through gc items using `GC_NEXT(gc)`. However in the case the current item was cleaned during iteration this would reuslt in UAF when fetching the next step in the loop. The fix is to move each item to a temporary `seen` list before printing it, and always take the next item from the head of the original list, so the loop never reads from a freed item. This takes a similar approach to `finalize_garbage()`.
|
Wonder if we just call |
sergey-miryanov
left a comment
sergey-miryanov
left a comment
There was a problem hiding this comment.
Same notes about tests and list traversal.
|
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 |
This comment was marked as resolved.
This comment was marked as resolved.
|
@johng Could you please fix a cast warning in debug_cycles? |
|
@nascheme Could you please take a look? |
|
As mentioned on the other issue, my inclination would be to have a way to output debug info without the GIL drop. Likely we can't be using OTOH, if we look what's done for resource warnings, they do get written to Another possible solution: write the debugging output to a buffer while GC is in the middle of its work. Then, write that buffer to |
|
Happy to explore those suggestions, thanks @nascheme ! |
|
I think the buffer is the cleanest approach, it does result in iterating twice through the loop since we need to know the length ahead of time for buffer allocations unless we have something quite a bit more complex (I believe) |
|
Thanks for trying the buffered approach! At quick glance, this seems the right solution to me. PR still needs review though. |
|
Ah, btw, the gc_free_threading.c file will likely need the same kind of fix because it looks like it has the same issue. |
|
When I checked this with free threading it was quite a different error case, instead we seem to hit a deadlock instead likely cause by Stop the World rather than anything being wrong with the debug loop. This seems like a new issue given it would have a very different bug and fix (which I'm happy to attempt to address, I should raise an issue too). |
nascheme
left a comment
nascheme
left a comment
There was a problem hiding this comment.
This looks good, I had two minor suggestions (NEWS update, comment revision). Could be merged as is. Note that we should still fix the FT GC, IMHO.
|
|
||
| /* Format a line for each object in `list`, then print them. | ||
| * sys.stderr.write(). Stores into buffers so can iterate | ||
| * to avoid calling into Python whilst we iterate the objects |
There was a problem hiding this comment.
Suggested wording:
"Format a line for each object in list before printing any of them, since sys.stderr.write() can run Python code that frees objects in the list."
| if (line == NULL) { | ||
| return NULL; | ||
| } | ||
| /* Not %p: its output is platform-specific (no "0x" prefix on Windows). */ |
There was a problem hiding this comment.
You should mention in NEWS that formatting on some platforms like Windows has changed (lowercase, now always with 0x prefix). Likely no one should care but mention.
|
Thanks for the review! Note I fixed the above mentioned deadlock for FT in #161444 Per above I did not come across a similar class of bug for this, just the deadlock |
nascheme
added
needs backport to 3.14
|
Sorry, @johng and @nascheme, I could not cleanly backport this to Please backport manually with cherry_picker, see the devguide for more information. |
|
Sorry, @johng and @nascheme, I could not cleanly backport this to Please backport manually with cherry_picker, see the devguide for more information. |
|
GH-161895 is a backport of this pull request to the 3.14 branch. |
|
GH-161896 is a backport of this pull request to the 3.15 branch. |
In both uncollectable and collectables we would
iterate through gc items using
GC_NEXT(gc).However in the case the current item was cleaned
during iteration this would reuslt in UAF when
fetching the next step in the loop.
The fix is to move each item to a temporary
seenlistbefore printing it, and always take the next
item from the head of the original list, so the
loop never reads from a freed item. This takes
a similar approach to
finalize_garbage().