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

gh-58956: Fix a frame refleak in bdb #128190

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+54 −0
Open

gh-58956: Fix a frame refleak in bdb #128190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
39818b0
Fix a frame refleak in bdb
gaogaotiantian Dec 23, 2024
9188f32
📜🤖 Added by blurb_it.
blurb-it[bot] Dec 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Lib/bdb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,7 @@ def set_trace(self, frame=None):
frame.f_trace_lines = True
frame = frame.f_back
self.set_stepinstr()
self.enterframe = None
Copy link
Member

@iritkatriel iritkatriel Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should this be in a finally block?

Copy link
Member Author

Choose a reason for hiding this comment

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

Theoretically, it's safer to put it in a try finally block. I did not do it for two reasons:

  1. Currently there's no exception trigger in the code. We'll have a much bigger issue if there is an unexpected exception here.
  2. This is a "not a bug fix" code that we want to backport, so I want to make it as trivial as possible.

I suggest that we do it in the future when there is a possible exception trigger. Or at least after the backport.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I see how putting this in a finally makes any difference w.r.t to backporting?

Copy link
Member Author

Choose a reason for hiding this comment

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

The code diff would be a bit more and we would have an extra indentation.

Currently, all the finally in bdb/pdb protect exceptions from user code, not the debugger code. If we set a precedence to assume every line of code can generate an exception and we have to make sure everything, including references, needs to be properly released, we would be in a giant hole.

For example, we are holding a frame reference in self.returnframe, which is set by _set_stopinfo. And the way we release it, is when we do a continue, set_continue would set it to None. But what if there's a random exception somewhere? Do we protect that with a finally?

We are also using the frame as a key in self.frame_trace_line_opcodes, thus keeping a reference to it, which will also be released in set_continue as long as there are no breakpoints, do we protect those as well (it's right before the next change)? If so, we have two resources to release, do we do

try:
    ...
finally:
    try:
        self.frame_trace_lines_opcodes = {}
    finally:
        self.enterframe = None

I think my point is - the debugger should not crash. If it crashes by itself, we don't really care about reference leaks, we should fix the debugger. Therefore, we should only care about exceptions from the user code and make sure resources are released in those cases.

Copy link
Member

Choose a reason for hiding this comment

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

self.frame_trace_lines_opcodes can be cleared in the same finally. Anyway, up to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, we normally assume self.frame_trace_lines_opcodes = {} won't raise an exception - that's why we can put them together in the same finally. I think we should do the same for most of the internal debugger code - unless it's very critical. That's how pdb/bdb does it for now. It assumes the debugger code works (at least the simple ones) and release resources accordingly.

In set_trace, the code between setting enter_frame and releasing enter_frame are basically all assignments (including set_stepinstr), so we can assume that no exception will be raised (otherwise self.frame_trace_lines_opcodes = {} is not safe either).

As for set_continue, we actually can't prevent the leak from happening if there's a crash in the debugger, because that means it's possible to break out of the debugger without running set_continue. We now assume the user always gets a chance to run continue, which would release the reference - that might not be the case if we consider the possibility of a debugger crash. Then our protection to ensure the release happens in set_continue does not do much.

sys.settrace(self.trace_dispatch)

def set_continue(self):
Expand All @@ -423,6 +424,7 @@ def set_continue(self):
for frame, (trace_lines, trace_opcodes) in self.frame_trace_lines_opcodes.items():
frame.f_trace_lines, frame.f_trace_opcodes = trace_lines, trace_opcodes
self.frame_trace_lines_opcodes = {}
self.enterframe = None

def set_quit(self):
"""Set quitting attribute to True.
Expand Down
51 changes: 51 additions & 0 deletions Lib/test/test_pdb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3022,6 +3022,57 @@ def test_pdb_f_trace_lines():
(Pdb) continue
"""

def test_pdb_frame_refleak():
"""
pdb should not leak reference to frames
>>> def frame_leaker(container):
... import sys
... container.append(sys._getframe())
... import pdb; pdb.Pdb(nosigint=True, readrc=False).set_trace()
... pass
>>> def test_function():
... import gc
... container = []
... frame_leaker(container) # c
... print(len(gc.get_referrers(container[0])))
... container = []
... frame_leaker(container) # n c
... print(len(gc.get_referrers(container[0])))
... container = []
... frame_leaker(container) # r c
... print(len(gc.get_referrers(container[0])))
>>> with PdbTestInput([ # doctest: +NORMALIZE_WHITESPACE
... 'continue',
... 'next',
... 'continue',
... 'return',
... 'continue',
... ]):
... test_function()
> <doctest test.test_pdb.test_pdb_frame_refleak[0]>(4)frame_leaker()
-> import pdb; pdb.Pdb(nosigint=True, readrc=False).set_trace()
(Pdb) continue
1
> <doctest test.test_pdb.test_pdb_frame_refleak[0]>(4)frame_leaker()
-> import pdb; pdb.Pdb(nosigint=True, readrc=False).set_trace()
(Pdb) next
> <doctest test.test_pdb.test_pdb_frame_refleak[0]>(5)frame_leaker()
-> pass
(Pdb) continue
1
> <doctest test.test_pdb.test_pdb_frame_refleak[0]>(4)frame_leaker()
-> import pdb; pdb.Pdb(nosigint=True, readrc=False).set_trace()
(Pdb) return
--Return--
> <doctest test.test_pdb.test_pdb_frame_refleak[0]>(5)frame_leaker()->None
-> pass
(Pdb) continue
1
"""

def test_pdb_function_break():
"""Testing the line number of break on function
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2024-12-23-02-09-44.gh-issue-58956.4OdMdT.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed a frame reference leak in :mod:`bdb`.
Loading