Fix double free in Container

[KowalskiThomas]

What is this PR?

This PR is an attempt to fix a crash I have been seeing in some of my code, where the service crashes with the following stack trace.

Error UnixSignal: Process terminated with SI_KERNEL (SIGSEGV)
#0   0x00007f0544208efa cfree
#1   0x00007f037419eda6 avio_close
#2   0x00007f037419ee4c avio_closep
#3   0x00007f030c058997 __pyx_f_2av_9container_6output_close_output (/project/src/av/container/output.c:3670:18)
#4   0x00007f030c058d6e __pyx_pf_2av_9container_6output_15OutputContainer_2__dealloc__ (/project/src/av/container/output.c:3989:15)
#5   0x00007f030c058d6e __pyx_pw_2av_9container_6output_15OutputContainer_3__dealloc__ (/project/src/av/container/output.c:3964:3)
#6   0x00007f030c058d6e __pyx_tp_dealloc_2av_9container_6output_OutputContainer (/project/src/av/container/output.c:10141:5)
#7   0x00007f0544506bd8 _Py_Dealloc (/usr/src/python/Objects/object.c:2390:6)
#8   0x00007f0544506bd8 Py_DECREF (/usr/src/python/./Include/object.h:538:9)
#9   0x00007f0544506bd8 delete_garbage (/usr/src/python/Modules/gcmodule.c:1018:17)
#10  0x00007f0544506bd8 gc_collect_main (/usr/src/python/Modules/gcmodule.c:1287:5)
#11  0x00007f054459a42b gc_collect_with_callback (/usr/src/python/Modules/gcmodule.c:1400:14)
#12  0x00007f05444fb6b3 _PyObject_GC_Link (/usr/src/python/Modules/gcmodule.c:2270:9)
#13  0x00007f05444fb6b3 gc_alloc (/usr/src/python/Modules/gcmodule.c:2290:5)
#14  0x00007f05444fb6b3 _PyObject_GC_New (/usr/src/python/Modules/gcmodule.c:2298:20)
#15  0x00007f05444fc1c4 new_dict (/usr/src/python/Objects/dictobject.c:740:14)
#16  0x00007f05444fc1c4 PyDict_New (/usr/src/python/Objects/dictobject.c:844:12)
#17  0x00007f054004fa40 _parse_object_unicode (/tmp/.tmpoBPaX4/sdists-v9/pypi/simplejson/3.17.6/6hOxRSxlrhf7G0Zm1T5Ae/src/simplejson/_speedups.c:1545:16)
#18  0x00007f054004fa40 scan_once_unicode (/tmp/.tmpoBPaX4/sdists-v9/pypi/simplejson/3.17.6/6hOxRSxlrhf7G0Zm1T5Ae/src/simplejson/_speedups.c:2218:20)
#19  0x00007f054004e7d5 _parse_object_unicode (/tmp/.tmpoBPaX4/sdists-v9/pypi/simplejson/3.17.6/6hOxRSxlrhf7G0Zm1T5Ae/src/simplejson/_speedups.c:1591:19)
#20  0x00007f054004e7d5 scan_once_unicode (/tmp/.tmpoBPaX4/sdists-v9/pypi/simplejson/3.17.6/6hOxRSxlrhf7G0Zm1T5Ae/src/simplejson/_speedups.c:2218:20)
#21  0x00007f054004e7d5 _parse_object_unicode (/tmp/.tmpoBPaX4/sdists-v9/pypi/simplejson/3.17.6/6hOxRSxlrhf7G0Zm1T5Ae/src/simplejson/_speedups.c:1591:19)
#22  0x00007f054004e7d5 scan_once_unicode (/tmp/.tmpoBPaX4/sdists-v9/pypi/simplejson/3.17.6/6hOxRSxlrhf7G0Zm1T5Ae/src/simplejson/_speedups.c:2218:20)
#23  0x00007f0540050037 scanner_call (/tmp/.tmpoBPaX4/sdists-v9/pypi/simplejson/3.17.6/6hOxRSxlrhf7G0Zm1T5Ae/src/simplejson/_speedups.c:2337:16)
#24  0x00007f054450f0af _PyObject_MakeTpCall (/usr/src/python/Objects/call.c:214:18)
#25  0x00007f0544517bad _PyEval_EvalFrameDefault (/usr/src/python/Python/ceval.c:4769:23)
#26  0x00007f0544513d9a _PyEval_EvalFrame (/usr/src/python/./Include/internal/pycore_ceval.h:73:16)
#27  0x00007f0544513d9a _PyEval_Vector (/usr/src/python/Python/ceval.c:6434:24)
#28  0x00007f054451b660 _PyEval_EvalFrameDefault (/usr/src/python/Python/ceval.c:5376:22)
#29  0x00007f0544513d9a _PyEval_EvalFrame (/usr/src/python/./Include/internal/pycore_ceval.h:73:16)
#30  0x00007f0544513d9a _PyEval_Vector (/usr/src/python/Python/ceval.c:6434:24)
#31  0x00007f054453cbf5 _PyVectorcall_Call (/usr/src/python/Objects/call.c:257:24)
#32  0x00007f054451b660 _PyEval_EvalFrameDefault (/usr/src/python/Python/ceval.c:5376:22)
#33  0x00007f054454bf3a _PyEval_EvalFrame (/usr/src/python/./Include/internal/pycore_ceval.h:73:16)
#34  0x00007f054454bf3a gen_send_ex2 (/usr/src/python/Objects/genobject.c:219:14)
#35  0x00007f054451a627 _PyEval_EvalFrameDefault (/usr/src/python/Python/ceval.c:2585:30)
#36  0x00007f054454bf3a _PyEval_EvalFrame (/usr/src/python/./Include/internal/pycore_ceval.h:73:16)
#37  0x00007f054454bf3a gen_send_ex2 (/usr/src/python/Objects/genobject.c:219:14)
#38  0x00007f054457efcf gen_iternext (/usr/src/python/Objects/genobject.c:594:9)
#39  0x00007f054457efcf builtin_next (/usr/src/python/Python/bltinmodule.c:1477:12)
#40  0x00007f0544518938 _PyEval_EvalFrameDefault (/usr/src/python/Python/ceval.c:5050:30)
#41  0x00007f0544513d9a _PyEval_EvalFrame (/usr/src/python/./Include/internal/pycore_ceval.h:73:16)
#42  0x00007f0544513d9a _PyEval_Vector (/usr/src/python/Python/ceval.c:6434:24)
#43  0x00007f05444dbf83 _PyObject_VectorcallTstate (/usr/src/python/./Include/internal/pycore_call.h:92:11)
#44  0x00007f05444dc000 context_run (/usr/src/python/Python/context.c:673)
#45  0x00007f0544524976 cfunction_vectorcall_FASTCALL_KEYWORDS (/usr/src/python/Objects/methodobject.c:443:24)
#46  0x00007f054451b660 _PyEval_EvalFrameDefault (/usr/src/python/Python/ceval.c:5376:22)
#47  0x00007f0544513d9a _PyEval_EvalFrame (/usr/src/python/./Include/internal/pycore_ceval.h:73:16)
#48  0x00007f0544513d9a _PyEval_Vector (/usr/src/python/Python/ceval.c:6434:24)
#49  0x00007f054454d2c0 _PyFunction_Vectorcall (/usr/src/python/Objects/call.c:393:16)
#50  0x00007f054454d2c0 _PyObject_VectorcallTstate (/usr/src/python/./Include/internal/pycore_call.h:92:11)
#51  0x00007f054454d2c0 method_vectorcall (/usr/src/python/Objects/classobject.c:67:20)
#52  0x00007f05445fad64 thread_run (/usr/src/python/./Modules/_threadmodule.c:1124:21)
#53  0x00007f05445c9e84 pythread_wrapper (/usr/src/python/Python/thread_pthread.h:241:5)

