gh-129346: Handle allocation errors for SQLite aggregate context

[erlend-aasland]

• Issue: No NULL check in sqlite connection.c #129346

[erlend-aasland]

@serhiy-storchaka I would not bother with backporting this. It is not a bug.

[erlend-aasland]

I'll let this sit until tomorrow before landing (unless Serhiy gives a thumbs up before that time).

[serhiy-storchaka]

I think that there is a bug. The first call of sqlite3_aggregate_context can return NULL, but there are no checks for NULL anywhere.

https://www.sqlite.org/c3ref/aggregate_context.html

The sqlite3_aggregate_context(C,N) routine returns a NULL pointer when first called if N is less than or equal to zero or if a memory allocation error occurs.

If we can determine what the call was first, we can raise MemoryError for the first call and RuntimeError for the subsequent calls. Otherwise we should always raise MemoryError, it is simpler and safer.

[erlend-aasland]

I'll go through this again; will ping you later Serhiy.

[erlend-aasland]

If we can determine what the call was first, we can raise MemoryError for the first call and RuntimeError for the subsequent calls. Otherwise we should always raise MemoryError, it is simpler and safer.

The step callback is almost always the first, unless in some special circumstances where we'll go straight to the final callback (we've got a least one test case for this for aggregate functions in the test suite).

I think your suggestion makes sense, Serhiy.

[erlend-aasland]

[...] we can raise MemoryError for the first call and RuntimeError for the subsequent calls.

IIRC, the only subsequent callback will be the final callback if the first step callback fails.

[erlend-aasland]

If we had a way to hook into the SQLite memory allocator, we could simulate out-of-memory, but unfortunately the sqlite3 extension module does not support this (yet).

[serhiy-storchaka]

Should not we add checks also after other calls? Just for the case.

[erlend-aasland]

Should not we add checks also after other calls? Just for the case.

The query will be aborted if we set an SQLite error in step. We will go straight to final in that case; there will be no subsequent step callback (nor value nor inverse).

[erlend-aasland]

Quoting the example code the SQLite docs for the inverse callback:

/*
** xInverse for sumint().
**
** This does the opposite of xStep() - subtracts the value of the argument
** from the current context value. The error checking can be omitted from
** this function, as it is only ever called after xStep() (so the aggregate
** context has already been allocated) and with a value that has already
** been passed to xStep() without error (so it must be an integer).
*/

The comment from the value callback is not as explicit, though.

[serhiy-storchaka]

There are four calls of sqlite3_aggregate_context(). One (in final_callback) is checked for NULL. Two (in inverse_callback and value_callback) have asserts. Can we guarantee that they never fail?

[erlend-aasland]

There are four calls of sqlite3_aggregate_context(). One (in final_callback) is checked for NULL. Two (in inverse_callback and value_callback) have asserts. Can we guarantee that they never fail?

It is my understanding that the value and inverse callbacks will not be invoked after a failed step callback. I think it would be a breaking change if SQLite changed this behaviour. I think the asserts are enough.

[serhiy-storchaka]

I worry about the case when step was not failed. Can they be called without calling step?

[erlend-aasland]

I worry about the case when step was not failed. Can they be called without calling step?

final can be called for the case where no rows was returned by the SQL query. If no rows was returned, the value and inverse callbacks has no meaning at all.

[serhiy-storchaka]

Then LGTM.

[bedevere-app]

GH-129372 is a backport of this pull request to the 3.13 branch.

[bedevere-app]

GH-129373 is a backport of this pull request to the 3.12 branch.