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

[wm4]

On Fri, 26 Feb 2016 13:17:29 +0100
Reimar Döffinger <Reimar.Doeffinger at gmx.de> 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.

checking get_buffer2 usage sounds like a good idea. It should also
check whether the refcounted buffers are setup correctly.

> > By the way, this inconsistency at hand has actually helped me a little
> > in the past. I can use the "free" plane pointers in hwaccel frames for
> > additional hwaccel-specific information.  
> 
> Which from my point of view illustrates the whole problem:
> Now our API is "oh, we actually kind of expect you to set unused
> pointers to NULL. Except that it usually works either way.
> And of course sometimes apps will set them to something
> and rely on it, so we can't actually enforce it properly.
> So nobody really knows.
> But if you get it wrong, it's exploitable".
> To me this is not just a "minor thing".

Which is why the API should be exactly defined.

By the way, I find it TERRIBLE that ffmpeg code checks for plane
pointers being NULL, instead of the proper plane count.

> 
> > > So nobody has said a single thing about what you think _should_ be done.
> > > Just leave it and hey, if someone has the unbelievable gall of making a mistake and not setting a pointer to NULL (which like wm4 pointed out has happened to him, too) they deserve to have an exploit?
> > > I really don't feel like idle discussions that lead nowhere.  
> > 
> > Maybe clarify it in the AVFrame doxygen. And again, I'm not against
> > this specific patch; it just feels a bit pointless and won't fix the
> > deeper problems.  
> 
> Ok, but for that we actually need to figure out what we require
> (and if we do, why not actually check instead of just document?).
> I suspect you for example wouldn't be happy if we suddenly required
> all unused hwaccel pointers to be NULL.
> And if we don't require them to be NULL, should we maybe test it?
> E.g. a mode for FATE where it sets all pointers to 32 instead of NULL?
> Though do we ever test hwaccel much anyway? I think nobody did any
> further work on creating a test infrastructure for it?
> 
> > You could also ask why an API user is setting unused plane pointers to
> > random values, though.  
> 
> Funny thing to say considering you just said you do exactly that,
> on purpose...

True. But some things just don't make sense anyway API-wise with
hwaccel.