Looking at the stack trace, it's pretty clear the crash comes from close_output (definition). What's happening isn't obvious though.

Looking further into it, I think what is happening is the following:

• When OutputContainer gets GC'd, the Python reference to self.file is dropped
• Dropping that reference reference frees the PyIOFile which in turns call av_freep(&self.iocontext)
• Dropping the reference also triggers close_output
• close_output checks whether self.file is None. The thing is that, at this point it is None (as it was cleared by tp_clear) so it calls avio_closep(&self.ptr.pb)
• avio_closep(&self.ptr.pb) actually has already been called before -- by av_freep(&self.iocontext), which makes the whole thing crash.

I believe the reason why things are done this way is that self.file is None can also mean "the file was opened with avio_open / with a path", but it conflicts with what tp_clear does.

The proposed fix is to add a new bint _avio_opened attribute which will not be cleared (as opposed to a Python object reference/pointer) so the deallocator can just look at this to make the call of whether it should call avio_closep.

[WyattBlue]

avio_closep(&self.ptr.pb) actually has already been called before -- by av_freep(&self.iocontext), which makes the whole thing crash.

I'm calling AI slop, see #2202. av_freep does not call avio_closep(). Sure, it points to the same memory, but a human expect would make this distinction.

I think this does point to a real issue, so the human directing this is welcome to continue contributing to the project.
self.file is None does feel a little fishy here. However, this approach feels dubious and a bit brittle. I'm not elaborating further.

[KowalskiThomas]

Thanks for reviewing quickly. I plead guilty, I used LLMs for investigating and although the fix was not LLM made, it clearly was mostly guided by it. Sorry for posting that without digging further prior to doing it.

I also do believe that the issue is real as I’ve seen it happen several times, which is why I wanted to try and fix it.
So I’ll have another take at it when I get a chance (in the next few days). I’ll let you know what I find myself :)