From bbe3a0ebb700ba2c1fbf348e74ca4b2dcfd84a48 Mon Sep 17 00:00:00 2001 From: Liel Fridman Date: Wed, 11 Oct 2017 00:40:58 +0300 Subject: [PATCH 1/9] bpo-31746: Fixed Segfaults in the sqlite module when uninitialized. --- Modules/_sqlite/connection.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 5f8e41b6169a76..b3aa2a146a6b35 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -250,6 +250,12 @@ pysqlite_connection_dealloc(pysqlite_Connection *self) */ int pysqlite_connection_register_cursor(pysqlite_Connection* connection, PyObject* cursor) { + if (!connection || !connection->cursors) { + PyErr_Format(PyExc_RuntimeError, + "Tried to get a cursor of an uninitialized connection." + ); + goto error; + } PyObject* weakref; weakref = PyWeakref_NewRef((PyObject*)cursor, NULL); @@ -1225,6 +1231,9 @@ int pysqlite_check_thread(pysqlite_Connection* self) static PyObject* pysqlite_connection_get_isolation_level(pysqlite_Connection* self, void* unused) { + if (!self || !self->isolation_level) { + Py_RETURN_NONE; + } return Py_NewRef(self->isolation_level); } From b57a6af01ddf2708288e376cf6d54b42dec152ec Mon Sep 17 00:00:00 2001 From: Liel Fridman Date: Wed, 11 Oct 2017 14:16:07 +0300 Subject: [PATCH 2/9] Added regression tests, changed RuntimeError to ProgrammingError, and made closing an uninitialized connection throw a ProgrammingError instead of segfaulting --- Lib/sqlite3/test/regression.py | 31 +++++++++++++++++++++++++++++++ Modules/_sqlite/connection.c | 10 +++++++++- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/Lib/sqlite3/test/regression.py b/Lib/sqlite3/test/regression.py index 417a53109c87c7..676e53170e8fc5 100644 --- a/Lib/sqlite3/test/regression.py +++ b/Lib/sqlite3/test/regression.py @@ -414,6 +414,37 @@ def test_return_empty_bytestring(self): val = cur.fetchone()[0] self.assertEqual(val, b'') + def CheckUninitializedIsolationLevel(self): + """ + Previously trying to get the isolation level of an uninitialized Connection + caused a segfault, now it should just return None. + """ + conn = sqlite.Connection.__new__(sqlite.Connection) + self.assertEqual(conn.isolation_level, None) + + + def CheckCursorInvalidIsolationLevel(self): + """ + When trying to call conn.corsor() when conn is a Connection object that + was not initialized properly, it caused a segfault. Now it should raise + a ProgrammingError. + """ + try: + conn = sqlite.Connection.__new__(sqlite.Connection) + conn.__init__('', isolation_level='invalid isolation level') + except ValueError: + pass + + self.assertRaises(sqlite.ProgrammingError, conn.cursor) + + + def CheckCloseInvalidConnection(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 = [ diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index b3aa2a146a6b35..54b036477f8f5d 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -123,6 +123,9 @@ pysqlite_connection_init(pysqlite_Connection *self, PyObject *args, } 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); @@ -251,7 +254,7 @@ pysqlite_connection_dealloc(pysqlite_Connection *self) int pysqlite_connection_register_cursor(pysqlite_Connection* connection, PyObject* cursor) { if (!connection || !connection->cursors) { - PyErr_Format(PyExc_RuntimeError, + PyErr_Format(pysqlite_ProgrammingError, "Tried to get a cursor of an uninitialized connection." ); goto error; @@ -329,6 +332,11 @@ pysqlite_connection_close_impl(pysqlite_Connection *self) /*[clinic end generated code: output=a546a0da212c9b97 input=3d58064bbffaa3d3]*/ { int rc; + + if (!self->statements) { + PyErr_SetString(pysqlite_ProgrammingError, "Trying to close connection which was not initialized properly."); + return NULL; + } if (!pysqlite_check_thread(self)) { return NULL; From 9955edf9270f495fec6606922c1d8f07223457e4 Mon Sep 17 00:00:00 2001 From: Liel Fridman Date: Fri, 13 Oct 2017 17:38:58 +0300 Subject: [PATCH 3/9] Fixing typo, identation and tests according to reviews. --- Lib/sqlite3/test/regression.py | 10 +++------- Modules/_sqlite/connection.c | 5 ++--- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/Lib/sqlite3/test/regression.py b/Lib/sqlite3/test/regression.py index 676e53170e8fc5..79c908c5cd58d6 100644 --- a/Lib/sqlite3/test/regression.py +++ b/Lib/sqlite3/test/regression.py @@ -425,16 +425,12 @@ def CheckUninitializedIsolationLevel(self): def CheckCursorInvalidIsolationLevel(self): """ - When trying to call conn.corsor() when conn is a Connection object that + 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. """ - try: - conn = sqlite.Connection.__new__(sqlite.Connection) - conn.__init__('', isolation_level='invalid isolation level') - except ValueError: - pass - + conn = sqlite.Connection.__new__(sqlite.Connection) + self.assertRaises(ValueError, conn.__init__, '', isolation_level='invalid isolation level') self.assertRaises(sqlite.ProgrammingError, conn.cursor) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 54b036477f8f5d..bea6eab4d034f8 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -255,9 +255,8 @@ int pysqlite_connection_register_cursor(pysqlite_Connection* connection, PyObjec { if (!connection || !connection->cursors) { PyErr_Format(pysqlite_ProgrammingError, - "Tried to get a cursor of an uninitialized connection." - ); - goto error; + "Tried to get a cursor of an uninitialized connection."); + goto error; } PyObject* weakref; From c0920c84142e04b7eb67049d287ca51b239792de Mon Sep 17 00:00:00 2001 From: Liel Fridman Date: Mon, 3 Jun 2019 17:10:37 +0300 Subject: [PATCH 4/9] The first example code doesn't crash anymore. --- Modules/_sqlite/connection.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index bea6eab4d034f8..4b899e2e69f599 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -289,6 +289,12 @@ 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}; + PyObject* factory = NULL; PyObject* cursor; if (!pysqlite_check_thread(self) || !pysqlite_check_connection(self)) { From 77b29462854bfee1e146b2e49d6291a9ff1569d3 Mon Sep 17 00:00:00 2001 From: Liel Fridman Date: Mon, 3 Jun 2019 17:47:52 +0300 Subject: [PATCH 5/9] No more crashes from #31746 --- Modules/_sqlite/connection.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 4b899e2e69f599..69824b85beecb3 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -260,6 +260,11 @@ int pysqlite_connection_register_cursor(pysqlite_Connection* connection, PyObjec } PyObject* weakref; + if (!connection->cursors) { + PyErr_SetString(pysqlite_ProgrammingError, "Could not get the cursors list."); + return 0; + } + weakref = PyWeakref_NewRef((PyObject*)cursor, NULL); if (!weakref) { goto error; From 0fcd07525e61d7dea3132cea947cb44eb2868fb5 Mon Sep 17 00:00:00 2001 From: Liel Fridman Date: Mon, 3 Jun 2019 18:05:17 +0300 Subject: [PATCH 6/9] Fixed C whitespace error --- Modules/_sqlite/connection.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 69824b85beecb3..f1852159194e57 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -294,10 +294,10 @@ static PyObject * pysqlite_connection_cursor_impl(pysqlite_Connection *self, PyObject *factory) /*[clinic end generated code: output=562432a9e6af2aa1 input=4127345aa091b650]*/ { - if (self == NULL) { - return NULL; - } - + if (self == NULL) { + return NULL; + } + static char *kwlist[] = {"factory", NULL}; PyObject* factory = NULL; PyObject* cursor; From d8b4dd67b2a77e2e35c5b028a5bb296f7236458f Mon Sep 17 00:00:00 2001 From: Liel Fridman Date: Wed, 28 Apr 2021 23:07:40 +0300 Subject: [PATCH 7/9] Fixing the files according to the feedback in the PR --- Lib/sqlite3/test/regression.py | 10 +++++----- Modules/_sqlite/connection.c | 22 +++++++++++++--------- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/Lib/sqlite3/test/regression.py b/Lib/sqlite3/test/regression.py index 79c908c5cd58d6..3ddd2f8ec5da5a 100644 --- a/Lib/sqlite3/test/regression.py +++ b/Lib/sqlite3/test/regression.py @@ -414,16 +414,16 @@ def test_return_empty_bytestring(self): val = cur.fetchone()[0] self.assertEqual(val, b'') - def CheckUninitializedIsolationLevel(self): + 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. """ conn = sqlite.Connection.__new__(sqlite.Connection) - self.assertEqual(conn.isolation_level, None) - + with self.assertRaises(sqlite.ProgrammingError): + conn.isolation_level - def CheckCursorInvalidIsolationLevel(self): + 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 @@ -434,7 +434,7 @@ def CheckCursorInvalidIsolationLevel(self): self.assertRaises(sqlite.ProgrammingError, conn.cursor) - def CheckCloseInvalidConnection(self): + 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. diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index f1852159194e57..b52d1819a903c2 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -118,6 +118,7 @@ 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 { @@ -255,14 +256,15 @@ int pysqlite_connection_register_cursor(pysqlite_Connection* connection, PyObjec { if (!connection || !connection->cursors) { PyErr_Format(pysqlite_ProgrammingError, - "Tried to get a cursor of an uninitialized connection."); + "Base Connection.__init__ not called."); goto error; } PyObject* weakref; if (!connection->cursors) { - PyErr_SetString(pysqlite_ProgrammingError, "Could not get the cursors list."); - return 0; + PyErr_SetString(pysqlite_ProgrammingError, + "Base Connection.__init__ not called."); + goto error; } weakref = PyWeakref_NewRef((PyObject*)cursor, NULL); @@ -294,12 +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; - } + if (self == NULL) { + return NULL; + } static char *kwlist[] = {"factory", NULL}; - PyObject* factory = NULL; PyObject* cursor; if (!pysqlite_check_thread(self) || !pysqlite_check_connection(self)) { @@ -344,7 +345,8 @@ pysqlite_connection_close_impl(pysqlite_Connection *self) int rc; if (!self->statements) { - PyErr_SetString(pysqlite_ProgrammingError, "Trying to close connection which was not initialized properly."); + PyErr_SetString(pysqlite_ProgrammingError, + "Base Connection.__init__ not called."); return NULL; } @@ -1250,7 +1252,9 @@ int pysqlite_check_thread(pysqlite_Connection* self) static PyObject* pysqlite_connection_get_isolation_level(pysqlite_Connection* self, void* unused) { if (!self || !self->isolation_level) { - Py_RETURN_NONE; + PyErr_Format(pysqlite_ProgrammingError, + "Object is null or isolation_level is uninitialized."); + return 0; } return Py_NewRef(self->isolation_level); } From 51e7a9f8fd3ae2f296e5ead7fd8dca219656eb0e Mon Sep 17 00:00:00 2001 From: Liel Fridman Date: Wed, 28 Apr 2021 23:09:20 +0300 Subject: [PATCH 8/9] Adding whitespace at the end of regression.py --- Lib/sqlite3/test/regression.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/sqlite3/test/regression.py b/Lib/sqlite3/test/regression.py index 3ddd2f8ec5da5a..a1056711350820 100644 --- a/Lib/sqlite3/test/regression.py +++ b/Lib/sqlite3/test/regression.py @@ -456,3 +456,4 @@ def test(): if __name__ == "__main__": test() + From 84326a23fe5ced0383f28147134d03ca39d0d9f3 Mon Sep 17 00:00:00 2001 From: Liel Fridman Date: Wed, 28 Apr 2021 23:10:45 +0300 Subject: [PATCH 9/9] Fixing unnecessary extra whitespace in regression.py --- Lib/sqlite3/test/regression.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/sqlite3/test/regression.py b/Lib/sqlite3/test/regression.py index a1056711350820..3ddd2f8ec5da5a 100644 --- a/Lib/sqlite3/test/regression.py +++ b/Lib/sqlite3/test/regression.py @@ -456,4 +456,3 @@ def test(): if __name__ == "__main__": test() -