On Mon, Aug 31, 2020 at 11:14:07AM +0200, Paul B Mahol wrote: > On 8/31/20, Michael Niedermayer <mich...@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 <mich...@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.
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
