KaxBlockBlob brokenness

Dennis Filder via Matroska-devel <[email protected]> Sat, 24 Apr 2021 16:45:49 +0200
Newsgroups gmane.comp.multimedia.matroska.devel
Message-ID <20210424144549.GA3567@reader>
Hi,

in my on-going quest to get the test suite working again I've hit a
bit of a roadblock.  The way I see it the class KaxBlockBlob in its
current form is inherently broken due to its member "Block".  If used
as intended the .group/.simpleblock points at a KaxBlockGroup or
KaxSimpleBlock instance.  The problem is that this instance will
sooner or later end up as a child in a KaxCluster instance whose
~EbmlMaster() dtor will then invoke "delete" on it.  ~KaxBlockBlob()
will try the same leading to a double-free and segfault.

To make matters worse, KaxReferenceBlock::SetReferencedBlock(const
KaxBlockGroup &) allocates a KaxBlockBlob instance and hooks the
passed-in KaxBlockGroup (which stems from a previously written block
and is now reused as a B- or P-frame referent) to it and then sets
bOurBlob=true.  The call to FreeBlob() (introduced with commit
697ce733589c44ac10dd46d46bfd28a075dea5cc) in ~KaxReferenceBlock() then
frees it, but in the mean time, the dtor of the referent's parent (a
KaxCluster) could have already freed it leading to another double-free
and segfault.

I could get mux/test6.cpp to run only by disabling any deletes on
KaxBlockBlob::Block.group, but that of course is not a permanent
remedy.  How is this supposed to work/be fixed?  I was thinking up
thousands of clever little additions to solve this with bools, but my
conclusion is that the only way to not make this even worse is to
involve shared_ptr.  One approach could be to involve shared_ptr only
for those elements which have another element point to it.  This could
work by attaching to the Segment instance a map that maps raw pointer
values to shared_ptr instances.  When associating a KaxBlockBlob with
a KaxBlockGroup or KaxSimpleBlock (or creating any other kind of thing
referencing another element), a shared_ptr is created from the raw
pointer and a copy of it is put into the map for ~EbmlMaster() to
free.  ~EbmlMaster() could then (1) walk up the tree (for which it
would probably need a schema-specific lambda since EbmlElement has no
GetParent()) to get to the EbmlMaster holding the map, (2) then look
up each raw pointer from Element_List in it, and (3) if nothing is
found invoke "delete" on the raw pointer, otherwise call reset() on
the retrieved shared_ptr.  KaxBlockBlob::Block would have to become a
union of shared_ptr, of course.

This would clarify the ownership rules in the library by mandating
that an object is owned by its parent EbmlMaster unless it has been
entered into the Segment's map in which case shared_ptr rules apply.

Let me know what you think of this (and also why EbmlElement lacks
pointers to the parent element -- given that it would have been
trivial to add it must have been omitted for a good reason).

Regards,
Dennis.
_______________________________________________
Matroska-devel mailing list
[email protected]
https://lists.matroska.org/cgi-bin/mailman/listinfo/matroska-devel
Read Matroska-Devel on GMane: http://dir.gmane.org/gmane.comp.multimedia.matroska.devel