8000 bpo-31770: Prevent a crash and refleaks when calling sqlite3.Cursor.__init__() more than once by orenmn · Pull Request #3968 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-31770: Prevent a crash and refleaks when calling sqlite3.Cursor.__init__() more than once #3968

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

Merged
merged 6 commits into from
Nov 7, 2017
Merged

bpo-31770: Prevent a crash and refleaks when calling sqlite3.Cursor.__init__() more than once #3968

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6f30a1d
init commit
orenmn Oct 12, 2017
15f071a
added a NEWS.d item and a test.
orenmn Oct 12, 2017
497ced8
improve the test's comment
orenmn Oct 12, 2017
617c45a
Merge branch 'master' into bpo31770-fix-crash-and-refleaks
orenmn Oct 14, 2017
82dfbf8
improve the test.
orenmn Oct 14, 2017
51bb341
changed the test to include both 'del ref' and the call to gc_collect()
orenmn Oct 20, 2017
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
15 changes: 15 additions & 0 deletions Lib/sqlite3/test/regression.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import datetime
import unittest
import sqlite3 as sqlite
import weakref

class RegressionTests(unittest.TestCase):
def setUp(self):
Expand Down Expand Up @@ -376,6 +377,20 @@ def CheckCommitCursorReset(self):
counter += 1
self.assertEqual(counter, 3, "should have returned exactly three rows")

def CheckBpo31770(self):
"""
The interpreter shouldn't crash in case Cursor.__init__() is called
more than once.
"""
def callback(*args):
pass
con = sqlite.connect(":memory:")
cur = sqlite.Cursor(con)
ref = weakref.ref(cur, callback)
cur.__init__(con)
del cur
del ref # shouldn't crash
Copy link
Member

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" ?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.)

Copy link
Contributor Author

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?

Copy link
Member

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.



def suite():
regression_suite = unittest.makeSuite(RegressionTests, "Check")
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2017-10-12-18-45-38.bpo-31770.GV3MPx.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Prevent a crash when calling the ``__init__()`` method of a
``sqlite3.Cursor`` object more than once. Patch by Oren Milman.
15 changes: 7 additions & 8 deletions Modules/_sqlite/cursor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,21 +39,20 @@ static int pysqlite_cursor_init(pysqlite_Cursor* self, PyObject* args, PyObject*
}

Py_INCREF(connection);
self->connection = connection;
self->statement = NULL;
self->next_row = NULL;
self->in_weakreflist = NULL;
Py_XSETREF(self->connection, connection);
Py_CLEAR(self->statement);
Py_CLEAR(self->next_row);

self->row_cast_map = PyList_New(0);
Py_XSETREF(self->row_cast_map, PyList_New(0));
if (!self->row_cast_map) {
return -1;
}

Py_INCREF(Py_None);
self->description = Py_None;
Py_XSETREF(self->description, Py_None);

Py_INCREF(Py_None);
self->lastrowid= Py_None;
Py_XSETREF(self->lastrowid, Py_None);

self->arraysize = 1;
self->closed = 0;
Expand All @@ -62,7 +61,7 @@ static int pysqlite_cursor_init(pysqlite_Cursor* self, PyObject* args, PyObject*
self->rowcount = -1L;

Py_INCREF(Py_None);
self->row_factory = Py_None;
Py_XSETREF(self->row_factory, Py_None);

if (!pysqlite_check_thread(self->connection)) {
return -1;
Expand Down
0