From 19f2c47e20d00566fc44c088994e00cde7e604f9 Mon Sep 17 00:00:00 2001 From: Erlend Egeberg Aasland Date: Tue, 3 May 2022 17:03:06 -0600 Subject: [PATCH 1/3] [3.7] gh-80254: Disallow recursive usage of cursors in `sqlite3` converters (cherry picked from commit c908dc5b4798c311981bd7e1f7d92fb623ee448b) Co-authored-by: Sergey Fedoseev Co-authored-by: Jelle Zijlstra --- Lib/sqlite3/test/regression.py | 40 +++++++++++++++++++ .../2019-06-22-11-01-45.bpo-36073.ED8mB9.rst | 2 + Modules/_sqlite/cursor.c | 30 +++++++++++--- 3 files changed, 66 insertions(+), 6 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2019-06-22-11-01-45.bpo-36073.ED8mB9.rst diff --git a/Lib/sqlite3/test/regression.py b/Lib/sqlite3/test/regression.py index 25c58f562d4c3f..7672b38df60b17 100644 --- a/Lib/sqlite3/test/regression.py +++ b/Lib/sqlite3/test/regression.py @@ -27,6 +27,9 @@ import weakref from test import support +from unittest.mock import patch + + class RegressionTests(unittest.TestCase): def setUp(self): self.con = sqlite.connect(":memory:") @@ -444,11 +447,48 @@ class UnhashableType(type): self.con.execute('SELECT %s()' % aggr_name) +class RecursiveUseOfCursors(unittest.TestCase): + # GH-80254: sqlite3 should not segfault for recursive use of cursors. + msg = "Recursive use of cursors not allowed" + + def setUp(self): + self.con = sqlite.connect(":memory:", + detect_types=sqlite.PARSE_COLNAMES) + self.cur = self.con.cursor() + self.cur.execute("create table test(x foo)") + self.cur.executemany("insert into test(x) values (?)", + [("foo",), ("bar",)]) + + def tearDown(self): + self.cur.close() + self.con.close() + + def test_recursive_cursor_init(self): + conv = lambda x: self.cur.__init__(self.con) + with patch.dict(sqlite.converters, {"INIT": conv}): + with self.assertRaisesRegex(sqlite.ProgrammingError, self.msg): + self.cur.execute(f'select x as "x [INIT]", x from test') + + def test_recursive_cursor_close(self): + conv = lambda x: self.cur.close() + with patch.dict(sqlite.converters, {"CLOSE": conv}): + with self.assertRaisesRegex(sqlite.ProgrammingError, self.msg): + self.cur.execute(f'select x as "x [CLOSE]", x from test') + + def test_recursive_cursor_fetch(self): + conv = lambda x, l=[]: self.cur.fetchone() if l else l.append(None) + with patch.dict(sqlite.converters, {"ITER": conv}): + self.cur.execute(f'select x as "x [ITER]", x from test') + with self.assertRaisesRegex(sqlite.ProgrammingError, self.msg): + self.cur.fetchall() + + def suite(): regression_suite = unittest.makeSuite(RegressionTests, "Check") return unittest.TestSuite(( regression_suite, unittest.makeSuite(UnhashableCallbacksTestCase), + unittest.makeSuite(RecursiveUseOfCursors), )) def test(): diff --git a/Misc/NEWS.d/next/Library/2019-06-22-11-01-45.bpo-36073.ED8mB9.rst b/Misc/NEWS.d/next/Library/2019-06-22-11-01-45.bpo-36073.ED8mB9.rst new file mode 100644 index 00000000000000..6c214d8191601c --- /dev/null +++ b/Misc/NEWS.d/next/Library/2019-06-22-11-01-45.bpo-36073.ED8mB9.rst @@ -0,0 +1,2 @@ +Raise :exc:`~sqlite3.ProgrammingError` instead of segfaulting on recursive +usage of cursors in :mod:`sqlite3` converters. Patch by Sergey Fedoseev. diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c index 6c98a99bd02573..6566ab89dc6c53 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -27,10 +27,25 @@ PyObject* pysqlite_cursor_iternext(pysqlite_Cursor* self); +static inline int +check_cursor_locked(pysqlite_Cursor *cur) +{ + if (cur->locked) { + PyErr_SetString(pysqlite_ProgrammingError, + "Recursive use of cursors not allowed."); + return 0; + } + return 1; +} + static const char errmsg_fetch_across_rollback[] = "Cursor needed to be reset because of commit/rollback and can no longer be fetched from."; static int pysqlite_cursor_init(pysqlite_Cursor* self, PyObject* args, PyObject* kwargs) { + if (!check_cursor_locked(self)) { + return -1; + } + pysqlite_Connection* connection; if (!PyArg_ParseTuple(args, "O!", &pysqlite_ConnectionType, &connection)) @@ -376,12 +391,9 @@ static int check_cursor(pysqlite_Cursor* cur) return 0; } - if (cur->locked) { - PyErr_SetString(pysqlite_ProgrammingError, "Recursive use of cursors not allowed."); - return 0; - } - - return pysqlite_check_thread(cur->connection) && pysqlite_check_connection(cur->connection); + return (pysqlite_check_thread(cur->connection) + && pysqlite_check_connection(cur->connection) + && check_cursor_locked(cur)); } PyObject* _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* args) @@ -802,7 +814,9 @@ PyObject* pysqlite_cursor_iternext(pysqlite_Cursor *self) } if (rc == SQLITE_ROW) { + self->locked = 1; // GH-80254: Prevent recursive use of cursors. self->next_row = _pysqlite_fetch_one_row(self); + self->locked = 0; if (self->next_row == NULL) { (void)pysqlite_statement_reset(self->statement); return NULL; @@ -905,6 +919,10 @@ PyObject* pysqlite_noop(pysqlite_Connection* self, PyObject* args) PyObject* pysqlite_cursor_close(pysqlite_Cursor* self, PyObject* args) { + if (!check_cursor_locked(self)) { + return NULL; + } + if (!self->connection) { PyErr_SetString(pysqlite_ProgrammingError, "Base Cursor.__init__ not called."); From f6c89bc0c696f269001db5242b57ea96dac0a7a6 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Wed, 4 May 2022 18:22:56 -0600 Subject: [PATCH 2/3] Fix ref leak in pysqlite_cursor_iternext --- Modules/_sqlite/cursor.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c index 6566ab89dc6c53..b0aaa68eab08e1 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -802,15 +802,11 @@ PyObject* pysqlite_cursor_iternext(pysqlite_Cursor *self) if (self->statement) { rc = pysqlite_step(self->statement->st, self->connection); if (PyErr_Occurred()) { - (void)pysqlite_statement_reset(self->statement); - Py_DECREF(next_row); - return NULL; + goto error; } if (rc != SQLITE_DONE && rc != SQLITE_ROW) { - (void)pysqlite_statement_reset(self->statement); - Py_DECREF(next_row); _pysqlite_seterror(self->connection->db, NULL); - return NULL; + goto error; } if (rc == SQLITE_ROW) { @@ -818,13 +814,17 @@ PyObject* pysqlite_cursor_iternext(pysqlite_Cursor *self) self->next_row = _pysqlite_fetch_one_row(self); self->locked = 0; if (self->next_row == NULL) { - (void)pysqlite_statement_reset(self->statement); - return NULL; + goto error; } } } return next_row; + +error: + (void)pysqlite_statement_reset(self->statement); + Py_DECREF(next_row); + return NULL; } PyObject* pysqlite_cursor_fetchone(pysqlite_Cursor* self, PyObject* args) From d52fd31e76aac4811ddf9e2192f986f2df67fbc8 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Wed, 4 May 2022 23:40:10 -0500 Subject: [PATCH 3/3] Explicitly free resources at test tearDown() --- Lib/sqlite3/test/regression.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Lib/sqlite3/test/regression.py b/Lib/sqlite3/test/regression.py index 7672b38df60b17..ef0dce5fbaa44d 100644 --- a/Lib/sqlite3/test/regression.py +++ b/Lib/sqlite3/test/regression.py @@ -462,6 +462,8 @@ def setUp(self): def tearDown(self): self.cur.close() self.con.close() + del self.cur + del self.con def test_recursive_cursor_init(self): conv = lambda x: self.cur.__init__(self.con)