Skip to content
27 changes: 27 additions & 0 deletions Lib/sqlite3/test/regression.py
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,33 @@ def test_return_empty_bytestring(self):
val = cur.fetchone()[0]
self.assertEqual(val, b'')

def test_uninitialized_isolation_level(self):
"""
Previously trying to get the isolation level of an uninitialized Connection
caused a segfault, now it should just return None.
Copy link
Contributor

Choose a reason for hiding this comment

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

Other getters raise a ProgrammingError when they are called on an uninitialized Connection object.
IMHO this should be the same here.

"""
conn = sqlite.Connection.__new__(sqlite.Connection)
with self.assertRaises(sqlite.ProgrammingError):
conn.isolation_level

def test_cursor_invalid_isolation_level(self):
"""
When trying to call conn.cursor() when conn is a Connection object that
was not initialized properly, it caused a segfault. Now it should raise
a ProgrammingError.
"""
conn = sqlite.Connection.__new__(sqlite.Connection)
self.assertRaises(ValueError, conn.__init__, '', isolation_level='invalid isolation level')
self.assertRaises(sqlite.ProgrammingError, conn.cursor)


def test_close_invalid_connection(self):
"""
Trying to call close() on a connection which was not initialized properly,
it caused a segfault. Now it should raise a ProgrammingError.
"""
conn = sqlite.Connection.__new__(sqlite.Connection)
self.assertRaises(sqlite.ProgrammingError, conn.close)

def suite():
tests = [
Expand Down
31 changes: 31 additions & 0 deletions Modules/_sqlite/connection.c
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,15 @@ pysqlite_connection_init(pysqlite_Connection *self, PyObject *args,
if (!isolation_level) {
isolation_level = PyUnicode_FromString("");
if (!isolation_level) {
PyErr_SetString(pysqlite_ProgrammingError, "Isolation level could not be set.");
return -1;
}
} else {
Py_INCREF(isolation_level);
}

self->initialized = 1;

Py_CLEAR(self->isolation_level);
if (pysqlite_connection_set_isolation_level(self, isolation_level, NULL) < 0) {
Py_DECREF(isolation_level);
Expand Down Expand Up @@ -250,8 +254,19 @@ pysqlite_connection_dealloc(pysqlite_Connection *self)
*/
int pysqlite_connection_register_cursor(pysqlite_Connection* connection, PyObject* cursor)
{
if (!connection || !connection->cursors) {
PyErr_Format(pysqlite_ProgrammingError,
"Base Connection.__init__ not called.");
goto error;
}
PyObject* weakref;

if (!connection->cursors) {
PyErr_SetString(pysqlite_ProgrammingError,
"Base Connection.__init__ not called.");
goto error;
}

weakref = PyWeakref_NewRef((PyObject*)cursor, NULL);
if (!weakref) {
goto error;
Expand Down Expand Up @@ -281,6 +296,11 @@ static PyObject *
pysqlite_connection_cursor_impl(pysqlite_Connection *self, PyObject *factory)
/*[clinic end generated code: output=562432a9e6af2aa1 input=4127345aa091b650]*/
{
if (self == NULL) {
return NULL;
}

static char *kwlist[] = {"factory", NULL};
Comment on lines +299 to +303
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed if self->initialized is only set if pysqlite_connection_init was successful. Also, it looks like the rebase left some artifacts :)

PyObject* cursor;

if (!pysqlite_check_thread(self) || !pysqlite_check_connection(self)) {
Expand Down Expand Up @@ -323,6 +343,12 @@ pysqlite_connection_close_impl(pysqlite_Connection *self)
/*[clinic end generated code: output=a546a0da212c9b97 input=3d58064bbffaa3d3]*/
{
int rc;

if (!self->statements) {
PyErr_SetString(pysqlite_ProgrammingError,
"Base Connection.__init__ not called.");
return NULL;
}
Comment on lines +346 to +351
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to handle this in pysqlite_do_all_statements, as it has multiple callers? Perhaps pysqlite_do_all_statements could be split in two: do_all_statements and do_all_cursors. Each of them would do a pointer sanity check before looping.

Copy link
Author

Choose a reason for hiding this comment

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

Are you sure about splitting pysqlite_do_all_statements into two functions? It seems like it always performs the statements part, but optionally also performs the cursors part.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I'm not sure :) Let's leave it as it is; both pysqlite_do_all_statements and this particular change.

Nit: if (self->statements == NULL) is more explicit, and I believe more consistent with the rest of the code base.


if (!pysqlite_check_thread(self)) {
return NULL;
Expand Down Expand Up @@ -1225,6 +1251,11 @@ int pysqlite_check_thread(pysqlite_Connection* self)

static PyObject* pysqlite_connection_get_isolation_level(pysqlite_Connection* self, void* unused)
{
if (!self || !self->isolation_level) {
PyErr_Format(pysqlite_ProgrammingError,
"Object is null or isolation_level is uninitialized.");
return 0;
}
Comment on lines +1254 to +1258
Copy link
Contributor

Choose a reason for hiding this comment

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

If self->initialized is only set if pysqlite_connection_init was successful, this can be reduced to:

Suggested change
if (!self || !self->isolation_level) {
PyErr_Format(pysqlite_ProgrammingError,
"Object is null or isolation_level is uninitialized.");
return 0;
}
if (!pysqlite_check_connection(self)) {
return NULL;
}

return Py_NewRef(self->isolation_level);
}

Expand Down