Re: bug: sipWrapper_dealloc should save/restore Python exception state to avoid corrupting it

On 05/06/2020 19:44, Nathaniel Smith wrote:
> Hi all,
> 
> In the Python C API, tp_dealloc hooks can get called at all sorts of
> weird times. For example, it can be called at a moment when there's a
> live exception being propagated.
> 
> This means that if your tp_dealloc tries to naively run some Python
> code, you might end up corrupting the interpreter's exception-tracking
> state, since it doesn't expect to start running regular code with a
> live exception. The solution is that tp_dealloc hooks should use
> PyErr_Fetch/PyErr_Restore to bracket any arbitrary code execution. For
> example, this is how the interpreter itself calls __del__ methods from
> tp_dealloc:
> 
> 
> https://github.com/python/cpython/blob/5f4b229df7812f1788287095eb6b138bb21876a4/Objects/typeobject.c#L6952-L6967
> 
> Unfortunately, sip's tp_dealloc code doesn't seem to include a similar
> save/restore. We encountered this while trying to use PyQt5 together
> with Python's async/await support:
> 
>    https://github.com/python-trio/trio/issues/1576
> 
> The summary is: async functions are essentially generators, and so
> when they return they raise a StopIteration exception to carry the
> return value. So this exception is "live" when the function frame is
> garbage collected, and all its locals are deallocated. This makes it
> especially easy to provoke bugs in exception/tp_dealloc interactions.
> In the link above we hit one case where sip's tp_dealloc ends up
> running arbitrary Python code and then generating a bunch of confused
> SystemError exceptions from the Python VM, but there are probably
> others. The link also includes a gdb backtrace showing a path from
> _Py_Dealloc -> sipWrapper_dealloc -> arbitrary Python.
> 
> Fortunately the solution is easy: just add PyErr_Fetch/PyErr_Restore
> to sipWrapper_dealloc.
> 
> It would be great if this could be fixed, since it's probably a
> blocker to using async/await with PyQt5.

Applied.

Thanks,
Phil