[FFmpeg-devel] [PATCH] mjpegdec: Do not assume unused plane pointer are NULL.

Michael Niedermayer michael at niedermayer.cc
Fri Feb 26 14:48:14 CET 2016


On Fri, Feb 26, 2016 at 02:20:33PM +0100, Reimar Döffinger wrote:
> On 26.02.2016, at 14:01, Michael Niedermayer <michael at niedermayer.cc> wrote:
> 
> > On Fri, Feb 26, 2016 at 01:17:29PM +0100, Reimar Döffinger wrote:
> >> Well, there I go, doing my own idle discussions
> >> I just complained about ;)
> >> 
> >> On Fri, Feb 26, 2016 at 01:00:23PM +0100, wm4 wrote:
> >>> On Fri, 26 Feb 2016 12:19:18 +0100
> >>> Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> >>>> I am not exactly sure what their opinion is to be honest.
> >>>> However this attitude of "it's of course a bug in the calling code! Nobody can actually know it because it's 100% undocumented requirement but it's their bug" pisses me of quite a bit.
> >>> 
> >>> Well, that often happens to me too in general. You're free top audit
> >>> ALL the decoders and filters to use the number of planes instead of
> >>> checking plane pointers. As you see, there's a bit of a practical
> >>> problem with this. It doesn't help either making half of the API handle
> >>> this "correctly" (as you or any other sane person might consider it),
> >>> while the other half still exposes the other behavior.
> >> 
> >> For something that is potentially exploitable?
> >> I disagree. Every single one fixed is a relevant improvement.
> >> There is also the point that every place that uses a better way
> >> is one place less where someone might copy-paste non-optimal
> >> code from.
> >> But I admit I consider this mostly a stop-gap, I am kind of in
> >> favour of checking the get_buffer2 result.
> >> But that has some issues and tricky points so probably needs to
> >> be done with care.
> > 
> > so the possible "solutions" are (not neccessarily exclusive)
> > 1. document that unused pointers must be NULL
> > 2. Check if the unused pointers from the user app are NULL, fail
> >   if not.
> > 3. clear the pointers from the user app which are unused
> > 4. make a copy and clear the pointers from the user app which are
> >   unused, if they arent NULL
> > 5. audit alot of code and change it so it works even if some pointers
> >   are invalid
> > 6. Ignore it, assuming all user apps which do this have hit crashes
> >   and no longer pass stale pointers
> > 
> > 
> > either 1, 3, 4 or 5 is needed, as docs and code mismatch
> 
> I think I'd like to do 3 + print warning, with hwaccel excepted.

> I might also look into 5, as a kind of "defense in depth" thing, so please tell me if you think you'd object to such "unnecessary" changes so I do not waste my time.

i dont object, and iam ok with the mjpeg patch
though i was wondering how robust using nb_components is
ultimately it is a variable read from the bitstream, pix_fmt and
frame set after it
it is checked but its consistency depends on quite some code (that is
pix_fmt being set consistently with it and no change to nb_components
or pix_fmt / frame leaking without the other, i think its ok but
maybe a small explicit check should be added somewhere to check that
the nb_components and pix_fmt are consistent

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160226/5e476a7d/attachment.sig>


More information about the ffmpeg-devel mailing list