Skip to content

gh-139327: consolidate sqlite3_finalize and sqlite3_reset usages #139329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 15, 2025
Merged

gh-139327: consolidate sqlite3_finalize and sqlite3_reset usages #139329

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b50713c
consolidate `sqlite3_finalize` and `sqlite3_reset` usages
picnixz Sep 26, 2025
a1b9eaa
reset_ok -> reset_rc
picnixz Oct 5, 2025
5653512
check that `blob_error` set an exception
picnixz Oct 5, 2025
6a3a708
assert that `PyErr_SetString` is not called with an active exception
picnixz Oct 5, 2025
44f72e9
avoid a possible double-free at the cost of a possible leak
picnixz Oct 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 88 additions & 11 deletions Modules/_sqlite/cursor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,41 @@ check_cursor_locked(pysqlite_Cursor *cur)
return 1;
}

static pysqlite_state *
get_module_state_by_cursor(pysqlite_Cursor *cursor)
{
if (cursor->connection != NULL && cursor->connection->state != NULL) {
return cursor->connection->state;
}
return pysqlite_get_state_by_type(Py_TYPE(cursor));
}

static void
cursor_sqlite3_internal_error(pysqlite_Cursor *cursor,
const char *error_message,
int chain_exceptions)
{
pysqlite_state *state = get_module_state_by_cursor(cursor);
if (chain_exceptions) {
assert(PyErr_Occurred());
PyObject *exc = PyErr_GetRaisedException();
PyErr_SetString(state->InternalError, error_message);
_PyErr_ChainExceptions1(exc);
}
else {
// assert(!PyErr_Occurred());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment about why this isn't asserted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

PyErr_SetString(state->InternalError, error_message);
}
}

static void
cursor_cannot_reset_stmt_error(pysqlite_Cursor *cursor, int chain_exceptions)
{
cursor_sqlite3_internal_error(cursor,
"cannot reset statement",
chain_exceptions);
}

/*[clinic input]
module _sqlite3
class _sqlite3.Cursor "pysqlite_Cursor *" "clinic_state()->CursorType"
Expand Down Expand Up @@ -173,8 +208,12 @@ cursor_clear(PyObject *op)
Py_CLEAR(self->row_factory);
if (self->statement) {
/* Reset the statement if the user has not closed the cursor */
stmt_reset(self->statement);
int rc = stmt_reset(self->statement);
Py_CLEAR(self->statement);
if (rc != SQLITE_OK) {
cursor_cannot_reset_stmt_error(self, 0);
PyErr_FormatUnraisable("Exception ignored in cursor_clear()");
}
}

return 0;
Expand Down Expand Up @@ -834,7 +873,9 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation

if (self->statement) {
// Reset pending statements on this cursor.
(void)stmt_reset(self->statement);
if (stmt_reset(self->statement) != SQLITE_OK) {
goto reset_failure;
}
}

PyObject *stmt = get_statement_from_cache(self, operation);
Expand All @@ -858,7 +899,9 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation
}
}

(void)stmt_reset(self->statement);
if (stmt_reset(self->statement) != SQLITE_OK) {
goto reset_failure;
}
self->rowcount = self->statement->is_dml ? 0L : -1L;

/* We start a transaction implicitly before a DML statement.
Expand Down Expand Up @@ -940,7 +983,9 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation
if (self->statement->is_dml) {
self->rowcount += (long)sqlite3_changes(self->connection->db);
}
stmt_reset(self->statement);
if (stmt_reset(self->statement) != SQLITE_OK) {
goto reset_failure;
}
}
Py_XDECREF(parameters);
}
Expand All @@ -965,8 +1010,15 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation

if (PyErr_Occurred()) {
if (self->statement) {
(void)stmt_reset(self->statement);
sqlite3 *db = sqlite3_db_handle(self->statement->st);
int sqlite3_state = sqlite3_errcode(db);
// stmt_reset() may return a previously set exception,
// either triggered because of Python or sqlite3.
rc = stmt_reset(self->statement);
Py_CLEAR(self->statement);
if (sqlite3_state == SQLITE_OK && rc != SQLITE_OK) {
cursor_cannot_reset_stmt_error(self, 1);
}
}
self->rowcount = -1L;
return NULL;
Expand All @@ -975,6 +1027,20 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation
Py_CLEAR(self->statement);
}
return Py_NewRef((PyObject *)self);

reset_failure:
/* suite to execute when stmt_reset() failed and no exception is set */
assert(!PyErr_Occurred());

Py_XDECREF(parameters);
Py_XDECREF(parameters_iter);
Py_XDECREF(parameters_list);

