diff mbox

[FFmpeg-devel] mov: only read e_old if there were any old streams

Message ID d8bb479a-6b58-9245-6fbe-bf3c6593c7ca@googlemail.com
State Accepted
Headers show

Commit Message

Andreas Cadhalpun Nov. 1, 2016, 12:17 a.m. UTC 
On 31.10.2016 19:20, Sasi Inguva wrote:
> First of all, if nb_old == 0 i.e. there are no entries in AVIndex, then
> there is no point in calling mov_fix_index function at all. So instead of
> doing the above , you can directly check for st->nb_index_entries > 0 at
> the top of mov_fix_index and return otherwise.

OK, patch doing that is attached.

> Also, I don't understand how nb_old==0 can cause heap overflow. If I read
> the code correctly, if nb_old==0  find_prev_closest_keyframe_index , should
> return -1, which would make the function skip that edit list here
> 
>         if (index == -1) {
>            av_log(mov>->fc, AV_LOG_ERROR, "Missing key frame while reordering index according to edit list\n");
>           continue;
>        }

This checks is four lines below the heap buffer overflow, which is obviously too late.

Best regards,
Andreas

Comments

Sasi Inguva Nov. 1, 2016, 5:32 a.m. UTC | #1 
patch looks good to me. Thanks for the fix.

On Mon, Oct 31, 2016 at 5:17 PM, Andreas Cadhalpun <
andreas.cadhalpun@googlemail.com> wrote:

> On 31.10.2016 19:20, Sasi Inguva wrote:
> > First of all, if nb_old == 0 i.e. there are no entries in AVIndex, then
> > there is no point in calling mov_fix_index function at all. So instead of
> > doing the above , you can directly check for st->nb_index_entries > 0 at
> > the top of mov_fix_index and return otherwise.
>
> OK, patch doing that is attached.
>
> > Also, I don't understand how nb_old==0 can cause heap overflow. If I read
> > the code correctly, if nb_old==0  find_prev_closest_keyframe_index ,
> should
> > return -1, which would make the function skip that edit list here
> >
> >         if (index == -1) {
> >            av_log(mov>->fc, AV_LOG_ERROR, "Missing key frame while
> reordering index according to edit list\n");
> >           continue;
> >        }
>
> This checks is four lines below the heap buffer overflow, which is
> obviously too late.
>
> Best regards,
> Andreas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
Andreas Cadhalpun Nov. 1, 2016, 6:04 p.m. UTC | #2 
On 01.11.2016 06:32, Sasi Inguva wrote:
> patch looks good to me. Thanks for the fix.

Pushed.

Best regards,
Andreas
diff mbox

Patch

From 634682d0628d02a2941140800e901611bfee2d0c Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
Date: Tue, 1 Nov 2016 01:05:01 +0100
Subject: [PATCH] mov: immediately return from mov_fix_index without old index
 entries

If there are no index entries, e_old = st->index_entries is only one
byte large, since it was created by av_realloc called with size 0.

Thus accessing e_old[0].timestamp causes a heap buffer overflow.

Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
---
 libavformat/mov.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 414007e..7614632 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2961,7 +2961,7 @@  static void mov_fix_index(MOVContext *mov, AVStream *st)
     int first_non_zero_audio_edit = -1;
     int packet_skip_samples = 0;
 
-    if (!msc->elst_data || msc->elst_count <= 0) {
+    if (!msc->elst_data || msc->elst_count <= 0 || nb_old <= 0) {
         return;
     }
     // Clean AVStream from traces of old index
-- 
2.10.1