Hi, Pär. I got a few comments inline. On 21 October 2014 05:47, Pär Karlsson <feino...@gmail.com> 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 <feino...@gmail.com> > + > + * 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 <tim.rueh...@gmx.de> > > * 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; >