gh-143198: fix some crashes in sqlite3.executemany with concurrent mutations
#143210
Conversation
…nt parameter iterator
picnixz
requested review from
berkerpeksag and
erlend-aasland
as code owners
Misc/NEWS.d/next/Library/2025-12-27-10-36-18.gh-issue-143198.DdIHyC.rst
Outdated
Show resolved
Hide resolved
picnixz
added
needs backport to 3.13
picnixz
changed the title
sqlite3.execute[many] with re-entrant parameter iteratorsqlite3.execute[many] with concurrent mutations
picnixz
changed the title
sqlite3.execute[many] with concurrent mutationssqlite3.executemany with concurrent mutations
|
There were more UAFs that I could find... |
picnixz
changed the title
sqlite3.executemany with concurrent mutationssqlite3.executemany with concurrent mutations
Modules/_sqlite/cursor.c
Outdated
|
|
||
| // PyObject_GetIter() may have a side-effect on the connection's state. | ||
| // See: https://github.com/python/cpython/issues/143198. | ||
| if (!pysqlite_check_connection(self->connection)) { |
There was a problem hiding this comment.
Instead of checking after functions which can allow closing the connection, why not test immediately before the code that implies the connection is alive and would crash otherwise? For example, setting self->description looks safe, but reading self->connection->state is not.
There was a problem hiding this comment.
That's what I had in mind but that forces me to pass the connection around. I'm fine with this approach though. I expect the diff to also be larger. And I think it's also time to split the implementation of execute and executemany because execute doesn't need a loop, and does not always need some checks. OTOH, executemany() don't need part of the execute() logic.
There was a problem hiding this comment.
Yes, I planned to split them long time ago, but it had very low priority.
There is a risk that we will introduce other point that executes an arbitrary Python code or releases the GIL between the last check and the use which can crash. So we should keep them as close one to other as possible.
There was a problem hiding this comment.
I am currently splitting them actually. It's just about extracting part of the functions into separate functions so there should not be any surprises.
Modules/_sqlite/cursor.c
Outdated
|
|
||
| // PyObject_GetIter() may have a side-effect on the connection's state. | ||
| // See: https://github.com/python/cpython/issues/143198. | ||
| if (!pysqlite_check_connection(self->connection)) { |
There was a problem hiding this comment.
Yes, I planned to split them long time ago, but it had very low priority.
There is a risk that we will introduce other point that executes an arbitrary Python code or releases the GIL between the last check and the use which can crash. So we should keep them as close one to other as possible.
Lib/test/test_sqlite3/test_dbapi.py
Outdated
| return iter([(1,), (2,), (3,)]) | ||
|
|
||
|
|
||
| class ParamsCxCloseInNext(CxWrapper): |
There was a problem hiding this comment.
Cannot we simply use local generator functions?
def ParamsCxCloseInNext():
for i in range(5):
cx.close()
yield (i,)There was a problem hiding this comment.
Mmh, it can be easier yes, let me check if I can do it that way.
|
@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. |
3 participants
FTR, the reason why
executeis 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._sqlitecursor cache via re-entrant parameter iterator #143198