[pysqlite] Duplicate rows can be returned after rolling back a transaction

[cyang1]

BPO	33376
Nosy	@benjaminp, @berkerpeksag, @serhiy-storchaka, @animalize, @palaviv, @cyang1, @tirkarthi, @erlend-aasland
PRs	bpo-33376: clear cursor->statement when setting cursor->reset #10250

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2022-01-03.20:03:08.273>
created_at = <Date 2018-04-28.00:15:11.415>
labels = ['extension-modules', 'type-bug', '3.8', '3.9']
title = '[pysqlite] Duplicate rows can be returned after rolling back a transaction'
updated_at = <Date 2022-01-03.20:03:46.339>
user = 'https://github.com/cyang1'

bugs.python.org fields:

activity = <Date 2022-01-03.20:03:46.339>
actor = 'erlendaasland'
assignee = 'none'
closed = True
closed_date = <Date 2022-01-03.20:03:08.273>
closer = 'erlendaasland'
components = ['Extension Modules']
creation = <Date 2018-04-28.00:15:11.415>
creator = 'cary'
dependencies = []
files = []
hgrepos = []
issue_num = 33376
keywords = ['patch']
message_count = 9.0
messages = ['315862', '337584', '337585', '393402', '393435', '407185', '407187', '407236', '409614']
nosy_count = 9.0
nosy_names = ['benjamin.peterson', 'berker.peksag', 'serhiy.storchaka', 'malin', 'Maxime Belanger', 'palaviv', 'cary', 'xtreak', 'erlendaasland']
pr_nums = ['10250']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue33376'
versions = ['Python 3.8', 'Python 3.9']

[cyang1]

Description

Rolling back a transaction causes all statements associated with that transaction to be reset, which allows the statements to be used again from the pysqlite statement cache. This can interact with various methods on Cursor objects to cause these statements to be reset again, possibly when they are in use by a different cursor.

This appears to be very similar to bpo-10513 and bpo-23129.

# Impact
Duplicate rows will be returned. Exceptions can be raised.

Repro steps

• Cursor A executes query X
• Rollback occurs
• Cursor B executes query X
• Any of the following (and there might be other cases too):
  • Cursor A is closed
  • Cursor A is deallocated
  • Cursor A executes any other query.
• Result: Cursor B returns duplicate rows.
• Furthermore: Executing query X again afterwards raises sqlite3.InterfaceError

Possible solutions

• Similar to the solution for bpo-10513 and bpo-23129, we could remove pysqlite_do_all_statements(self, ACTION_RESET, 1) from pysqlite_connection_rollback. This fixes the given issue, but I'm not sure what the implications are for the rest of the system.

• Do not reset self->statement in Cursor if self->reset. This is the fix we've adopted for now (through a local patch to our Python), but it's worth noting that this is rather brittle, and only works because pysqlite_do_all_statements is always called with reset_cursors = 1, and self->reset is not modified in too many places.

# Example

import sqlite3 as sqlite

if __name__ == '__main__':
    conn = sqlite.connect(":memory:")
    conn.executescript("""
        CREATE TABLE t(c);
        INSERT INTO t VALUES(0);
        INSERT INTO t VALUES(1);
        INSERT INTO t VALUES(2);
    """)

    curs = conn.cursor()
    curs.execute("BEGIN TRANSACTION")
    curs.execute("SELECT c FROM t WHERE ?", (1,))
    conn.rollback()

    # Reusing the same statement from the statement cache, which has been
    # reset by the rollback above.
    gen = conn.execute("SELECT c FROM t WHERE ?", (1,))

    # Any of the following will cause a spurious reset of the statement.
    curs.close()
    # curs.execute("SELECT 1")
    # del curs

    # Expected output: [(0,), (1,), (2,)]
    # Observed output: [(0,), (0,), (1,), (2,)]
    print(list(gen))

    # Raises `sqlite3.InterfaceError: Error binding parameter 0 - probably unsupported type.`
    conn.execute("SELECT c FROM t WHERE ?", (1,))

[benjaminp]

New changeset 738c19f by Benjamin Peterson in branch 'master':
closes bpo-33376: Update to Unicode 12.0.0. (GH-12256)
738c19f

[benjaminp]

Sorry, wrong bug.

[animalize]

Erlend, please take a look at this bug.

[erlend-aasland]

I believe the former proposed solution is the correct solution. I'm digging though the pysqlite git history (both in the original repo and in CPython), the SQLite changelogs, and different test suites to prove me wrong :) I'm using bpo-44092 to track my findings.

[animalize]

Since 243b6c3 (21-08-19), this bug can't be reproduced.

In pysqlite_do_all_statements(), 243b6c3 resets statements like this:

    sqlite3_stmt *stmt = NULL;
    while ((stmt = sqlite3_next_stmt(self->db, stmt))) {
        if (sqlite3_stmt_busy(stmt)) {
            (void)sqlite3_reset(stmt);
        }
    }
    
But the `pysqlite_Statement.in_use` flag is not reset.
In `_pysqlite_query_execute()` function, if `pysqlite_Statement.in_use` flag is 1, it creates a new `pysqlite_Statement` instance. So this line will use a new statement:

    gen = conn.execute("SELECT c FROM t WHERE ?", (1,))

The duplicate row is from pysqlite_Cursor.next_row before 3df0fc8 (21-08-25).

A digressive suggestion is whether it can be changed like this, and add a check for resetting statement. So that statements are not allowed to be reset by other Cursors, which may improve code robust:

    typedef struct
    {
        ...
-       int in_use;
+       pysqlite_Cursor *in_use; // points to the attached cursor
        ...
    } pysqlite_Statement;

[animalize]

This issue is not resolved, but was covered by a problematic behavior.
Maybe this issue will be solved in bpo-44092, I'll study that issue later.