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

[Yousong Zhou]

On 21 October 2014 10:02, Yousong Zhou <address@hidden> wrote:
> 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?

I mean, we can skip the strlen part and just do strcpy in the second
loop as we already know we have enough space in the dest buffer for
all those null-terminated arguments.

                 yousong