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

[Reimar Döffinger]

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.

> 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".

> > 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...