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

[Yousong Zhou]

Hi, Pär.  I got a few comments inline.

On 21 October 2014 05:47, Pär Karlsson <address@hidden> wrote:
> Whoops, I realised I failed on the GNU coding standards, please disregard
> the last one; the patch below should be better.
>
> My apologies :-/
>
> /Pär
>
> diff --git a/src/ChangeLog b/src/ChangeLog
> index d5aeca0..87abd85 100644
> --- a/src/ChangeLog
> +++ b/src/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-10-20     Pär Karlsson  <address@hidden>
> +
> +       * utils.c (concat_strings): got rid of double loop, cleaned up
> potential
> +       memory corruption if concat_strings was called with more than five
> args
> +
>  2014-10-16  Tim Ruehsen  <address@hidden>
>
>         * url.c (url_parse): little code cleanup
> diff --git a/src/utils.c b/src/utils.c
> index 78c282e..5f359e0 100644
> --- a/src/utils.c
> +++ b/src/utils.c
> @@ -356,42 +356,36 @@ char *
>  concat_strings (const char *str0, ...)
>  {
>    va_list args;
> -  int saved_lengths[5];         /* inspired by Apache's apr_pstrcat */
>    char *ret, *p;
>
>    const char *next_str;
> -  int total_length = 0;
> -  size_t argcount;
> +  size_t len;
> +  size_t total_length = 0;
> +  size_t charsize = sizeof (char);

I am not sure here.  Do we always assume sizeof(char) to be 1 for
platforms supported by wget?

> +  size_t chunksize = 64;
> +  size_t bufsize = 64;
> +
> +  p = ret = xmalloc (charsize * bufsize);
>
>    /* Calculate the length of and allocate the resulting string. */
>
> -  argcount = 0;
>    va_start (args, 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;
> +      len = strlen (next_str);
> +      if (len == 0)
> +        continue;
>        total_length += len;
> -    }
> -  va_end (args);
> -  p = ret = xmalloc (total_length + 1);
> -
> -  /* Copy the strings into the allocated space. */
> -
> -  argcount = 0;
> -  va_start (args, str0);
> -  for (next_str = str0; next_str != NULL; next_str = va_arg (args, char *))
> -    {
> -      int len;
> -      if (argcount < countof (saved_lengths))
> -        len = saved_lengths[argcount++];
> -      else
> -        len = strlen (next_str);
> +      if (total_length > bufsize)
> +      {
> +        bufsize += chunksize;

Should be `bufsize = total_length` ?

> +        ret = xrealloc (ret, charsize * bufsize);
> +      }
>        memcpy (p, next_str, len);

Xrealloc may return a new block different from p, so memcpy(p, ...)
may not be what you want.

>        p += len;
>      }
>    va_end (args);
> +  ret = xrealloc (ret, charsize * total_length + 1);
>    *p = '\0';

Malloc takes time.  How about counting total_length in one loop and
doing the copy in another?

Regards.

                yousong

>
>    return ret;
>