On Wed, Oct 26, 2016 at 01:59:59AM +0200, Andreas Cadhalpun wrote: > On 25.10.2016 14:56, Hendrik Leppkes wrote: > > On Tue, Oct 25, 2016 at 2:39 PM, wm4 <nfx...@googlemail.com> wrote: > >> On Tue, 25 Oct 2016 09:47:29 +0200 > >> Hendrik Leppkes <h.lepp...@gmail.com> wrote: > >> > >>> On Tue, Oct 25, 2016 at 1:50 AM, Andreas Cadhalpun > >>> <andreas.cadhal...@googlemail.com> wrote: > >>>> This should reduce the impact of a demuxer (or API user) setting bogus > >>>> codec parameters. > >>>> > >>>> > >>> > >>> This seems rather noisy > > I've added a macro to make it less noisy. > > >>> and doesn't really solve anything, does it? > > As you analyze below, it was fixing only a part of the problem. > > >>> Decoders still need to validate values instead of blindly trusting > >>> them, and this just hides some problems in these decoders, > > Yes, the problem hiding is bad, which is why I added av_assert2's > so that developers can easily check if the validation fails. > > >>> instead of > >>> fixing them there. API users of avcodec would not fill > >>> AVCodecParameters, they would fill a codec context directly. > >> > >> You could also argue that the demuxer shouldn't return invalid > >> parameters either. > > > > It should not, but this patch does not address this. > > Indeed, the demuxers should be fixed in addition to this patch. > > > There is various combinations of component usage that are possible, > > and in fact are used in the real world: > > > > avformat -> avcodec > > other demuxer -> avcodec > > avformat -> other decoder > > > > This patch only addresses the first case, and only if you actually use > > this function (which I for example do not, since I have an abstraction > > layer in between, so I never have AVCodecParameters and AVCodecContext > > in the same function). > > So in short, it just doesn't fix much, and you can still get invalid > > output from avformat, and potentially still undefined behavior in > > avcodec if its fed those values through other means. > > For the third case, the demuxers have to be fixed. Having the asserts > in the central code makes it much easier to find these problems. > > >> How about this: always convert the params to a temporary codecpar, and > >> provide a function to determine the validity of a codecpar. This way > >> the check could be done in multiple places without duplicating the code > >> needed for it. > > > > That still sounds odd, although slightly better. At the very least it > > should be a dedicated function that checks the values in a key place, > > say you want to check params that are fed to a decoder, then call this > > check in avcodec_open, because thats something everyone has to call to > > use avcodec. > > I tried to implement the suggestions of both of you, see attached patch. > > Note that the added asserts are triggered by *many* of my fuzzed samples. > I'm happy to write patches for these cases, if we achieve agreement > that the central check alone is insufficient. > Another noteworthy point is that this patch makes it easy to trigger > the av_assert0 in iff_read_packet. I think that assert should be replaced > with an error return. > > Best regards, > Andreas >
> utils.c | 82 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 80 insertions(+), 2 deletions(-) > 40a8bafecb6d289a4220a27ac411fbcac3204952 > 0001-avcodec-validate-codec-parameters.patch > From f371be7a027da3958e221b4dc88ad558c1489107 Mon Sep 17 00:00:00 2001 > From: Andreas Cadhalpun <andreas.cadhal...@googlemail.com> > Date: Tue, 25 Oct 2016 01:45:27 +0200 > Subject: [PATCH] avcodec: validate codec parameters > > This should reduce the impact of a broken demuxer (or API user) setting bogus > codec parameters. > > The av_assert2 calls should ease detecting broken demuxers. have you tried a fuzzer ? these assertions fail on fuzzed files Assertion 0 failed at libavcodec/utils.c:4157 Aborted Assertion !((unsigned)par->color_primaries > AVCOL_PRI_NB) failed at libavcodec/utils.c:4161 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Observe your enemies, for they first find out your faults. -- Antisthenes
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
