From c8bcf3b2b8f0a469f659af56d53057bd9e1018f0 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Wed, 8 Jun 2022 14:55:26 +0200 Subject: [PATCH 01/21] Add tests --- Lib/test/test_sqlite3/test_dbapi.py | 15 +++++++++++++++ .../2022-06-06-12-58-27.gh-issue-79579.e8rB-M.rst | 2 ++ 2 files changed, 17 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2022-06-06-12-58-27.gh-issue-79579.e8rB-M.rst diff --git a/Lib/test/test_sqlite3/test_dbapi.py b/Lib/test/test_sqlite3/test_dbapi.py index 05180a3616c5d4..6a468a2bfa5762 100644 --- a/Lib/test/test_sqlite3/test_dbapi.py +++ b/Lib/test/test_sqlite3/test_dbapi.py @@ -906,6 +906,21 @@ def test_rowcount_update_returning(self): self.assertEqual(self.cu.fetchone()[0], 1) self.assertEqual(self.cu.rowcount, 1) + def test_rowcount_prefixed_with_comment(self): + # gh-79579: rowcount is updated even if query is prefixed with comments + self.cu.execute("/* foo */ insert into test(name) values (?)", ('foo',)) + self.assertEqual(self.cu.rowcount, 1) + self.cu.execute("/* bar */ update test set name='bar' where name='foo'") + self.assertEqual(self.cu.rowcount, 2) + + def test_rowcount_vaccuum(self): + data = ((1,), (2,), (3,)) + self.cu.executemany("insert into test(income) values(?)", data) + self.assertEqual(self.cu.rowcount, 3) + self.cx.commit() + self.cu.execute("vacuum") + self.assertEqual(self.cu.rowcount, -1) + def test_total_changes(self): self.cu.execute("insert into test(name) values ('foo')") self.cu.execute("insert into test(name) values ('foo')") diff --git a/Misc/NEWS.d/next/Library/2022-06-06-12-58-27.gh-issue-79579.e8rB-M.rst b/Misc/NEWS.d/next/Library/2022-06-06-12-58-27.gh-issue-79579.e8rB-M.rst new file mode 100644 index 00000000000000..3c61b7e50365cb --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-06-06-12-58-27.gh-issue-79579.e8rB-M.rst @@ -0,0 +1,2 @@ +:data:`sqlite3.Cursor.rowcount` is now correctly updated for all datamodifying +SQL queries. Patch by Erlend E. Aasland. From d38e632068fc9377dee1b729d325a10f7ecdbbd3 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Thu, 9 Jun 2022 00:59:56 +0200 Subject: [PATCH 02/21] Modify pysqlite_check_remaining_sql to return char * iso. int --- Modules/_sqlite/statement.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index f9cb70f0ef146c..4ff846da88d4fb 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -26,7 +26,7 @@ #include "util.h" /* prototypes */ -static int pysqlite_check_remaining_sql(const char* tail); +static const char *pysqlite_check_remaining_sql(const char *sql); typedef enum { LINECOMMENT_1, @@ -146,16 +146,17 @@ stmt_traverse(pysqlite_Statement *self, visitproc visit, void *arg) * * Returns 1 if there is more left than should be. 0 if ok. */ -static int pysqlite_check_remaining_sql(const char* tail) +static const char * +pysqlite_check_remaining_sql(const char *sql) { - const char* pos = tail; + const char *pos = sql; parse_remaining_sql_state state = NORMAL; for (;;) { switch (*pos) { case 0: - return 0; + return NULL; case '-': if (state == NORMAL) { state = LINECOMMENT_1; @@ -178,14 +179,14 @@ static int pysqlite_check_remaining_sql(const char* tail) } else if (state == COMMENTEND_1) { state = NORMAL; } else if (state == COMMENTSTART_1) { - return 1; + return pos; } break; case '*': if (state == NORMAL) { - return 1; + return pos; } else if (state == LINECOMMENT_1) { - return 1; + return pos; } else if (state == COMMENTSTART_1) { state = IN_COMMENT; } else if (state == IN_COMMENT) { @@ -198,14 +199,14 @@ static int pysqlite_check_remaining_sql(const char* tail) } else if (state == IN_LINECOMMENT) { } else if (state == IN_COMMENT) { } else { - return 1; + return pos; } } pos++; } - return 0; + return NULL; } static PyType_Slot stmt_slots[] = { From 7fcca68a093a53ef8cc10dc3cd318f30596ad334 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Thu, 9 Jun 2022 01:01:16 +0200 Subject: [PATCH 03/21] Rename pysqlite_check_remaining_sql to lstrip_sql --- Lib/test/test_sqlite3/test_dbapi.py | 7 ++++++- Modules/_sqlite/statement.c | 6 +++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_sqlite3/test_dbapi.py b/Lib/test/test_sqlite3/test_dbapi.py index 6a468a2bfa5762..5c35f98628f5e4 100644 --- a/Lib/test/test_sqlite3/test_dbapi.py +++ b/Lib/test/test_sqlite3/test_dbapi.py @@ -906,9 +906,14 @@ def test_rowcount_update_returning(self): self.assertEqual(self.cu.fetchone()[0], 1) self.assertEqual(self.cu.rowcount, 1) + @unittest.skip("") def test_rowcount_prefixed_with_comment(self): # gh-79579: rowcount is updated even if query is prefixed with comments - self.cu.execute("/* foo */ insert into test(name) values (?)", ('foo',)) + #self.cu.execute("/* foo */ insert into test(name) values (?)", ('foo',)) + self.cu.execute(""" + -- foo + insert into test(name) values ('foo') + """) self.assertEqual(self.cu.rowcount, 1) self.cu.execute("/* bar */ update test set name='bar' where name='foo'") self.assertEqual(self.cu.rowcount, 2) diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index 4ff846da88d4fb..96abf9f88c66dc 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -26,7 +26,7 @@ #include "util.h" /* prototypes */ -static const char *pysqlite_check_remaining_sql(const char *sql); +static const char *lstrip_sql(const char *sql); typedef enum { LINECOMMENT_1, @@ -73,7 +73,7 @@ pysqlite_statement_create(pysqlite_Connection *connection, PyObject *sql) return NULL; } - if (pysqlite_check_remaining_sql(tail)) { + if (lstrip_sql(tail)) { PyErr_SetString(connection->ProgrammingError, "You can only execute one statement at a time."); goto error; @@ -147,7 +147,7 @@ stmt_traverse(pysqlite_Statement *self, visitproc visit, void *arg) * Returns 1 if there is more left than should be. 0 if ok. */ static const char * -pysqlite_check_remaining_sql(const char *sql) +lstrip_sql(const char *sql) { const char *pos = sql; From 3061dc57c73bce958e3f58ec51d31bd7b5f3d68d Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Thu, 9 Jun 2022 01:04:50 +0200 Subject: [PATCH 04/21] Use lstrip_sql() to strip comments and whitespace --- Lib/test/test_sqlite3/test_dbapi.py | 1 - Modules/_sqlite/statement.c | 12 ++---------- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/Lib/test/test_sqlite3/test_dbapi.py b/Lib/test/test_sqlite3/test_dbapi.py index 5c35f98628f5e4..612637202194de 100644 --- a/Lib/test/test_sqlite3/test_dbapi.py +++ b/Lib/test/test_sqlite3/test_dbapi.py @@ -906,7 +906,6 @@ def test_rowcount_update_returning(self): self.assertEqual(self.cu.fetchone()[0], 1) self.assertEqual(self.cu.rowcount, 1) - @unittest.skip("") def test_rowcount_prefixed_with_comment(self): # gh-79579: rowcount is updated even if query is prefixed with comments #self.cu.execute("/* foo */ insert into test(name) values (?)", ('foo',)) diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index 96abf9f88c66dc..f26665518c6c39 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -82,20 +82,12 @@ pysqlite_statement_create(pysqlite_Connection *connection, PyObject *sql) /* Determine if the statement is a DML statement. SELECT is the only exception. See #9924. */ int is_dml = 0; - for (const char *p = sql_cstr; *p != 0; p++) { - switch (*p) { - case ' ': - case '\r': - case '\n': - case '\t': - continue; - } - + const char *p = lstrip_sql(sql_cstr); + if (p != NULL) { is_dml = (PyOS_strnicmp(p, "insert", 6) == 0) || (PyOS_strnicmp(p, "update", 6) == 0) || (PyOS_strnicmp(p, "delete", 6) == 0) || (PyOS_strnicmp(p, "replace", 7) == 0); - break; } pysqlite_Statement *self = PyObject_GC_New(pysqlite_Statement, From 631f24dd076319234720da4f2584996a0f1a316b Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Thu, 9 Jun 2022 01:08:08 +0200 Subject: [PATCH 05/21] Adjust comment --- Modules/_sqlite/statement.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index f26665518c6c39..e15a99b0761156 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -131,12 +131,15 @@ stmt_traverse(pysqlite_Statement *self, visitproc visit, void *arg) } /* - * Checks if there is anything left in an SQL string after SQLite compiled it. + * Strips leading whitespace and SQL comments from input string and returns a + * pointer to the first non-whitespace, non-comment character. + * * This is used to check if somebody tried to execute more than one SQL command * with one execute()/executemany() command, which the DB-API and we don't * allow. * - * Returns 1 if there is more left than should be. 0 if ok. + * It is also used to strip leading whitespace and comments from input SQL + * queries, so we can easily detect DML queries. */ static const char * lstrip_sql(const char *sql) From 8f29283ac59e07d9a82bfb66219647e4b92213f9 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Thu, 9 Jun 2022 01:25:45 +0200 Subject: [PATCH 06/21] Explicit comparison --- Modules/_sqlite/statement.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index e15a99b0761156..226773d93082de 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -73,7 +73,7 @@ pysqlite_statement_create(pysqlite_Connection *connection, PyObject *sql) return NULL; } - if (lstrip_sql(tail)) { + if (lstrip_sql(tail) != NULL) { PyErr_SetString(connection->ProgrammingError, "You can only execute one statement at a time."); goto error; From a51cfe6e4ac747bdf5419db6019b50adf37d7d6c Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Thu, 9 Jun 2022 01:31:55 +0200 Subject: [PATCH 07/21] Add more tests --- Lib/test/test_sqlite3/test_dbapi.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Lib/test/test_sqlite3/test_dbapi.py b/Lib/test/test_sqlite3/test_dbapi.py index 612637202194de..781b4c80e1347f 100644 --- a/Lib/test/test_sqlite3/test_dbapi.py +++ b/Lib/test/test_sqlite3/test_dbapi.py @@ -914,6 +914,12 @@ def test_rowcount_prefixed_with_comment(self): insert into test(name) values ('foo') """) self.assertEqual(self.cu.rowcount, 1) + self.cu.execute(""" + /* -- messy /* /* *- *-- + */ + /* one more */ insert into test(name) values ('messy') + """) + self.assertEqual(self.cu.rowcount, 1) self.cu.execute("/* bar */ update test set name='bar' where name='foo'") self.assertEqual(self.cu.rowcount, 2) From bf0e150c483832b77e45d0794460aca3a1822274 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Thu, 9 Jun 2022 01:32:17 +0200 Subject: [PATCH 08/21] Fix bug in lstrip parser --- Modules/_sqlite/statement.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index 226773d93082de..10df7d9bb91292 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -161,6 +161,9 @@ lstrip_sql(const char *sql) break; case ' ': case '\t': + if (state == COMMENTEND_1) { + state = IN_COMMENT; + } break; case '\n': case 13: From 5918fbed3b50177058b128ee64662f7d6a965869 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Thu, 9 Jun 2022 01:38:14 +0200 Subject: [PATCH 09/21] Adjust NEWS --- .../Library/2022-06-06-12-58-27.gh-issue-79579.e8rB-M.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2022-06-06-12-58-27.gh-issue-79579.e8rB-M.rst b/Misc/NEWS.d/next/Library/2022-06-06-12-58-27.gh-issue-79579.e8rB-M.rst index 3c61b7e50365cb..c3bb3548553433 100644 --- a/Misc/NEWS.d/next/Library/2022-06-06-12-58-27.gh-issue-79579.e8rB-M.rst +++ b/Misc/NEWS.d/next/Library/2022-06-06-12-58-27.gh-issue-79579.e8rB-M.rst @@ -1,2 +1,2 @@ -:data:`sqlite3.Cursor.rowcount` is now correctly updated for all datamodifying -SQL queries. Patch by Erlend E. Aasland. +:mod:`sqlite3` now correctly detects DML queries, even when prefixed with +comments. Patch by Erlend E. Aasland. From b45e5b50fd71fce8e779550361662d91a03729cd Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Thu, 9 Jun 2022 09:48:02 +0200 Subject: [PATCH 10/21] Make test more messy --- Lib/test/test_sqlite3/test_dbapi.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Lib/test/test_sqlite3/test_dbapi.py b/Lib/test/test_sqlite3/test_dbapi.py index 781b4c80e1347f..37668c0cd8e218 100644 --- a/Lib/test/test_sqlite3/test_dbapi.py +++ b/Lib/test/test_sqlite3/test_dbapi.py @@ -908,14 +908,13 @@ def test_rowcount_update_returning(self): def test_rowcount_prefixed_with_comment(self): # gh-79579: rowcount is updated even if query is prefixed with comments - #self.cu.execute("/* foo */ insert into test(name) values (?)", ('foo',)) self.cu.execute(""" -- foo insert into test(name) values ('foo') """) self.assertEqual(self.cu.rowcount, 1) self.cu.execute(""" - /* -- messy /* /* *- *-- + /* -- messy /* /* ** *- *-- */ /* one more */ insert into test(name) values ('messy') """) From b0a7fbc7cabe1e6e5f4c98129305c219a54b7da2 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Thu, 9 Jun 2022 09:48:34 +0200 Subject: [PATCH 11/21] Address Ma Lin's review - normalise switch cases - improve NEWS entry accuracy --- .../Library/2022-06-06-12-58-27.gh-issue-79579.e8rB-M.rst | 4 ++-- Modules/_sqlite/statement.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2022-06-06-12-58-27.gh-issue-79579.e8rB-M.rst b/Misc/NEWS.d/next/Library/2022-06-06-12-58-27.gh-issue-79579.e8rB-M.rst index c3bb3548553433..82b1a1c28a6001 100644 --- a/Misc/NEWS.d/next/Library/2022-06-06-12-58-27.gh-issue-79579.e8rB-M.rst +++ b/Misc/NEWS.d/next/Library/2022-06-06-12-58-27.gh-issue-79579.e8rB-M.rst @@ -1,2 +1,2 @@ -:mod:`sqlite3` now correctly detects DML queries, even when prefixed with -comments. Patch by Erlend E. Aasland. +:mod:`sqlite3` now correctly detects DML queries with leading comments. +Patch by Erlend E. Aasland. diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index 10df7d9bb91292..a5ebc7280135ab 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -166,7 +166,7 @@ lstrip_sql(const char *sql) } break; case '\n': - case 13: + case '\r': if (state == IN_LINECOMMENT) { state = NORMAL; } From 61a81eb23ca6212edcd7c05fd858898f2938a87b Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Thu, 9 Jun 2022 09:56:42 +0200 Subject: [PATCH 12/21] Reword comment --- Modules/_sqlite/statement.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index a5ebc7280135ab..97b4fa04d0f728 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -131,15 +131,13 @@ stmt_traverse(pysqlite_Statement *self, visitproc visit, void *arg) } /* - * Strips leading whitespace and SQL comments from input string and returns a + * Strip leading whitespace and comments from SQL string and return a * pointer to the first non-whitespace, non-comment character. * - * This is used to check if somebody tried to execute more than one SQL command - * with one execute()/executemany() command, which the DB-API and we don't - * allow. + * This is used to check if somebody tries to execute more than one SQL query + * with one execute()/executemany() command, which the DB-API don't allow. * - * It is also used to strip leading whitespace and comments from input SQL - * queries, so we can easily detect DML queries. + * It is also used to harden DML query detection. */ static const char * lstrip_sql(const char *sql) From e3fea31348d2cdab28710a0ebbb7e246d7b1212f Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Thu, 9 Jun 2022 10:47:40 +0200 Subject: [PATCH 13/21] Inline it --- Modules/_sqlite/statement.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index 97b4fa04d0f728..77b630c62a410f 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -139,7 +139,7 @@ stmt_traverse(pysqlite_Statement *self, visitproc visit, void *arg) * * It is also used to harden DML query detection. */ -static const char * +static inline const char * lstrip_sql(const char *sql) { const char *pos = sql; From d6ce75c04b73023dbd047de66d871ca9f8da5342 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Thu, 9 Jun 2022 12:55:31 +0200 Subject: [PATCH 14/21] Further mess up messy test to increase code coverage --- Lib/test/test_sqlite3/test_dbapi.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_sqlite3/test_dbapi.py b/Lib/test/test_sqlite3/test_dbapi.py index 37668c0cd8e218..15230898368918 100644 --- a/Lib/test/test_sqlite3/test_dbapi.py +++ b/Lib/test/test_sqlite3/test_dbapi.py @@ -914,7 +914,7 @@ def test_rowcount_prefixed_with_comment(self): """) self.assertEqual(self.cu.rowcount, 1) self.cu.execute(""" - /* -- messy /* /* ** *- *-- + /* -- messy *r /* /* ** *- *-- */ /* one more */ insert into test(name) values ('messy') """) From 7f7943fdbca8f0b5af12d0e5b52b0803128499df Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Fri, 10 Jun 2022 15:32:00 +0200 Subject: [PATCH 15/21] Increase code coverage --- Lib/test/test_sqlite3/test_dbapi.py | 31 +++++++++++++++++++---------- Modules/_sqlite/statement.c | 8 +------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/Lib/test/test_sqlite3/test_dbapi.py b/Lib/test/test_sqlite3/test_dbapi.py index 15230898368918..366fe879c251a0 100644 --- a/Lib/test/test_sqlite3/test_dbapi.py +++ b/Lib/test/test_sqlite3/test_dbapi.py @@ -746,22 +746,33 @@ def test_execute_illegal_sql(self): with self.assertRaises(sqlite.OperationalError): self.cu.execute("select asdf") - def test_execute_too_much_sql(self): - self.assertRaisesRegex(sqlite.ProgrammingError, - "You can only execute one statement at a time", - self.cu.execute, "select 5+4; select 4+5") - - def test_execute_too_much_sql2(self): - self.cu.execute("select 5+4; -- foo bar") + def test_execute_multiple_statements(self): + msg = "You can only execute one statement at a time" + dataset = ( + "select 5+4; select 4+5", + "select 1; //c++ comment", + "select 1; *notsql", + "select 1; -*not a comment", + ) + for query in dataset: + with self.subTest(query=query): + with self.assertRaisesRegex(sqlite.ProgrammingError, msg): + self.cu.execute(query) - def test_execute_too_much_sql3(self): - self.cu.execute(""" + def test_execute_with_appended_comments(self): + dataset = ( + "select 5+4; -- foo bar", + """ select 5+4; /* foo */ - """) + """, + ) + for query in dataset: + with self.subTest(query=query): + self.cu.execute(query) def test_execute_wrong_sql_arg(self): with self.assertRaises(TypeError): diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index 77b630c62a410f..c510cafb4710b9 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -142,14 +142,10 @@ stmt_traverse(pysqlite_Statement *self, visitproc visit, void *arg) static inline const char * lstrip_sql(const char *sql) { - const char *pos = sql; - parse_remaining_sql_state state = NORMAL; - for (;;) { + for (const char *pos = sql; *pos; pos++) { switch (*pos) { - case 0: - return NULL; case '-': if (state == NORMAL) { state = LINECOMMENT_1; @@ -198,8 +194,6 @@ lstrip_sql(const char *sql) return pos; } } - - pos++; } return NULL; From e5fafc72b2cd1b8888f55b5ff59b94d4232dc4fa Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Mon, 13 Jun 2022 11:05:27 +0200 Subject: [PATCH 16/21] Rewrite lstrip_sql by borrowing from SQLite's parser --- Lib/test/test_sqlite3/test_dbapi.py | 19 ++++++- Modules/_sqlite/statement.c | 88 ++++++++++++----------------- 2 files changed, 53 insertions(+), 54 deletions(-) diff --git a/Lib/test/test_sqlite3/test_dbapi.py b/Lib/test/test_sqlite3/test_dbapi.py index 366fe879c251a0..4fc9822e945e09 100644 --- a/Lib/test/test_sqlite3/test_dbapi.py +++ b/Lib/test/test_sqlite3/test_dbapi.py @@ -749,10 +749,21 @@ def test_execute_illegal_sql(self): def test_execute_multiple_statements(self): msg = "You can only execute one statement at a time" dataset = ( - "select 5+4; select 4+5", - "select 1; //c++ comment", - "select 1; *notsql", + "select 1; select 2", + "select 1; // c++ comments are not allowed", + "select 1; *not a comment", "select 1; -*not a comment", + "select 1; /* */ a", + "select 1; /**/a", + "select 1; -", + "select 1; /", + """select 1; + -- comment + select 2 + """, + """select 1; - + - select 2 + """, ) for query in dataset: with self.subTest(query=query): @@ -762,6 +773,8 @@ def test_execute_multiple_statements(self): def test_execute_with_appended_comments(self): dataset = ( "select 5+4; -- foo bar", + "select 5+4; --", + "select 5+4; /*", # Unclosed comments ending in \0 are skipped. """ select 5+4; diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index c510cafb4710b9..b6b5e2f95bc82c 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -28,15 +28,6 @@ /* prototypes */ static const char *lstrip_sql(const char *sql); -typedef enum { - LINECOMMENT_1, - IN_LINECOMMENT, - COMMENTSTART_1, - IN_COMMENT, - COMMENTEND_1, - NORMAL -} parse_remaining_sql_state; - pysqlite_Statement * pysqlite_statement_create(pysqlite_Connection *connection, PyObject *sql) { @@ -131,8 +122,9 @@ stmt_traverse(pysqlite_Statement *self, visitproc visit, void *arg) } /* - * Strip leading whitespace and comments from SQL string and return a - * pointer to the first non-whitespace, non-comment character. + * Strip leading whitespace and comments from incoming SQL (null terminated C + * string) and return a pointer to the first non-whitespace, non-comment + * character. * * This is used to check if somebody tries to execute more than one SQL query * with one execute()/executemany() command, which the DB-API don't allow. @@ -142,57 +134,51 @@ stmt_traverse(pysqlite_Statement *self, visitproc visit, void *arg) static inline const char * lstrip_sql(const char *sql) { - parse_remaining_sql_state state = NORMAL; - + // This loop is borrowed from the SQLite source code. for (const char *pos = sql; *pos; pos++) { switch (*pos) { - case '-': - if (state == NORMAL) { - state = LINECOMMENT_1; - } else if (state == LINECOMMENT_1) { - state = IN_LINECOMMENT; - } - break; case ' ': case '\t': - if (state == COMMENTEND_1) { - state = IN_COMMENT; - } - break; + case '\f': case '\n': case '\r': - if (state == IN_LINECOMMENT) { - state = NORMAL; - } + // Skip whitespace. break; + case '-': + if (pos[1] && pos[1] != '-') { + return &pos[1]; + } + // Skip line comments. + if (pos[1] == '-') { + pos += 2; + while (pos[0] && pos[0] != '\n') { + pos++; + } + if (pos[0] == '\0') { + return NULL; + } + continue; + } + return pos; case '/': - if (state == NORMAL) { - state = COMMENTSTART_1; - } else if (state == COMMENTEND_1) { - state = NORMAL; - } else if (state == COMMENTSTART_1) { - return pos; + if (pos[1] && pos[1] != '*') { + return &pos[1]; } - break; - case '*': - if (state == NORMAL) { - return pos; - } else if (state == LINECOMMENT_1) { - return pos; - } else if (state == COMMENTSTART_1) { - state = IN_COMMENT; - } else if (state == IN_COMMENT) { - state = COMMENTEND_1; + // Skip C style comments. + if (pos[1] == '*') { + pos += 2; + while (pos[0] && (pos[0] != '*' || pos[1] != '/')) { + pos++; + } + if (pos[0] == '\0') { + return NULL; + } + pos++; + continue; } - break; + return pos; default: - if (state == COMMENTEND_1) { - state = IN_COMMENT; - } else if (state == IN_LINECOMMENT) { - } else if (state == IN_COMMENT) { - } else { - return pos; - } + return pos; } } From 7d7a654334995436c76f624fa123892615e91ca8 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Mon, 13 Jun 2022 11:52:00 +0200 Subject: [PATCH 17/21] Test code nit --- Lib/test/test_sqlite3/test_dbapi.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_sqlite3/test_dbapi.py b/Lib/test/test_sqlite3/test_dbapi.py index 4fc9822e945e09..0ad3f3e601a47b 100644 --- a/Lib/test/test_sqlite3/test_dbapi.py +++ b/Lib/test/test_sqlite3/test_dbapi.py @@ -772,9 +772,9 @@ def test_execute_multiple_statements(self): def test_execute_with_appended_comments(self): dataset = ( - "select 5+4; -- foo bar", - "select 5+4; --", - "select 5+4; /*", # Unclosed comments ending in \0 are skipped. + "select 1; -- foo bar", + "select 1; --", + "select 1; /*", # Unclosed comments ending in \0 are skipped. """ select 5+4; From 2e638a0422200abcee1b49b147f98676ca2ce462 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Mon, 13 Jun 2022 12:05:21 +0200 Subject: [PATCH 18/21] Harden test_rowcount_prefixed_with_comment test Make sure a different rowcount is returned for each query --- Lib/test/test_sqlite3/test_dbapi.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_sqlite3/test_dbapi.py b/Lib/test/test_sqlite3/test_dbapi.py index 0ad3f3e601a47b..84c58b4f07a219 100644 --- a/Lib/test/test_sqlite3/test_dbapi.py +++ b/Lib/test/test_sqlite3/test_dbapi.py @@ -934,9 +934,9 @@ def test_rowcount_prefixed_with_comment(self): # gh-79579: rowcount is updated even if query is prefixed with comments self.cu.execute(""" -- foo - insert into test(name) values ('foo') + insert into test(name) values ('foo'), ('foo') """) - self.assertEqual(self.cu.rowcount, 1) + self.assertEqual(self.cu.rowcount, 2) self.cu.execute(""" /* -- messy *r /* /* ** *- *-- */ @@ -944,7 +944,7 @@ def test_rowcount_prefixed_with_comment(self): """) self.assertEqual(self.cu.rowcount, 1) self.cu.execute("/* bar */ update test set name='bar' where name='foo'") - self.assertEqual(self.cu.rowcount, 2) + self.assertEqual(self.cu.rowcount, 3) def test_rowcount_vaccuum(self): data = ((1,), (2,), (3,)) From b932e4f3558d967da60d6f88d5b094d58b148e23 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Mon, 13 Jun 2022 12:12:31 +0200 Subject: [PATCH 19/21] Simplify one test --- Lib/test/test_sqlite3/test_dbapi.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Lib/test/test_sqlite3/test_dbapi.py b/Lib/test/test_sqlite3/test_dbapi.py index 84c58b4f07a219..18e84e9e58632b 100644 --- a/Lib/test/test_sqlite3/test_dbapi.py +++ b/Lib/test/test_sqlite3/test_dbapi.py @@ -757,13 +757,11 @@ def test_execute_multiple_statements(self): "select 1; /**/a", "select 1; -", "select 1; /", + "select 1; -\n- select 2", """select 1; -- comment select 2 """, - """select 1; - - - select 2 - """, ) for query in dataset: with self.subTest(query=query): From ca053d7f7848548729985d0428d3a3f30fd4a29e Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Mon, 13 Jun 2022 20:23:50 +0200 Subject: [PATCH 20/21] Fix ret value for invalid comments --- Modules/_sqlite/statement.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index b6b5e2f95bc82c..4390050be732c7 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -146,7 +146,7 @@ lstrip_sql(const char *sql) break; case '-': if (pos[1] && pos[1] != '-') { - return &pos[1]; + return pos; } // Skip line comments. if (pos[1] == '-') { @@ -162,7 +162,7 @@ lstrip_sql(const char *sql) return pos; case '/': if (pos[1] && pos[1] != '*') { - return &pos[1]; + return pos; } // Skip C style comments. if (pos[1] == '*') { From c6fa9b298372603f174785f014e291b62b04fb8d Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" <erlend.aasland@protonmail.com> Date: Mon, 13 Jun 2022 20:29:57 +0200 Subject: [PATCH 21/21] Simplify --- Modules/_sqlite/statement.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index 4390050be732c7..aee460747b45f4 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -145,9 +145,6 @@ lstrip_sql(const char *sql) // Skip whitespace. break; case '-': - if (pos[1] && pos[1] != '-') { - return pos; - } // Skip line comments. if (pos[1] == '-') { pos += 2; @@ -161,9 +158,6 @@ lstrip_sql(const char *sql) } return pos; case '/': - if (pos[1] && pos[1] != '*') { - return pos; - } // Skip C style comments. if (pos[1] == '*') { pos += 2;