gh-142830: prevent some crashes when mutating sqlite3 callbacks

[picnixz]

This is not a complete fix because there are other callbacks that need to be tested. I could technically split the PR into two but I preferred testing the conversion to PyObject * in the same PR.

• Issue: Use-after-free in sqlite3 progress handler via re-entrant __bool__ #142830

[picnixz]

Actually, I'm a bit worried about backporting the change to 3.13 and 3.14 because we are now using a real heap type.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @picnixz for commit 4dd0652 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F143245%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

[serhiy-storchaka]

I think this can be resolved with smaller changes.

a) Just add a refcounter to the context struct. Increment and decrement it for use, and free the struct when it is 0. This is just a field and two macros or functions.
b) Save strong references to what you need in local variables just after getting the context.

[picnixz]

Mmh yes, that would aloow me to backport this and avoid burdening the GC. I didn't consider this approach as I am more used to rely on existing mechanisms.

I think I will add some macros or functions for holding the refs when needed.

[picnixz]

So I've tried, but I miserably failed. Each callback creates a shared callback context that is either freed when the connection dies or when it got replaced. The problem is when I replace a callback:

• Define cb(). This creates some conn->ctx.
• Execute cb. During the execution, remove cb.
• Removal of cb triggers removal of conn->ctx (it gets replaced by NULL) and conn->ctx is freed directly (deferring freeing the memory isn't possible because no one is tracking it after it's replaced in the connection's object)

Maybe I got it wrong or understood the mechanism wrongly, but relying on the GC is much more easier.

[serhiy-storchaka]

When the callback is added, set its refcount to 1. If it is replaced, decrement its refcount. When the callback is called, increment its refcount (so that it doesn't disappear from under our feet), after calling it, decrement its refcount. If after any decrement it becomes 0, free it. This is very similar to how refcounts work for PyObject, but We don't need PyTypeObject and other boilerplate.

Other way is just make local strong references to callback and module (if needed) and a weak reference to state (if needed) for the time of calling the callback, On this way you will only need to change the code that calls the callback, and use Python refcounts.

[serhiy-storchaka]

I do not talk what you should do, I only describe variants which may be more laconic than the current PR.

[picnixz]

I actually tried this initial approach but I thought "why not just rely on the GC" and then went with a full-fledged PyObject. Using a PyObject definitely makes my life easier as it's not me who's responsible for that but it's also fine to use another strategy that doesn't need this overhead (considering every statement would require INCREF/DECREF of the entire context, it's clearly not a good idea indeed).

[picnixz]

@serhiy-storchaka Thanks for the guidance! it's a bit annoying to do the manual refcounting and someone could easily forget to do it, but it's definitely better for the diff. I can also backport this PR with more confidence.

[serhiy-storchaka]

LGTM. 👍

[miss-islington-app]

Thanks @picnixz for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

[miss-islington-app]

Sorry, @picnixz, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 7f6c16a956d598663d8c67071c492f197045d967 3.14

[miss-islington-app]

Sorry, @picnixz, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 7f6c16a956d598663d8c67071c492f197045d967 3.13

[picnixz]

Of course backports have conflicts -_-

[bedevere-app]

GH-143322 is a backport of this pull request to the 3.14 branch.

[bedevere-app]

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