self->locked = 0;
self->rowcount = -1L;
Py_CLEAR(self->statement);
cursor_cannot_reset_stmt_error(self, 0);
return NULL;
}

/*[clinic input]
Expand Down Expand Up @@ -1117,14 +1183,20 @@ pysqlite_cursor_iternext(PyObject *op)
if (self->statement->is_dml) {
self->rowcount = (long)sqlite3_changes(self->connection->db);
}
(void)stmt_reset(self->statement);
int rc = stmt_reset(self->statement);
Py_CLEAR(self->statement);
if (rc != SQLITE_OK) {
goto reset_failure;
}
}
else if (rc != SQLITE_ROW) {
set_error_from_db(self->connection->state, self->connection->db);
(void)stmt_reset(self->statement);
rc = set_error_from_db(self->connection->state, self->connection->db);
int reset_ok = stmt_reset(self->statement);
Py_CLEAR(self->statement);
Py_DECREF(row);
if (rc == SQLITE_OK && reset_ok != SQLITE_OK) {
goto reset_failure;
}
return NULL;
}
if (!Py_IsNone(self->row_factory)) {
Expand All @@ -1134,6 +1206,10 @@ pysqlite_cursor_iternext(PyObject *op)
Py_SETREF(row, new_row);
}
return row;

reset_failure:
cursor_cannot_reset_stmt_error(self, 0);
return NULL;
}

/*[clinic input]
Expand Down Expand Up @@ -1291,11 +1367,12 @@ pysqlite_cursor_close_impl(pysqlite_Cursor *self)
return NULL;
}

if (self->statement) {
(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);
Copy link
Member Author

@picnixz picnixz Oct 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, actually I could have a double-free:

  • stmt_clear fails 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.

return NULL;
}

Py_CLEAR(self->statement);
self->closed = 1;

Py_RETURN_NONE;
Expand Down
17 changes: 14 additions & 3 deletions Modules/_sqlite/statement.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,12 @@ pysqlite_statement_create(pysqlite_Connection *connection, PyObject *sql)
return self;

error:
(void)sqlite3_finalize(stmt);
assert(PyErr_Occurred());
if (sqlite3_finalize(stmt) != SQLITE_OK) {
PyObject *exc = PyErr_GetRaisedException();
PyErr_SetString(connection->InternalError, "cannot finalize statement");
_PyErr_ChainExceptions1(exc);
}
return NULL;
}

Expand All @@ -107,10 +112,16 @@ stmt_dealloc(PyObject *op)
PyTypeObject *tp = Py_TYPE(self);
PyObject_GC_UnTrack(op);
if (self->st) {
int rc;
Py_BEGIN_ALLOW_THREADS
sqlite3_finalize(self->st);
rc = sqlite3_finalize(self->st);
Py_END_ALLOW_THREADS
self->st = 0;
self->st = NULL;
if (rc != SQLITE_OK) {
pysqlite_state *state = PyType_GetModuleState(Py_TYPE(op));
PyErr_SetString(state->InternalError, "cannot finalize statement");
PyErr_FormatUnraisable("Exception ignored in stmt_dealloc()");
}
}
tp->tp_free(self);
Py_DECREF(tp);
Expand Down
5 changes: 3 additions & 2 deletions Modules/_sqlite/util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,21 +135,22 @@ set_error_from_code(pysqlite_state *state, int code)
/**
* Checks the SQLite error code and sets the appropriate DB-API exception.
*/
void
int
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I missed the blob stuff. I think we should also assert this indeed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

set_error_from_db(pysqlite_state *state, sqlite3 *db)
{
int errorcode = sqlite3_errcode(db);
PyObject *exc_class = get_exception_class(state, errorcode);
if (exc_class == NULL) {
// No new exception need be raised.
return;
return SQLITE_OK;
}

/* Create and set the exception. */
int extended_errcode = sqlite3_extended_errcode(db);
// sqlite3_errmsg() always returns an UTF-8 encoded message
const char *errmsg = sqlite3_errmsg(db);
raise_exception(exc_class, extended_errcode, errmsg);
return errorcode;
}

#ifdef WORDS_BIGENDIAN
Expand Down
2 changes: 1 addition & 1 deletion Modules/_sqlite/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
/**
* Checks the SQLite error code and sets the appropriate DB-API exception.
*/
void set_error_from_db(pysqlite_state *state, sqlite3 *db);
int set_error_from_db(pysqlite_state *state, sqlite3 *db);
void set_error_from_code(pysqlite_state *state, int code);

sqlite_int64 _pysqlite_long_as_int64(PyObject * value);
Expand Down
Loading