[FFmpeg-devel] [Patch] fix ffprobe crash #3603

Stefano Sabatini stefasab at gmail.com
Tue May 13 01:53:07 CEST 2014


In data Tuesday 2014-05-13 02:11:14 +0530, Anshul ha scritto:
> On May 13, 2014 12:04:25 AM IST, Stefano Sabatini <stefasab at gmail.com> wrote:
> >On date Saturday 2014-05-10 13:44:40 +0530, Anshul encoded:
> >> On May 10, 2014 3:45:41 AM IST, Michael Niedermayer
> ><michaelni at gmx.at> wrote:
> >> >On Fri, May 09, 2014 at 04:15:36PM +0530, anshul wrote:
> >> >> On 05/09/2014 12:47 PM, Clément Boesch wrote:
> >> >> >On Fri, May 09, 2014 at 09:15:53AM +0200, Clément Boesch wrote:
> >> >> >>On Fri, May 09, 2014 at 12:33:36PM +0530, anshul wrote:
> >> >> >>>On 05/09/2014 06:15 AM, Michael Niedermayer wrote:
> >> >> >>>>>this patch consider all three things.
> >> >> >>>>did you intend to attach anoter patch ?
> >> >> >>>>iam asking as there was no patch attached to your last mail
> >> >> >>>>
> >> >> >>>>
> >> >> >>>yes, I am sorry for that.
> >> >> >>>
> >> >> >>>-Anshul
> >> >> >>> From 3ee1e369b42f0baa29706739f0b328615d20ebee Mon Sep 17
> >00:00:00
> >> >2001
> >> >> >>>From: Anshul Maheshwari <er.anshul.maheshwari at gmail.com>
> >> >> >>>Date: Thu, 8 May 2014 12:23:26 +0530
> >> >> >>>Subject: [PATCH] ffprobe: fix crash because of new streams
> >> >occuring
> >> >> >>>
> >> >> >>>Fix ticket #3603
> >> >> >>>---
> >> >> >>>  ffprobe.c | 19 ++++++++++++++-----
> >> >> >>>  1 file changed, 14 insertions(+), 5 deletions(-)
> >> >> >>>
> >> >> >>>diff --git a/ffprobe.c b/ffprobe.c
> >> >> >>>index c6e0469..5d6bf01 100644
> >> >> >>>--- a/ffprobe.c
> >> >> >>>+++ b/ffprobe.c
> >> >> >>>@@ -191,6 +191,7 @@ static const char unit_hertz_str[]         
> >=
> >> >"Hz"   ;
> >> >> >>>  static const char unit_byte_str[]           = "byte" ;
> >> >> >>>  static const char unit_bit_per_second_str[] = "bit/s";
> >> >> >>>+static int nb_streams;
> >> >> >>>  static uint64_t *nb_streams_packets;
> >> >> >>>  static uint64_t *nb_streams_frames;
> >> >> >>>  static int *selected_streams;
> >> >> >>>@@ -1893,6 +1894,12 @@ static int
> >> >read_interval_packets(WriterContext *w, AVFormatContext *fmt_ctx,
> >> >> >>>          goto end;
> >> >> >>>      }
> >> >> >>>      while (!av_read_frame(fmt_ctx, &pkt)) {
> >> >> >>>+        if(fmt_ctx->nb_streams >= nb_streams) {
> >> >> >>>+            nb_streams_frames  =
> >> >av_realloc(nb_streams_frames,fmt_ctx->nb_streams*
> >> >sizeof(*nb_streams_frames));
> >> >> >>>+            nb_streams_packets =
> >> >av_realloc(nb_streams_packets,fmt_ctx->nb_streams*
> >> >sizeof(*nb_streams_packets));
> >> >> >>>+            selected_streams   =
> >> >av_realloc(selected_streams,fmt_ctx->nb_streams*
> >> >sizeof(*selected_streams));
> >> >> >>space after ,
> >> >> >>space before *
> >> >> >for the mult obviously
> >> >> >
> >> >> >And speaking of this, you should use av_realloc_array for the
> >> >overflow
> >> >> >check.
> >> >> >
> >> >> >>space before (
> >> >> >>
> >> >> >for the if
> >> >> >
> >> >> >[...]
> >> >> >
> >> >> >
> >> >> >
> >> >> >_______________________________________________
> >> >> >ffmpeg-devel mailing list
> >> >> >ffmpeg-devel at ffmpeg.org
> >> >> >http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >> >> Please ignore previous patch, i don't know what is wrong with me.
> >> >> Again attached new patch for fixing this crash
> >> >> -Anshul
> >> >
> >> >>  ffprobe.c |   40 ++++++++++++++++++++++++++++++++++------
> >> >>  1 file changed, 34 insertions(+), 6 deletions(-)
> >> >> cefae455261a61fba6796d0dc5d261349ee44385 
> >> >0001-ffprobe-fix-crash-because-of-new-streams-occuring.patch
> >> >> From 12685c54a34b6ab5fcbc70cf86c4248dede61bdc Mon Sep 17 00:00:00
> >> >2001
> >> >> From: Anshul Maheshwari <er.anshul.maheshwari at gmail.com>
> >> >> Date: Fri, 9 May 2014 16:12:28 +0530
> >> >> Subject: [PATCH] ffprobe: fix crash because of new streams
> >occuring
> >> >> 
> >> >> Fix ticket #3603
> >> >> ---
> >> >>  ffprobe.c | 40 ++++++++++++++++++++++++++++++++++------
> >> >>  1 file changed, 34 insertions(+), 6 deletions(-)
> >> >> 
> >> >> diff --git a/ffprobe.c b/ffprobe.c
> >> >> index c6e0469..b9528e5 100644
> >> >> --- a/ffprobe.c
> >> >> +++ b/ffprobe.c
> >> >> @@ -191,9 +191,10 @@ static const char unit_hertz_str[]          =
> >> >"Hz"   ;
> >> >>  static const char unit_byte_str[]           = "byte" ;
> >> >>  static const char unit_bit_per_second_str[] = "bit/s";
> >> >>  
> >> >> +static int nb_streams;
> >> >
> >> >> -static uint64_t *nb_streams_packets;
> >> >> -static uint64_t *nb_streams_frames;
> >> >> -static int *selected_streams;
> >> >> +static uint64_t *nb_streams_packets = NULL;
> >> >> +static uint64_t *nb_streams_frames = NULL;
> >> >> +static int *selected_streams = NULL;
> >> >
> >> >thats unrelated
> >> >statics are already initialized to 0 by default
> >> >
> >> >
> >> 
> >> Done 
> >> >>  
> >> >>  static void ffprobe_cleanup(int ret)
> >> >>  {
> >> >> @@ -1893,6 +1894,25 @@ static int
> >read_interval_packets(WriterContext
> >> >*w, AVFormatContext *fmt_ctx,
> >> >>          goto end;
> >> >>      }
> >> >>      while (!av_read_frame(fmt_ctx, &pkt)) {
> >> >> +        if (fmt_ctx->nb_streams > nb_streams) {
> >> >> +            ret = av_reallocp_array(&nb_streams_frames,
> >> >fmt_ctx->nb_streams, sizeof(*nb_streams_frames));
> >> >> +            if(ret)
> >> >> +                goto end;
> >> >> +            ret = av_reallocp_array(&nb_streams_packets,
> >> >fmt_ctx->nb_streams, sizeof(*nb_streams_packets));
> >> >> +            if(ret)
> >> >> +                goto end;
> >> >> +            ret = av_reallocp_array(&selected_streams,
> >> >fmt_ctx->nb_streams, sizeof(*selected_streams));
> >> >> +            if(ret)
> >> >> +                goto end;
> >> >> +            memset(nb_streams_frames + nb_streams, 0,
> >> >> +                  (fmt_ctx->nb_streams - nb_streams) *
> >> >sizeof(*nb_streams_frames));
> >> >> +            memset(nb_streams_packets + nb_streams, 0,
> >> >> +                  (fmt_ctx->nb_streams - nb_streams) *
> >> >sizeof(*nb_streams_packets));
> >> >> +            memset(selected_streams + nb_streams, 0,
> >> >> +                  (fmt_ctx->nb_streams - nb_streams) *
> >> >sizeof(*selected_streams));
> >> >> +            nb_streams = fmt_ctx->nb_streams;
> >> >> +        }
> >> >
> >> >> +#if 0
> >> >>          if (selected_streams[pkt.stream_index]) {
> >> >>              AVRational tb =
> >> >fmt_ctx->streams[pkt.stream_index]->time_base;
> >> >>  
> >> >> @@ -1928,6 +1948,7 @@ static int
> >read_interval_packets(WriterContext
> >> >*w, AVFormatContext *fmt_ctx,
> >> >>              }
> >> >>          }
> >> >>          av_free_packet(&pkt);
> >> >> +#endif
> >> >
> >> >why is this code left in there and disabled ?
> >> >
> >> >[...]
> >> 
> >> Sorry for that I was debugging by ifdefs and left one there only,
> >thats not required.
> >> 
> >> -Anshul
> >> 
> >> -- 
> >> Sent from my Android device with K-9 Mail. Please excuse my brevity.
> >
> >> From 59c61b155ce290b80da4c50db84d0ecd6a139180 Mon Sep 17 00:00:00
> >2001
> >> From: Anshul <er.anshul.maheshwari at gmail.com>
> >> Date: Sat, 10 May 2014 13:23:11 +0530
> >> Subject: [PATCH] ffprobe: fix crash because of new streams occuring
> >> 
> >> Fix ticket #3603
> >> ---
> >>  ffprobe.c | 32 +++++++++++++++++++++++++++++---
> >>  1 file changed, 29 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/ffprobe.c b/ffprobe.c
> >> index c6e0469..b3d7aed 100644
> >> --- a/ffprobe.c
> >> +++ b/ffprobe.c
> >> @@ -191,6 +191,7 @@ static const char unit_hertz_str[]          =
> >"Hz"   ;
> >>  static const char unit_byte_str[]           = "byte" ;
> >>  static const char unit_bit_per_second_str[] = "bit/s";
> >>  
> >> +static int nb_streams;
> >>  static uint64_t *nb_streams_packets;
> >>  static uint64_t *nb_streams_frames;
> >>  static int *selected_streams;
> >> @@ -1893,6 +1894,24 @@ static int read_interval_packets(WriterContext
> >*w, AVFormatContext *fmt_ctx,
> >>          goto end;
> >>      }
> >>      while (!av_read_frame(fmt_ctx, &pkt)) {
> >> +        if (fmt_ctx->nb_streams > nb_streams) {
> >> +            ret = av_reallocp_array(&nb_streams_frames,
> >fmt_ctx->nb_streams, sizeof(*nb_streams_frames));
> >
> >> +            if(ret)
> >> +                goto end;
> >
> >if (ret < 0)
> >
> >here and below
> 
> Done
> 
> >
> >> +            ret = av_reallocp_array(&nb_streams_packets,
> >fmt_ctx->nb_streams, sizeof(*nb_streams_packets));
> >> +            if(ret)
> >> +                goto end;
> >> +            ret = av_reallocp_array(&selected_streams,
> >fmt_ctx->nb_streams, sizeof(*selected_streams));
> >> +            if(ret)
> >> +                goto end;
> >> +            memset(nb_streams_frames + nb_streams, 0,
> >> +                  (fmt_ctx->nb_streams - nb_streams) *
> >sizeof(*nb_streams_frames));
> >> +            memset(nb_streams_packets + nb_streams, 0,
> >> +                  (fmt_ctx->nb_streams - nb_streams) *
> >sizeof(*nb_streams_packets));
> >> +            memset(selected_streams + nb_streams, 0,
> >> +                  (fmt_ctx->nb_streams - nb_streams) *
> >sizeof(*selected_streams));
> >
> >nit: you can probably factorize this with a macro
> >
> I have tried to factorize. 
> >> +            nb_streams = fmt_ctx->nb_streams;
> >> +        }
> >>          if (selected_streams[pkt.stream_index]) {
> >>              AVRational tb =
> >fmt_ctx->streams[pkt.stream_index]->time_base;
> >>  
> >> @@ -2373,10 +2392,17 @@ static int probe_file(WriterContext *wctx,
> >const char *filename)
> >>          return ret;
> >>  
> >>  #define CHECK_END if (ret < 0) goto end
> >> +    nb_streams = fmt_ctx->nb_streams;
> >> +    if(av_reallocp_array(&nb_streams_frames, fmt_ctx->nb_streams,
> >sizeof(*nb_streams_frames)))
> >> +        CHECK_END;
> >
> >are you missing the ret assignment?
> >ret = av_reallocp_array(...);
> >CHECK_END;
> >
> Yes i was.
> 
> >> +    if(av_reallocp_array(&nb_streams_packets, fmt_ctx->nb_streams,
> >sizeof(*nb_streams_packets)))
> >> +        CHECK_END;
> >> +    if(av_reallocp_array(&selected_streams, fmt_ctx->nb_streams,
> >sizeof(*selected_streams)))
> >> +        CHECK_END;
> >>  
> >> -    nb_streams_frames  = av_calloc(fmt_ctx->nb_streams,
> >sizeof(*nb_streams_frames));
> >> -    nb_streams_packets = av_calloc(fmt_ctx->nb_streams,
> >sizeof(*nb_streams_packets));
> >> -    selected_streams   = av_calloc(fmt_ctx->nb_streams,
> >sizeof(*selected_streams));
> >> +    memset(nb_streams_frames, 0, nb_streams *
> >sizeof(*nb_streams_frames));
> >> +    memset(nb_streams_packets, 0, nb_streams *
> >sizeof(*nb_streams_packets));
> >> +    memset(selected_streams, 0, nb_streams *
> >sizeof(*selected_streams));
> >
> >[Will probably fix it and push it myself later if I'm still alive at
> >the end of the day.]
> 
> I was already dead, so could not pull the git, and download that sample to test this latest one. 
> 
> -Anshul
> 
> -- 
> Sent from my Android device with K-9 Mail. Please excuse my brevity.

> From f0c899cae1568795fd5391d87bb1c8aef8328911 Mon Sep 17 00:00:00 2001
> From: Anshul <er.anshul.maheshwari at gmail.com>
> Date: Tue, 13 May 2014 01:54:09 +0530
> Subject: [PATCH] ffprobe: fix crash because of new streams occuring
> 
> Fix ticket #3603
> ---
>  ffprobe.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/ffprobe.c b/ffprobe.c
> index c6e0469..22a9886 100644
> --- a/ffprobe.c
> +++ b/ffprobe.c
> @@ -191,6 +191,7 @@ static const char unit_hertz_str[]          = "Hz"   ;
>  static const char unit_byte_str[]           = "byte" ;
>  static const char unit_bit_per_second_str[] = "bit/s";
>  
> +static int nb_streams;
>  static uint64_t *nb_streams_packets;
>  static uint64_t *nb_streams_frames;
>  static int *selected_streams;
> @@ -1893,6 +1894,19 @@ static int read_interval_packets(WriterContext *w, AVFormatContext *fmt_ctx,
>          goto end;
>      }
>      while (!av_read_frame(fmt_ctx, &pkt)) {
> +        if (fmt_ctx->nb_streams > nb_streams) {
> +#define REALLOCZ_ARRAY_STREAM (ptr, cur_n, new_n)                      \
> +{                                                                      \
> +    ret = av_reallocp_array(&(ptr), (new_n), sizeof(*(ptr)));          \

> +    if( ret < 0)                                                       \
> +        goto end;                                                      \

Nit: if_(ret < 0)

> +    memset( (ptr) + (cur_n), 0, (new_n) - (cur_n) * sizeof(*(ptr)) )   \
> +}

> +            REALLOCZ_ARRAY_STREAM(nb_streams_frames, nb_streams, fmt_ctx->nb_streams);
> +            REALLOCZ_ARRAY_STREAM(nb_streams_packets,fmt_ctx->nb_streams);
> +            REALLOCZ_ARRAY_STREAM(selected_streams,fmt_ctx->nb_streams);

inconsistent number of arguments

> +            nb_streams = fmt_ctx->nb_streams;
> +        }
>          if (selected_streams[pkt.stream_index]) {
>              AVRational tb = fmt_ctx->streams[pkt.stream_index]->time_base;
>  
> @@ -2373,10 +2387,17 @@ static int probe_file(WriterContext *wctx, const char *filename)
>          return ret;
>  
>  #define CHECK_END if (ret < 0) goto end
> +    nb_streams = fmt_ctx->nb_streams;
> +    ret = av_reallocp_array(&nb_streams_frames, fmt_ctx->nb_streams, sizeof(*nb_streams_frames));
> +        CHECK_END;
> +    ret = av_reallocp_array(&nb_streams_packets, fmt_ctx->nb_streams, sizeof(*nb_streams_packets));
> +        CHECK_END;
> +    ret = av_reallocp_array(&selected_streams, fmt_ctx->nb_streams, sizeof(*selected_streams));
> +        CHECK_END;
>  
> -    nb_streams_frames  = av_calloc(fmt_ctx->nb_streams, sizeof(*nb_streams_frames));
> -    nb_streams_packets = av_calloc(fmt_ctx->nb_streams, sizeof(*nb_streams_packets));
> -    selected_streams   = av_calloc(fmt_ctx->nb_streams, sizeof(*selected_streams));
> +    memset(nb_streams_frames, 0, nb_streams * sizeof(*nb_streams_frames));
> +    memset(nb_streams_packets, 0, nb_streams * sizeof(*nb_streams_packets));
> +    memset(selected_streams, 0, nb_streams * sizeof(*selected_streams));

what about using the same macro as above?


More information about the ffmpeg-devel mailing list