[FFmpeg-devel] [PATCH 1/4] fftools/cmdutils: Atomically add elements to list of pointers, fix crash

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Currently, adding a (separately allocated) element to a list of pointers
> works by first reallocating the array of pointers and (on success)
> incrementing its size and only then allocating the new element.
> If the latter allocation fails, the size is inconsistent, i.e.
> array[nb_array_elems - 1] is NULL. Our cleanup code crashes in such
> scenarios.
> 
> Fix this by adding an auxiliary function that atomically allocates
> and adds a new element to a list of pointers.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
> ---
>  fftools/cmdutils.c      | 16 ++++++++++++++++
>  fftools/cmdutils.h      | 17 +++++++++++++++++
>  fftools/ffmpeg_filter.c | 17 ++++-------------
>  fftools/ffmpeg_opt.c    | 22 ++++++----------------
>  4 files changed, 43 insertions(+), 29 deletions(-)
> 
> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
> index 45322f8c71..1464b122df 100644
> --- a/fftools/cmdutils.c
> +++ b/fftools/cmdutils.c
> @@ -2214,6 +2214,22 @@ void *grow_array(void *array, int elem_size, int *size, int new_size)
>      return array;
>  }
>  
> +void *allocate_array_elem(void *ptr, size_t elem_size, int *nb_elems)
> +{
> +    void *new_elem, **array = (void**)ptr;
> +
> +    if (*nb_elems == INT_MAX) {
> +        av_log(NULL, AV_LOG_ERROR, "Array too big.\n");
> +        exit_program(1);
> +    }
> +    new_elem = av_mallocz(elem_size);
> +    if (!new_elem)
> +        exit_program(1);
> +    GROW_ARRAY(array, *nb_elems);
> +    array[*nb_elems - 1] = new_elem;
> +    return array;
> +}
> +
>  double get_rotation(int32_t *displaymatrix)
>  {
>      double theta = 0;
> diff --git a/fftools/cmdutils.h b/fftools/cmdutils.h
> index 64dd7266bc..ae78e60f4c 100644
> --- a/fftools/cmdutils.h
> +++ b/fftools/cmdutils.h
> @@ -628,11 +628,28 @@ FILE *get_preset_file(char *filename, size_t filename_size,
>   */
>  void *grow_array(void *array, int elem_size, int *size, int new_size);
>  
> +/**
> + * Atomically add a new element to an array of pointers, i.e. allocate
> + * a new entry, reallocate the array of pointers and make the new last
> + * member of this array point to the newly allocated buffer.
> + * Calls exit() on failure.
> + *
> + * @param array     array of pointers to reallocate
> + * @param elem_size size of the new element to allocate
> + * @param nb_elems  pointer to the number of elements of the array array;
> + *                  *nb_elems will be incremented by one by this function.
> + * @return reallocated array
> + */
> +void *allocate_array_elem(void *array, size_t elem_size, int *nb_elems);
> +
>  #define media_type_string av_get_media_type_string
>  
>  #define GROW_ARRAY(array, nb_elems)\
>      array = grow_array(array, sizeof(*array), &nb_elems, nb_elems + 1)
>  
> +#define ALLOC_ARRAY_ELEM(array, nb_elems)\
> +    array = allocate_array_elem(array, sizeof(*array[0]), &nb_elems)
> +
>  #define GET_PIX_FMT_NAME(pix_fmt)\
>      const char *name = av_get_pix_fmt_name(pix_fmt);
>  
> diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
> index dab0f28819..75194fe66e 100644
> --- a/fftools/ffmpeg_filter.c
> +++ b/fftools/ffmpeg_filter.c
> @@ -164,18 +164,14 @@ int init_simple_filtergraph(InputStream *ist, OutputStream *ost)
>          exit_program(1);
>      fg->index = nb_filtergraphs;
>  
> -    GROW_ARRAY(fg->outputs, fg->nb_outputs);
> -    if (!(fg->outputs[0] = av_mallocz(sizeof(*fg->outputs[0]))))
> -        exit_program(1);
> +    ALLOC_ARRAY_ELEM(fg->outputs, fg->nb_outputs);
>      fg->outputs[0]->ost   = ost;
>      fg->outputs[0]->graph = fg;
>      fg->outputs[0]->format = -1;
>  
>      ost->filter = fg->outputs[0];
>  
> -    GROW_ARRAY(fg->inputs, fg->nb_inputs);
> -    if (!(fg->inputs[0] = av_mallocz(sizeof(*fg->inputs[0]))))
> -        exit_program(1);
> +    ALLOC_ARRAY_ELEM(fg->inputs, fg->nb_inputs);
>      fg->inputs[0]->ist   = ist;
>      fg->inputs[0]->graph = fg;
>      fg->inputs[0]->format = -1;
> @@ -280,9 +276,7 @@ static void init_input_filter(FilterGraph *fg, AVFilterInOut *in)
>      ist->decoding_needed |= DECODING_FOR_FILTER;
>      ist->st->discard = AVDISCARD_NONE;
>  
> -    GROW_ARRAY(fg->inputs, fg->nb_inputs);
> -    if (!(fg->inputs[fg->nb_inputs - 1] = av_mallocz(sizeof(*fg->inputs[0]))))
> -        exit_program(1);
> +    ALLOC_ARRAY_ELEM(fg->inputs, fg->nb_inputs);
>      fg->inputs[fg->nb_inputs - 1]->ist   = ist;
>      fg->inputs[fg->nb_inputs - 1]->graph = fg;
>      fg->inputs[fg->nb_inputs - 1]->format = -1;
> @@ -318,10 +312,7 @@ int init_complex_filtergraph(FilterGraph *fg)
>          init_input_filter(fg, cur);
>  
>      for (cur = outputs; cur;) {
> -        GROW_ARRAY(fg->outputs, fg->nb_outputs);
> -        fg->outputs[fg->nb_outputs - 1] = av_mallocz(sizeof(*fg->outputs[0]));
> -        if (!fg->outputs[fg->nb_outputs - 1])
> -            exit_program(1);
> +        ALLOC_ARRAY_ELEM(fg->outputs, fg->nb_outputs);
>  
>          fg->outputs[fg->nb_outputs - 1]->graph   = fg;
>          fg->outputs[fg->nb_outputs - 1]->out_tmp = cur;
> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> index a27263b879..ffba7010e3 100644
> --- a/fftools/ffmpeg_opt.c
> +++ b/fftools/ffmpeg_opt.c
> @@ -1265,11 +1265,8 @@ static int open_input_file(OptionsContext *o, const char *filename)
>      /* dump the file content */
>      av_dump_format(ic, nb_input_files, filename, 0);
>  
> -    GROW_ARRAY(input_files, nb_input_files);
> -    f = av_mallocz(sizeof(*f));
> -    if (!f)
> -        exit_program(1);
> -    input_files[nb_input_files - 1] = f;
> +    ALLOC_ARRAY_ELEM(input_files, nb_input_files);
> +    f = input_files[nb_input_files - 1];
>  
>      f->ctx        = ic;
>      f->ist_index  = nb_input_streams - ic->nb_streams;
> @@ -2261,11 +2258,8 @@ static int open_output_file(OptionsContext *o, const char *filename)
>          }
>      }
>  
> -    GROW_ARRAY(output_files, nb_output_files);
> -    of = av_mallocz(sizeof(*of));
> -    if (!of)
> -        exit_program(1);
> -    output_files[nb_output_files - 1] = of;
> +    ALLOC_ARRAY_ELEM(output_files, nb_output_files);
> +    of = output_files[nb_output_files - 1];
>  
>      of->ost_index      = nb_output_streams;
>      of->recording_time = o->recording_time;
> @@ -3262,9 +3256,7 @@ static int opt_audio_qscale(void *optctx, const char *opt, const char *arg)
>  
>  static int opt_filter_complex(void *optctx, const char *opt, const char *arg)
>  {
> -    GROW_ARRAY(filtergraphs, nb_filtergraphs);
> -    if (!(filtergraphs[nb_filtergraphs - 1] = av_mallocz(sizeof(*filtergraphs[0]))))
> -        return AVERROR(ENOMEM);
> +    ALLOC_ARRAY_ELEM(filtergraphs, nb_filtergraphs);
>      filtergraphs[nb_filtergraphs - 1]->index      = nb_filtergraphs - 1;
>      filtergraphs[nb_filtergraphs - 1]->graph_desc = av_strdup(arg);
>      if (!filtergraphs[nb_filtergraphs - 1]->graph_desc)
> @@ -3281,9 +3273,7 @@ static int opt_filter_complex_script(void *optctx, const char *opt, const char *
>      if (!graph_desc)
>          return AVERROR(EINVAL);
>  
> -    GROW_ARRAY(filtergraphs, nb_filtergraphs);
> -    if (!(filtergraphs[nb_filtergraphs - 1] = av_mallocz(sizeof(*filtergraphs[0]))))
> -        return AVERROR(ENOMEM);
> +    ALLOC_ARRAY_ELEM(filtergraphs, nb_filtergraphs);
>      filtergraphs[nb_filtergraphs - 1]->index      = nb_filtergraphs - 1;
>      filtergraphs[nb_filtergraphs - 1]->graph_desc = graph_desc;
>  
> 

Will apply this patchset tonight unless there are objections.

- Andreas