[FFmpeg-devel] [PATCH] avformat/mov: fix integer overflow

[Ganesh Ajjanagadde]

On Mon, Oct 12, 2015 at 12:43 PM, Michael Niedermayer
<michael at niedermayer.cc> wrote:
> On Sat, Oct 10, 2015 at 01:51:10PM -0400, Ganesh Ajjanagadde wrote:
>> Partially fixes Ticket 4727.
>>
>> -duration is not a safe expression, since duration can be INT_MIN.
>> One might ask how it can become INT_MIN.
>> Although it is true that line 2574 is no longer reached with INT_MIN due
>> to commit 053e80f6eaf8d87521fe58ea96886b6ee0bbe59d (which fixed another
>> integer overflow issue), mov_update_dts_shift is called on line 3549 as
>> well, right after a read of untrusted data.
>> One can do the fix locally there, but that function is already a huge
>> mess. Changing mov_update_dts_shift is likely better.
>>
>> This changes duration to INT_MIN + 1 in such cases. This should not make any
>> practical difference since such streams are anyway fuzzer files.
>>
>> Tested with FATE.
>>
>> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> ---
>>  libavformat/mov.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 4c073a3..87b46cf 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -2521,6 +2521,8 @@ static int mov_read_stts(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>  static void mov_update_dts_shift(MOVStreamContext *sc, int duration)
>>  {
>>      if (duration < 0) {
>> +        if (duration == INT_MIN)
>> +            duration++;
>>          sc->dts_shift = FFMAX(sc->dts_shift, -duration);
>
> should be ok
>
> though i think duration == INT_MIN should maybe be treated as error
> prior to mov_update_dts_shift

I think it should just be a warning, I will add a log line at that
AV_LOG_WARNING right before duration++. It makes no sense to me why
e.g passing in INT_MIN + 1 to mov_update_dts_shift should not be an
error, but this one is. Warning should suffice, and in my view is also
necessary: any weird transformation like this which is slightly lossy
should be warned about.

I would like to keep it in mov_update_dts_shift to avoid code
duplication, unless you strongly prefer otherwise.

>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> What does censorship reveal? It reveals fear. -- Julian Assange
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>