On Sat, Sep 19, 2020 at 7:45 PM Michael Niedermayer <mich...@niedermayer.cc> wrote:
> On Mon, Aug 31, 2020 at 07:42:20PM +0200, Paul B Mahol wrote: > > On 8/31/20, Michael Niedermayer <mich...@niedermayer.cc> wrote: > > > 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. > > > > There are only some restrictions that can be tracked by reading SDK code. > > I prefer minimal intrusion in code - the minimal restrictions possible. > > Iam not sure how to implement that in a way that works reliable and is not > more code than the table uses and probably a partial rewrite of the parser. > > some elements like width / height related ones are mandatory they must be > there, they must be there just once and they must be there for each > "channel" > that needs them. > The fuzzer just now found a case where just a missing height triggers a > segfault and we have a "minimal" test for height being too small / not set > already. The attacker just has too much freedom to structure the elements > so tests dont work in this case it seems > > If there is generic code that ensures all mandatory elements occur than > checking their values can be done when they are read. > Without such code, i guess all checks would need to be seperated from the > parsing as nothing in the parsing is guranteed to be run or the order in > which > its run. > The alternative is to rewrite the parser so as to only read elements in the > order they are supposed to occur and not a single loop parsing anything in > any order > Please share a sample that causes crash privately. > > thx > > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > it is not once nor twice but times without number that the same ideas make > their appearance in the world. -- Aristotle > _______________________________________________ > 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". _______________________________________________ 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".
