From c79e7846554bd36f85572bbc0f23d47bf8f0ea6f Mon Sep 17 00:00:00 2001 From: Sergey Fedoseev Date: Tue, 12 Feb 2019 13:44:01 +0500 Subject: [PATCH 1/8] bpo-36073: Raise ProgrammingError on recursive usage of cursors in sqlite converters --- Lib/sqlite3/test/regression.py | 37 ++++++++++++++++++- .../2019-06-22-11-01-45.bpo-36073.ED8mB9.rst | 2 + Modules/_sqlite/cursor.c | 32 +++++++++++++--- 3 files changed, 64 insertions(+), 7 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 417a53109c87c7..b206e2e48f0493 100644 --- a/Lib/sqlite3/test/regression.py +++ b/Lib/sqlite3/test/regression.py @@ -415,9 +415,44 @@ def test_return_empty_bytestring(self): self.assertEqual(val, b'') +class ConverterProgrammingErrorTestCase(unittest.TestCase): + 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)') + + sqlite.converters['CURSOR_INIT'] = lambda x: self.cur.__init__(self.con) + sqlite.converters['CURSOR_CLOSE'] = lambda x: self.cur.close() + sqlite.converters['CURSOR_ITER'] = lambda x, l=[]: self.cur.fetchone() if l else l.append(None) + + def tearDown(self): + del sqlite.converters['CURSOR_INIT'] + del sqlite.converters['CURSOR_CLOSE'] + del sqlite.converters['CURSOR_ITER'] + self.cur.close() + self.con.close() + + def test_cursor_init(self): + self.cur.execute('insert into test(x) values (?)', ('foo',)) + with self.assertRaises(sqlite.ProgrammingError): + self.cur.execute('select x as "x [CURSOR_INIT]", x from test') + + def test_cursor_close(self): + self.cur.execute('insert into test(x) values (?)', ('foo',)) + with self.assertRaises(sqlite.ProgrammingError): + self.cur.execute('select x as "x [CURSOR_CLOSE]", x from test') + + def test_cursor_iter(self): + self.cur.executemany('insert into test(x) values (?)', (('foo',),) * 2) + self.cur.execute('select x as "x [CURSOR_ITER]", x from test') + with self.assertRaises(sqlite.ProgrammingError): + self.cur.fetchone() + + def suite(): tests = [ - RegressionTests + RegressionTests, + ConverterProgrammingErrorTestCase, ] return unittest.TestSuite( [unittest.TestLoader().loadTestsFromTestCase(t) for t in tests] 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..bee0168dd52318 --- /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` 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 dfaa5577ab4081..31698ac47ea4b5 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -26,6 +26,19 @@ #include "util.h" #include "clinic/cursor.c.h" +static int +check_cursor_locked(pysqlite_Cursor* cur) +{ + if (cur->locked) { + PyErr_SetString( + pysqlite_ProgrammingError, + "Recursive use of cursors not allowed." + ); + return 0; + } + return 1; +} + /*[clinic input] module _sqlite3 class _sqlite3.Cursor "pysqlite_Cursor *" "pysqlite_CursorType" @@ -47,6 +60,10 @@ pysqlite_cursor_init_impl(pysqlite_Cursor *self, pysqlite_Connection *connection) /*[clinic end generated code: output=ac59dce49a809ca8 input=a8a4f75ac90999b2]*/ { + if (!check_cursor_locked(self)) { + return -1; + } + Py_INCREF(connection); Py_XSETREF(self->connection, connection); Py_CLEAR(self->statement); @@ -384,12 +401,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 check_cursor_locked(cur) && + pysqlite_check_thread(cur->connection) && + pysqlite_check_connection(cur->connection); } static PyObject * @@ -811,7 +825,9 @@ pysqlite_cursor_iternext(pysqlite_Cursor *self) } if (rc == SQLITE_ROW) { + self->locked = 1; self->next_row = _pysqlite_fetch_one_row(self); + self->locked = 0; if (self->next_row == NULL) { (void)pysqlite_statement_reset(self->statement); return NULL; @@ -962,6 +978,10 @@ static PyObject * pysqlite_cursor_close_impl(pysqlite_Cursor *self) /*[clinic end generated code: output=b6055e4ec6fe63b6 input=08b36552dbb9a986]*/ { + if (!check_cursor_locked(self)) { + return NULL; + } + if (!self->connection) { PyErr_SetString(pysqlite_ProgrammingError, "Base Cursor.__init__ not called."); From bb0a729e53e8200810aea2ace3d62b588554b04f Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 19 Oct 2021 11:44:20 +0200 Subject: [PATCH 2/8] Use subTest --- Lib/sqlite3/test/test_regression.py | 37 +++++++++++------------------ 1 file changed, 14 insertions(+), 23 deletions(-) diff --git a/Lib/sqlite3/test/test_regression.py b/Lib/sqlite3/test/test_regression.py index fc7d5ec84dd3d7..df7678117f6bbf 100644 --- a/Lib/sqlite3/test/test_regression.py +++ b/Lib/sqlite3/test/test_regression.py @@ -489,36 +489,27 @@ def test_executescript_step_through_select(self): class ConverterProgrammingErrorTestCase(unittest.TestCase): def setUp(self): - self.con = sqlite.connect(':memory:', detect_types=sqlite.PARSE_COLNAMES) + 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.execute("create table test(x foo)") + self.cur.executemany("insert into test(x) values (?)", [("foo",), ("bar",)]) - sqlite.converters['CURSOR_INIT'] = lambda x: self.cur.__init__(self.con) - sqlite.converters['CURSOR_CLOSE'] = lambda x: self.cur.close() - sqlite.converters['CURSOR_ITER'] = lambda x, l=[]: self.cur.fetchone() if l else l.append(None) + sqlite.converters["CURSOR_INIT"] = lambda x: self.cur.__init__(self.con) + sqlite.converters["CURSOR_CLOSE"] = lambda x: self.cur.close() + sqlite.converters["CURSOR_ITER"] = lambda x, l=[]: self.cur.fetchone() if l else l.append(None) def tearDown(self): - del sqlite.converters['CURSOR_INIT'] - del sqlite.converters['CURSOR_CLOSE'] - del sqlite.converters['CURSOR_ITER'] + del sqlite.converters["CURSOR_INIT"] + del sqlite.converters["CURSOR_CLOSE"] + del sqlite.converters["CURSOR_ITER"] self.cur.close() self.con.close() - def test_cursor_init(self): - self.cur.execute('insert into test(x) values (?)', ('foo',)) - with self.assertRaises(sqlite.ProgrammingError): - self.cur.execute('select x as "x [CURSOR_INIT]", x from test') - - def test_cursor_close(self): - self.cur.execute('insert into test(x) values (?)', ('foo',)) - with self.assertRaises(sqlite.ProgrammingError): - self.cur.execute('select x as "x [CURSOR_CLOSE]", x from test') - - def test_cursor_iter(self): - self.cur.executemany('insert into test(x) values (?)', (('foo',),) * 2) - self.cur.execute('select x as "x [CURSOR_ITER]", x from test') - with self.assertRaises(sqlite.ProgrammingError): - self.cur.fetchone() + def test_recursive_cursor_usage(self): + for converter in "CURSOR_INIT", "CURSOR_CLOSE", "CURSOR_ITER": + with self.subTest(converter=converter): + self.cur.execute(f'select x as "x [{converter}]", x from test') + self.assertRaises(sqlite.ProgrammingError, self.cur.fetchall) if __name__ == "__main__": From 51ab5518d6f4b55a782fd910d2382a9fa32b626a Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 19 Oct 2021 14:02:28 +0200 Subject: [PATCH 3/8] Address review: Apply Sergey's suggestions --- Lib/sqlite3/test/test_regression.py | 38 +++++++++++++++++------------ 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/Lib/sqlite3/test/test_regression.py b/Lib/sqlite3/test/test_regression.py index df7678117f6bbf..435e30667b1b90 100644 --- a/Lib/sqlite3/test/test_regression.py +++ b/Lib/sqlite3/test/test_regression.py @@ -26,9 +26,10 @@ import sqlite3 as sqlite import weakref import functools -from test import support from .test_dbapi import managed_connect +from test import support +from unittest.mock import patch class RegressionTests(unittest.TestCase): def setUp(self): @@ -489,27 +490,34 @@ def test_executescript_step_through_select(self): class ConverterProgrammingErrorTestCase(unittest.TestCase): def setUp(self): - self.con = sqlite.connect(":memory:", detect_types=sqlite.PARSE_COLNAMES) + 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",)]) - - sqlite.converters["CURSOR_INIT"] = lambda x: self.cur.__init__(self.con) - sqlite.converters["CURSOR_CLOSE"] = lambda x: self.cur.close() - sqlite.converters["CURSOR_ITER"] = lambda x, l=[]: self.cur.fetchone() if l else l.append(None) + self.cur.executemany("insert into test(x) values (?)", + [("foo",), ("bar",)]) def tearDown(self): - del sqlite.converters["CURSOR_INIT"] - del sqlite.converters["CURSOR_CLOSE"] - del sqlite.converters["CURSOR_ITER"] self.cur.close() self.con.close() - def test_recursive_cursor_usage(self): - for converter in "CURSOR_INIT", "CURSOR_CLOSE", "CURSOR_ITER": - with self.subTest(converter=converter): - self.cur.execute(f'select x as "x [{converter}]", x from test') - self.assertRaises(sqlite.ProgrammingError, self.cur.fetchall) + def test_recursive_cursor_init(self): + conv = lambda x: self.cur.__init__(self.con) + with patch.dict(sqlite.converters, {"INIT": conv}): + self.cur.execute(f'select x as "x [INIT]", x from test') + self.assertRaises(sqlite.ProgrammingError, self.cur.fetchall) + + def test_recursive_cursor_close(self): + conv = lambda x: self.cur.close() + with patch.dict(sqlite.converters, {"CLOSE": conv}): + self.cur.execute(f'select x as "x [CLOSE]", x from test') + self.assertRaises(sqlite.ProgrammingError, self.cur.fetchall) + + def test_recursive_cursor_iter(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') + self.assertRaises(sqlite.ProgrammingError, self.cur.fetchall) if __name__ == "__main__": From f737b6edbfa60f5ffcbd5e9da9e1f3c488952b39 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 19 Oct 2021 14:06:28 +0200 Subject: [PATCH 4/8] Also check exception message --- Lib/sqlite3/test/test_regression.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/Lib/sqlite3/test/test_regression.py b/Lib/sqlite3/test/test_regression.py index 435e30667b1b90..b37091bf9942d6 100644 --- a/Lib/sqlite3/test/test_regression.py +++ b/Lib/sqlite3/test/test_regression.py @@ -489,6 +489,8 @@ def test_executescript_step_through_select(self): class ConverterProgrammingErrorTestCase(unittest.TestCase): + msg = "Recursive use of cursors not allowed" + def setUp(self): self.con = sqlite.connect(":memory:", detect_types=sqlite.PARSE_COLNAMES) @@ -505,19 +507,22 @@ def test_recursive_cursor_init(self): conv = lambda x: self.cur.__init__(self.con) with patch.dict(sqlite.converters, {"INIT": conv}): self.cur.execute(f'select x as "x [INIT]", x from test') - self.assertRaises(sqlite.ProgrammingError, self.cur.fetchall) + self.assertRaisesRegex(sqlite.ProgrammingError, self.msg, + self.cur.fetchall) def test_recursive_cursor_close(self): conv = lambda x: self.cur.close() with patch.dict(sqlite.converters, {"CLOSE": conv}): self.cur.execute(f'select x as "x [CLOSE]", x from test') - self.assertRaises(sqlite.ProgrammingError, self.cur.fetchall) + self.assertRaisesRegex(sqlite.ProgrammingError, self.msg, + self.cur.fetchall) def test_recursive_cursor_iter(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') - self.assertRaises(sqlite.ProgrammingError, self.cur.fetchall) + self.assertRaisesRegex(sqlite.ProgrammingError, self.msg, + self.cur.fetchall) if __name__ == "__main__": From ee8c8c21aaa79a63189e67076ac3bf7fc40dd069 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 2 May 2022 13:59:00 -0600 Subject: [PATCH 5/8] Make NEWS entry more accurate --- .../next/Library/2019-06-22-11-01-45.bpo-36073.ED8mB9.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 index bee0168dd52318..6c214d8191601c 100644 --- 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 @@ -1,2 +1,2 @@ -Raise :exc:`~sqlite3.ProgrammingError` on recursive usage of cursors in -:mod:`sqlite3` converters. Patch by Sergey Fedoseev. +Raise :exc:`~sqlite3.ProgrammingError` instead of segfaulting on recursive +usage of cursors in :mod:`sqlite3` converters. Patch by Sergey Fedoseev. From abd7d108084b25f2b1be9a0120dd63f9e1e3a064 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 2 May 2022 14:31:58 -0600 Subject: [PATCH 6/8] Inline check_cursor_locked --- Modules/_sqlite/cursor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c index 78157cd8a70e77..8962c29e166590 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -29,7 +29,7 @@ #include "clinic/cursor.c.h" #undef clinic_state -static int +static inline int check_cursor_locked(pysqlite_Cursor *cur) { if (cur->locked) { From 85e1386d173c1a2fc45b883d02b48267d9a04617 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 2 May 2022 14:36:19 -0600 Subject: [PATCH 7/8] Add comment, explaining why we do this --- Modules/_sqlite/cursor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c index 8962c29e166590..8618089c2fbb04 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -806,7 +806,7 @@ pysqlite_cursor_iternext(pysqlite_Cursor *self) return NULL; } - self->locked = 1; + self->locked = 1; // GH-80254: Prevent recursive use of cursors. PyObject *row = _pysqlite_fetch_one_row(self); self->locked = 0; if (row == NULL) { From 7bcfc631ef3b37d916e88be2cc7b332fc252b125 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 2 May 2022 14:40:09 -0600 Subject: [PATCH 8/8] Also mention issue number in regression tests --- Lib/test/test_sqlite3/test_regression.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_sqlite3/test_regression.py b/Lib/test/test_sqlite3/test_regression.py index 785510b2731751..19bb84bf38a367 100644 --- a/Lib/test/test_sqlite3/test_regression.py +++ b/Lib/test/test_sqlite3/test_regression.py @@ -470,7 +470,8 @@ def test_executescript_step_through_select(self): self.assertEqual(steps, values) -class ConverterProgrammingErrorTestCase(unittest.TestCase): +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):