On Fri, 26 Feb 2016 13:17:29 +0100 Reimar Döffinger <reimar.doeffin...@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.doeffin...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
