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 <[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..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); + 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-20 22:14 GMT+02:00 Pär Karlsson <[email protected]>: > 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 >> > >
