[FFmpeg-devel] [PATCH v4] avcodec/cfhd: More strictly check tag order and multiplicity

[Michael Niedermayer]

On Mon, Aug 31, 2020 at 11:14:07AM +0200, Paul B Mahol wrote:
> On 8/31/20, Michael Niedermayer <michael at niedermayer.cc> wrote:
> > This is based on the encoder and a small number of CFHD sample files
> > It should make the decoder more robust against crafted input.
> > Due to the lack of a proper specification it is possible that this
> > may be too strict and may need to be tuned as files not following this
> > ordering are found.
> >
> > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > ---
> 
> I'm afraid this is not nice solution.
> Please consider sharing samples that causes crash to me.
> IMO I'm afraid that this patch is just band-aid and not proper solution.

As this ended up discussed on IRC too, heres the relevant log as reference
for the archieve as otherwise noone will find it
an interleaved discussion about AAC with JEEB is removed

<michaelni> durandal_1707, kierank, i dont have any crashing samples which are fixed by the cfhd table patch. That patch was intended to restrict what/where elements can occur so that for example the one specifying the subband number is in the subband "header" and not the plane "header" and also not twice and also not missing.
<michaelni> Iam happy to implement this differently if you have a suggestion
<durandal_1707> ah, this is patch to "fix" timeouts
<michaelni> durandal_1707, why do you write such flaimbait replies? The patch has nothing to do with timeouts. Its rather that i see multiple fixes for out of array writes in the history of the CFHD decoder and its header parser allowing basically anything in any order no matter if it has any sense. And the patch tries to restrict this.
<michaelni> if you are against the patch just say so and ill work on something else
<michaelni> if work on this fron continues, the next logic step would be to ensure each band, plane and so on is coded exactly once
<michaelni> because IIRC ATM the decoder doesnt check this either not just that any band related field could be in the plane or main header part and so on
<j-b> I agree with michaelni here.
<j-b> The tone is flamebait-y for no reason.
<durandal_1707> michaelni: restricting that with ugly patch is not nice

to continue this on the ML which is where patch review belongs
which other way do you prefer ?
It could be implemented in many differnt ways, i cannot read your mind ...
Each method has some advanatges and disadvanatges from ease of 
supporting unexpected order in odd files to code complexity to ease of 
backporting the patch ...

The whole loop could be redesigned and split up into 1 function for 
each header type (main / plane / band / ...) each such function would
then have a similar loop with the subset of tags that can occur.
if you prefer that ?
but that would probably not be backported far, leaving some releases 
without this
or as already said, i can just work on something else if people dont like
this. 
Or maybe you want to implement this ? I can ofer to review your patch if
you want to implement this instead. 

Thanks

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200831/c1e3dc62/attachment.sig>