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 :)

[KowalskiThomas]

So I had another look this morning and although I'm not 100% confident what the fix is, I think I may at least have found something that seems off to me.

My understanding is that OutputContainer extends Container which wraps the "file" (really, IO) that is being written to.

When OutputContainer is GC'd with a PyIOFile...

• PyIOFile.__dealloc__ is called, which does
  • lib.av_freep(cython.address(self.iocontext.buffer))
  • lib.av_freep(cython.address(self.iocontext))
• OutputContainer.__dealloc__ is called, which does
  • close_output which calls av_write_trailer...
  • ... and avio_closep.

... I don't really understand why that would be, as my understanding is deallocation should go "child class first, then parent class, then parent class, etc." and here it seems like attributes of the parent class are being freed first, but adding debug prints shows that it is happening (see at the bottom).
(I'll try to understand why because the real fix would probably come naturally, knowing this, but I'd like your opinion on the current finding first.)

avio_closep according to the docs:

Close the resource accessed by the AVIOContext *s, free it and set the pointer pointing to it to NULL.

avformat_write_trailer on the other hand:

Write the stream trailer to an output media file and free the file private data.
May only be called after a successful call to avformat_write_header.

I wasn't able to reproduce the exact crash that I initially reported but it comes from a different platform than my own laptop. However, I was able to produce another crash around approximately the same code path (see here), and I suspect that it might have something to do with how strict/paranoid memory allocators are across systems.

I added some debug logging around those code paths (see here, same branch as before) and what it shows is the following:

Freeing iocontext->buffer: 34519728128
then freeing iocontext: 34498363392
Writing trailer, ptr.pb: 34498363392

So clearly (unless I'm missing something of course), here we're freeing a pointer through av_freep, then trying to write the trailer to the exact same pointer through av_write_trailer.
Since that same av_write_trailer also does free the file private data, I think assuming the first part goes unnoticed, it'll result in a double free anyway.

What do you think?