bpo-31770: Prevent a crash and refleaks when calling sqlite3.Cursor.__init__() more than once by orenmn · Pull Request #3968 · python/cpython
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, except of "del ref".
@serhiy-storchaka: Would you mind to double-check this PR please?
It seems like "self->in_weakreflist" is already set to NULL by the memory allocator, so it's safe to remove "self->in_weakreflist = NULL;".
| ref = weakref.ref(cur, callback) | ||
| cur.__init__(con) | ||
| del cur | ||
| del ref # shouldn't crash |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the purpose of removing explicitly the last reference to a weak reference object. Just remove "del ref" ?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least on my Windows 10, when the test fails, the del ref causes the crash info that is printed to include the line (and test) that caused the crash
(i.e. File "C:\Users\orenm\cpython\lib\sqlite3\test\regression.py", line 392 in CheckBpo31770). If i remove the del ref, it doesn't show info about the line and test that caused the crash.
If you still think that i should remove it, i wouldn't mind, of course.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add test.support.gc_collect() for ensuring that objects are collected.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When i use test.support.gc_collect(), the crash info also doesn't include the line and the test that crashed.
Also, when i add 1/0 after the call to gc_collect(), and run the test (on cpython without the patch in C code), the code doesn't crash in gc_collect(), and a ZeroDivisionError is raised...
(of course, without the 1/0, it does crash, but after the gc_collect(), i guess.)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should i replace the del ref with a call to test.support.gc_collect() anyway?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you call test.support.gc_collect() instead of del ref? Or adding test.support.gc_collect() after del ref breaks the test?
I meant that you should keep del ref, but call test.support.gc_collect() after it for sure. Of course, if this breaks the test, don't add it.
I didn't approve the PR yet because of my question on "del ref".
The line number doesn't matter for a test triggering a crash, I mean non regression trst fixing a crash. Yes, replace the useless del with gc_collect() please.
IIUC, calling gc_collect() without doing del ref wouldn't really cause the collection of ref, because there is still one reference to it until the method is over. The call to gc_collect() is to make sure ref is collected, as Serhiy advised.
Is it ok, Victor, or am i missing something?
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
