python
gh-143198: fix some crashes in `sqlite3.executemany` with concurrent mutations
FTR, the reason why execute is actually not affected is that the iterator we are taking is built from a list we create ourselves so it's not possible to have "bad" side effects.
- Issue: gh-143198
@serhiy-storchaka So the split went well but... it's now a larger code =/ I also changed some variable names since we're talking about parameters everywhere but it's more arguments in this case or values. I can revert the change if you want (I'll make a commit for reverting and then we can decide whether to keep the revert or not) but I wanted to show you how it would be if the implementations are separate.
I feel that there are a lot of tests that are really looking-alike. It's kinda annoying to have redundancy... and it won't get any better with the other PRs for sqlite where we I need to test the callbacks.
I also changed some variable names since we're talking about parameters everywhere but it's more arguments in this case or values.
I understand you, the term "parameter" is used here in different meaning from what we used to. But this makes the diff unnecessary large and makes harder to follow semantic changes. Can we leave this to a separate PR?
Ok, I'll revert the naming changes and all other styling stuff. And then I'll address it separately.
Ok, let's avoid force-pushing more. I force pushed because I wanted a single commit for the follow-up PR
If possible, I would prefer to add checks closer to the code that can be affected by closing the connection then to the code that can close the connection.
This is not always possible and it's more prone to errors in the future. The connection may be closed but we would check it way later. I tried to only add checks when the code in question never uses the sqlite3 API (e.g., pysqlite_microprotocols_adapt is purely using the Python C API so it doesn't matter that the connection is alive or not).
I can't make the diff smaller though. It's the minimal diff on the C code I can do.
@serhiy-storchaka I've extensively added test cases for the adpater's part. I didn't check all __conform__ or __adapt__ methods because these calls are wrapped in pysqlite_microprotocols_adapt so I don't really need to care about it (as soon as I check after the call whether it's fine or not should be sufficient).
The minimal fix:
diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c
index 4611c9e5e3e..9a7e3d7164e 100644
--- a/Modules/_sqlite/cursor.c
+++ b/Modules/_sqlite/cursor.c
@@ -881,6 +881,9 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation
}
}
+ if (!pysqlite_check_connection(self->connection)) {
+ goto error;
+ }
PyObject *stmt = get_statement_from_cache(self, operation);
Py_XSETREF(self->statement, (pysqlite_Statement *)stmt);
if (!self->statement) {
@@ -930,6 +933,9 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation
if (PyErr_Occurred()) {
goto error;
}
+ if (!pysqlite_check_connection(self->connection)) {
+ goto error;
+ }
rc = stmt_step(self->statement->st);
if (rc != SQLITE_DONE && rc != SQLITE_ROW) {
I think I'll need to check with more than 1 parameter as well. I cannot say for sure that checks are all redundant because my table has a single parameter to bind. But if binding doesn't require the connection at all, it could be possible to be safe and reduce the amount of checks.