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

Paul B Mahol onemda at gmail.com
Mon Aug 31 20:42:20 EEST 2020


On 8/31/20, Michael Niedermayer <michael at niedermayer.cc> wrote:
> 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.

There are only some restrictions that can be tracked by reading SDK code.
I prefer minimal intrusion in code - the minimal restrictions possible.


More information about the ffmpeg-devel mailing list