bpo-31770: Prevent a crash and refleaks when calling sqlite3.Cursor.__init__() more than once #3968
Conversation
There was a problem hiding this comment.
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;".
Lib/sqlite3/test/regression.py
Outdated
| ref = weakref.ref(cur, callback) | ||
| cur.__init__(con) | ||
| del cur | ||
| del ref # shouldn't crash |
There was a problem hiding this comment.
I don't see the purpose of removing explicitly the last reference to a weak reference object. Just remove "del ref" ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I would add test.support.gc_collect() for ensuring that objects are collected.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Should i replace the del ref with a call to test.support.gc_collect() anyway?
There was a problem hiding this comment.
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 |
…_init__() more than once (pythonGH-3968) (cherry picked from commit e56ab74)
|
GH-4301 is a backport of this pull request to the 3.6 branch. |
|
GH-4302 is a backport of this pull request to the 2.7 branch. |
…_init__() more than once (pythonGH-3968) (cherry picked from commit e56ab74)
…_init__() more than once (python#3968)
In addition, add a test to
test_sqliteto make sure that the crash is no more.https://bugs.python.org/issue31770