Thank you for your feedback and suggestions. I thought about this during the weekend and figured it could be done much more efficiently, by only looping through the arguments once. Also, I realized the the original version would have segfaulted if called with more than 5 args, since the destination memory never got allocated before the memcpy (in the current code, it never is, though!).
I cleaned up the code according to the GNU coding standards as well. The test suite rolls with this, and I think it looks better (although the function is only really called in a handful of places in all of wget). Best regards, /Pär Patch below: 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 <[email protected]> + + * 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 <[email protected]> * url.c (url_parse): little code cleanup diff --git a/src/utils.c b/src/utils.c index 78c282e..dbeb9fe 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); + 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; + ret = xrealloc (ret, charsize * bufsize); + } memcpy (p, next_str, len); p += len; } va_end (args); + ret = xrealloc (ret, charsize * total_length + 1); *p = '\0'; return ret; 2014-10-19 16:16 GMT+02:00 Giuseppe Scrivano <[email protected]>: > Pär Karlsson <[email protected]> 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 <[email protected]> > > 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 >
