Re: [PATCH v2 8/9] list-objects-filter-options: clean up use of ALLOC_GROW

[Jacob Keller <jacob.keller@gmail.com>]

On Fri, May 31, 2019 at 5:40 PM Matthew DeVore <matvore@google.com> wrote:
>
> Introduce a new macro ALLOC_GROW_BY which automatically zeros the added
> array elements and takes care of updating the nr value. Use the macro in
> code introduced earlier in this patchset.
>
> Signed-off-by: Matthew DeVore <matvore@google.com>
> ---
>  cache.h                       | 22 ++++++++++++++++++++++
>  list-objects-filter-options.c | 17 +++++++----------
>  2 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index fa8ede9a2d..847fbdeff0 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -652,33 +652,55 @@ int init_db(const char *git_dir, const char *real_git_dir,
>  void sanitize_stdfds(void);
>  int daemonize(void);
>
>  #define alloc_nr(x) (((x)+16)*3/2)
>
>  /*
>   * Realloc the buffer pointed at by variable 'x' so that it can hold
>   * at least 'nr' entries; the number of entries currently allocated
>   * is 'alloc', using the standard growing factor alloc_nr() macro.
>   *
> + * Consider using ALLOC_GROW_BY instead of ALLOC_GROW as it has some
> + * added niceties.
> + *
>   * DO NOT USE any expression with side-effect for 'x', 'nr', or 'alloc'.
>   */
>  #define ALLOC_GROW(x, nr, alloc) \
>         do { \
>                 if ((nr) > alloc) { \
>                         if (alloc_nr(alloc) < (nr)) \
>                                 alloc = (nr); \
>                         else \
>                                 alloc = alloc_nr(alloc); \
>                         REALLOC_ARRAY(x, alloc); \
>                 } \
>         } while (0)
>
> +/*
> + * Similar to ALLOC_GROW but handles updating of the nr value and
> + * zeroing the bytes of the newly-grown array elements.
> + *
> + * DO NOT USE any expression with side-effect for any of the
> + * arguments.
> + */

Since ALLOC_GROW already doesn't handle this safely, there isn't
necessarily a reason to fix it, but you could read the macro values
into temporary variables inside the do { } while(0) loop in order to
avoid the multiple-expansion side effect issues...

Thanks,
Jake

> +#define ALLOC_GROW_BY(x, nr, increase, alloc) \
> +       do { \
> +               if (increase) { \
> +                       size_t new_nr = nr + (increase); \
> +                       if (new_nr < nr) \
> +                               BUG("negative growth in ALLOC_GROW_BY"); \
> +                       ALLOC_GROW(x, new_nr, alloc); \
> +                       memset((x) + nr, 0, sizeof(*(x)) * (increase)); \
> +                       nr = new_nr; \
> +               } \
> +       } while (0)
> +
>  /* Initialize and use the cache information */
>  struct lock_file;
>  void preload_index(struct index_state *index,
>                    const struct pathspec *pathspec,
>                    unsigned int refresh_flags);
>  int do_read_index(struct index_state *istate, const char *path,
>                   int must_exist); /* for testting only! */
>  int read_index_from(struct index_state *, const char *path,
>                     const char *gitdir);
>  int is_index_unborn(struct index_state *);
> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
> index 5e98e4a309..d8abe6cfcf 100644
> --- a/list-objects-filter-options.c
> +++ b/list-objects-filter-options.c
> @@ -142,26 +142,24 @@ static int has_reserved_character(
>         }
>
>         return 0;
>  }
>
>  static int parse_combine_subfilter(
>         struct list_objects_filter_options *filter_options,
>         struct strbuf *subspec,
>         struct strbuf *errbuf)
>  {
> -       size_t new_index = filter_options->sub_nr++;
> +       size_t new_index = filter_options->sub_nr;
>
> -       ALLOC_GROW(filter_options->sub, filter_options->sub_nr,
> -                  filter_options->sub_alloc);
> -       memset(&filter_options->sub[new_index], 0,
> -              sizeof(*filter_options->sub));
> +       ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,
> +                     filter_options->sub_alloc);
>
>         return has_reserved_character(subspec, errbuf) ||
>                 url_decode(subspec, errbuf) ||
>                 gently_parse_list_objects_filter(
>                         &filter_options->sub[new_index], subspec->buf, errbuf);
>  }
>
>  static int parse_combine_filter(
>         struct list_objects_filter_options *filter_options,
>         const char *arg,
> @@ -273,27 +271,26 @@ int parse_list_objects_filter(
>                 /*
>                  * Make filter_options an LOFC_COMBINE spec so we can trivially
>                  * add subspecs to it.
>                  */
>                 transform_to_combine_type(filter_options);
>
>                 strbuf_addstr(&filter_options->filter_spec, "+");
>                 add_url_encoded(&filter_options->filter_spec, arg);
>                 trace_printf("Generated composite filter-spec: %s\n",
>                              filter_options->filter_spec.buf);
> -               ALLOC_GROW(filter_options->sub, filter_options->sub_nr + 1,
> -                          filter_options->sub_alloc);
> -               filter_options = &filter_options->sub[filter_options->sub_nr++];
> -               memset(filter_options, 0, sizeof(*filter_options));
> +               ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,
> +                             filter_options->sub_alloc);
>
>                 parse_error = gently_parse_list_objects_filter(
> -                       filter_options, arg, &errbuf);
> +                       &filter_options->sub[filter_options->sub_nr - 1], arg,
> +                       &errbuf);
>         }
>         if (parse_error)
>                 die("%s", errbuf.buf);
>         return 0;
>  }
>
>  int opt_parse_list_objects_filter(const struct option *opt,
>                                   const char *arg, int unset)
>  {
>         struct list_objects_filter_options *filter_options = opt->value;
> --
> 2.17.1
>