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

[Hendrik Leppkes]

On Fri, Feb 26, 2016 at 11:35 AM, Reimar Döffinger
<Reimar.Doeffinger at gmx.de> wrote:
> 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.

I mostly agree with Michael and wm4, this would appear to be a bug in
the calling code more than anything else.
But on the other hand there is nothing really wrong with fixing this
particular case either, but personally I wouldn't feel  it worth it to
go chase through the entire code base trying to find all such cases.

For adding checks, would at the very least have to be careful when it
comes to hwaccels, as they store random pointers in the data pointers
at varying offsets.

- Hendrik