Re: [Bug-wget] [PATCH] Small fix for limited number of strings (and potential garbage value) in arguments to concat_strings

[Giuseppe Scrivano]

Pär Karlsson <address@hidden> writes:

> Hi, I fould a potential gotcha when playing with clang's code analysis tool.
>
> The concat_strings function silently stopped counting string lengths when
> given more than 5 arguments. clang warned about potential garbage values in
> the saved_lengths array, so I redid it with this approach.
>
> All tests working ok with this patch.

thanks for your contribution.  I've just few comments:


> commit 2d855670e0e1fbe578506b376cdd40b0e465d3ef
> Author: Pär Karlsson <address@hidden>
> Date:   Thu Oct 16 21:41:36 2014 +0200
>
>     Updated ChangeLog
>

we usually update the changelog in the same commit that made the change,
so please squash these two commits into one.

Also, it doesn't apply on current git master, as it seems to be based on
a old version of git from the ChangeLog file context, could you rebase
onto the master branch?


> diff --git a/src/utils.c b/src/utils.c
> index 78c282e..93c9ddc 100644
> --- a/src/utils.c
> +++ b/src/utils.c
> @@ -356,7 +356,8 @@ char *
>  concat_strings (const char *str0, ...)
>  {
>    va_list args;
> -  int saved_lengths[5];         /* inspired by Apache's apr_pstrcat */
> +  size_t psize = sizeof(int);

please leave a space between sizeof and '(' as mandated by the GNU
coding standards.

> +  int *saved_lengths = xmalloc (psize);
>    char *ret, *p;
>
>    const char *next_str;
> @@ -370,8 +371,8 @@ concat_strings (const char *str0, ...)
>    for (next_str = str0; next_str != NULL; next_str = va_arg (args, char *))
>      {
>        int len = strlen (next_str);
> -      if (argcount < countof (saved_lengths))
> -        saved_lengths[argcount++] = len;
> +      saved_lengths[argcount++] = len;
> +      xrealloc(saved_lengths, psize * argcount);

same here.

>        total_length += len;
>      }
>    va_end (args);
> @@ -393,7 +394,7 @@ concat_strings (const char *str0, ...)
>      }
>    va_end (args);
>    *p = '\0';
> -
> +  free(saved_lengths);

and here.

Regards,
Giuseppe