gh-139327: consolidate sqlite3_finalize and sqlite3_reset usages
#139329
Conversation
picnixz
requested review from
berkerpeksag and
erlend-aasland
as code owners
picnixz
force-pushed
the
feat/sqlite/consolidate-api-usage-139327
branch
from
2c0c2fe to
5843251
Compare
picnixz
force-pushed
the
feat/sqlite/consolidate-api-usage-139327
branch
from
5843251 to
b50713c
Compare
| * Checks the SQLite error code and sets the appropriate DB-API exception. | ||
| */ | ||
| void | ||
| int |
There was a problem hiding this comment.
In blob.c, should blob_seterror assert that set_error_from_db didn't return SQLITE_OK?
Same for when connection.c and the files changed here call blob_seterror without checking the result.
There was a problem hiding this comment.
Oh I missed the blob stuff. I think we should also assert this indeed.
There was a problem hiding this comment.
Also, I think there was an API misuse here. We pass rc but we never use it. I think we were meant to use set_error_from_code instead.
Modules/_sqlite/cursor.c
Outdated
| _PyErr_ChainExceptions1(exc); | ||
| } | ||
| else { | ||
| // assert(!PyErr_Occurred()); |
There was a problem hiding this comment.
Please add a comment about why this isn't asserted.
There was a problem hiding this comment.
I think it's because I had an abort here since PyErr_SetString can be called with an exception already set. I wanted to clean up the usages but I think it was too many changes that are not really related (see #139447 for the ongoing work). I'll retry now to see if this is indeed the case (it might also be a relicate of me testing the calls)
|
Er... the docs do not mention exactly the way to go wrong but I can try |
Modules/_sqlite/cursor.c
Outdated
| (void)stmt_reset(self->statement); | ||
| Py_CLEAR(self->statement); | ||
| if (self->statement && stmt_reset(self->statement) != SQLITE_OK) { | ||
| cursor_cannot_reset_stmt_error(self, 0); |
There was a problem hiding this comment.
Here I'm a bit unsure whether we should call Py_CLEAR(self->statement) after a reset failure. If we cannot reset the statement, that means we were not able to free the memory (or maybe it's something else). However, the exact return codes being returned are not documented on sqlite3's side so I can't have a finer check.
In other places, Py_CLEAR is always called even in the case of a reset failure, however here we're in the "close" method and thus I don't know if I should call Py_CLEAR.
There was a problem hiding this comment.
Ok, actually I could have a double-free:
stmt_clearfails but internally manages to free the memory.- self->statement isnot cleared so still not NULL
- someone recalls close() -> we retry to clear the memory -> double free.
picnixz
force-pushed
the
feat/sqlite/consolidate-api-usage-139327
branch
from
446f580 to
44f72e9
Compare
sqlite3module #139327