[vlc-devel] [RFC] demux/mp4: fix 17584: signal error if ReadBox_default fails

[Filip Roséen]

Hi Francois,

While composing this email I noticed that there is a bug in
`demux/mp4` that neither of our two approaches address; I will create
a trac-ticket and fix it as soon as possible.

On 2016-11-04 14:55, Francois Cartegnie wrote:

> Le 04/11/2016 à 14:28, Filip Roséen a écrit :
> 
> >> I'm allowed to check if a box exists or not, even if it's unknown.
> >> Use cases I can think about: encryption test. We don't decode it, but we
> >> want to check if it applies.
> 
> ^
> > With that said; rejecting the patch by saying that it is wrong due to
> > a problem that the implementation already suffers from is in my
> > opinion a *strawman* in terms of argumentation.
> 
> The strawman you're pointing to, isn't set in the right place.

I replied to what you wrote about the patch disrupting the debug
diagnostics, and given that your email did not contain any other
information my interpretation was that it was the reason for you not
liking the changes introduced.

> You're argumenting about a fix, which does not fix the original problem
> (ie the bug reported reduced the enclosing box by one, creating an
> infinite loop due to failed seek and relative position only), and that
> still broken function could be reused.

It does fix the immediate issue since known boxes contains data that
are to be read by the specific `ReadBox`-handler, if that
`ReadBox`-*callback* fails to read the contents of the box, it
would/should return `0` (to signal an error) and the box will be
discarded (aborting the infinite loop).

There will however be a problem for a `ReadBox`-handler, responsible
for a **known** box-type, that does not (directly or indirectly)
consume any data, but I cannot find any such box in the implementation
(nor do I think such box would make much sense).

> > If the structured debug output should include boxes that fails to
> > parse, the implementation should be changed to include this data (not
> > treat an unknown box as if it was parsed successfully, in my opinion).
> 
> This is an inherited 'feature' and there's/was no safe mechanism for
> controlling access to data members on corrupt/empty mandatory boxes.

Then I honestly do not see the point of bringing that up.

> And in case you still want to discuss about the "regression",
> 
> > p_box->e_flags |= BOX_FLAG_INCOMPLETE;
> 
> wouldn't be there if it was to be released on return.

The `BOX_FLAG_INCOMPLETE` is only used in order to print the
debug-tree, which inherently suffers from the problem described
earlier; boxes that fail to parse are not included.

So that "regression" is, in my opinion, just exposing a bug with the
debug implementation (not the other way around).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161104/84b18b87/attachment.html>