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

[Reimar Döffinger]

On 26.02.2016, at 02:38, Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Fri, Feb 26, 2016 at 12:15:19AM +0100, Reimar Döffinger wrote:
>> We do neither document nor check such a requirement
>> and for application-provided get_buffer2 they could
>> contain the result of a malloc(0) or whatever value
>> they had previously.
>> This fixes a use-after-free in e.g. MPlayer:
>> https://trac.mplayerhq.hu/ticket/2262
>> We might want to consider changing the (documented)
>> API in addition though.
>> 
>> Signed-off-by: Reimar Döffinger <Reimar.Doeffinger at gmx.de>
>> ---
>> libavcodec/mjpegdec.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
> 
> the assumtation that unused plane pointers are NULL is more
> widespread than mjpeg i think

Possible, but it's the only place someone ran across so far.
Decoders that cannot reconfigure are also much less likely to be affected by stale pointers.

> also, is it really a good idea to leave stale pointers in the array?

No, of course not. The question from my point of view is rather: should stale pointers or malloc(0) lead directly to an exploitable buffer overflow? Because that's what the current code does.
So I think we should avoid these lazy NULL checks as a matter of code resilience.
I think it would also be reasonable to extend e.g. code calling get_buffer2 to validate the result, though I have a slight concern that that is a bit of an API change.