gh-143198: fix some crashes in sqlite3.executemany with concurrent mutations

[picnixz]

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: Null pointer dereference in _sqlite cursor cache via re-entrant parameter iterator #143198

[picnixz]

There were more UAFs that I could find...

[picnixz]

Ok, let's avoid force-pushing more. I force pushed because I wanted a single commit for the follow-up PR

[picnixz]

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.

[picnixz]

There is a reason why I added this comment. I was myself confused when I used the same variable (one for internal C API and one for sqlite API call). Later I will rename that variable when I will refactor the functions.

[serhiy-storchaka]

Thank you for minimizing the diff, @picnixz.

It seems that most new checks are not covered by tests. At least tests were passed when I commented out them one by one. Maybe more tests are needed?

It seems that neither bind_param(), nor bind_parameters() use self->connection->db, so the check can be added only after the call of bind_parameters(). Maybe much later -- immediately before sqlite3_changes() and sqlite3_last_insert_rowid().

I wonder, what happens when we close the connection in one of numerous callbacks? Then self->connection->db can be set NULL after calling some SQLite C API, not only after calling some Python C API.

[serhiy-storchaka]

Isn't PyErr_Occurred redundant?

[picnixz]

This is what I thought but, in release mode the check is not performed. You can have a __len__ that returns -1 without setting an exception:

    PySequenceMethods *m = Py_TYPE(s)->tp_as_sequence;
    if (m && m->sq_length) {
        Py_ssize_t len = m->sq_length(s);
        assert(_Py_CheckSlotResult(s, "__len__", len >= 0));
        return len;
    }

[picnixz]

Actually, you're right. I thought we directly called __len__ but it's warpped as follows:

    PyObject *res = vectorcall_method(&_Py_ID(__len__), stack, 1);
    Py_ssize_t len;

    if (res == NULL)
        return -1;

    Py_SETREF(res, _PyNumber_Index(res));
    if (res == NULL)
        return -1;

So returning -1 always implies that there is an exception set here. I'll remove that check.

[serhiy-storchaka]

We can still get -1 for the extension class. But this is a bug in the extension.

[picnixz]

Do you want me to re-add the check though?

[picnixz]

Because on DEBUG builds there's some assert that would be crashing if the extension class does something bad

[picnixz]

@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).