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

[Andreas Cadhalpun]

On 01.11.2016 06:32, Sasi Inguva wrote:
> patch looks good to me. Thanks for the fix.

Pushed.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Sasi Inguva]

patch looks good to me. Thanks for the fix.

On Mon, Oct 31, 2016 at 5:17 PM, Andreas Cadhalpun <
andreas.cadhal...@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
>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Andreas Cadhalpun]

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
>From 634682d0628d02a2941140800e901611bfee2d0c Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun 
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 
---
 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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Sasi Inguva]

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.

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;}


On Sun, Oct 30, 2016 at 12:11 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> This fixes a heap buffer overflow.
>
> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavformat/mov.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 357d800..95b546e 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -3028,7 +3028,7 @@ static void mov_fix_index(MOVContext *mov, AVStream
> *st)
>  // Audio decoders like AAC need need a decoder delay samples
> previous to the current sample,
>  // to correctly decode this frame. Hence for audio we seek to
> a frame 1 sec. before the
>  // edit_list_media_time to cover the decoder delay.
> -search_timestamp = FFMAX(search_timestamp - mov->time_scale,
> e_old[0].timestamp);
> +search_timestamp = FFMAX(search_timestamp - mov->time_scale,
> nb_old ? e_old[0].timestamp : INT64_MIN);
>  }
>
>  index = find_prev_closest_keyframe_index(st, e_old, nb_old,
> search_timestamp, 0);
> --
> 2.10.1
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel