Yes, you are right, of course. Looking through the original implementation again, it seems water tight. clang probably complains about the uninitialized values above argcount in saved_lengths[], that are never reached.
The precalculated strlen:s saved is likely only an optimization(?) attempt, I suppose. Still, it seems wasteful to set up two complete loops with va_arg, and considering what this function actually does, I wonder if not s(n)printf should be used instead of this function? :-) Best regards, /Pär 2014-10-21 4:22 GMT+02:00 Yousong Zhou <yszhou4t...@gmail.com>: > Hi, Pär > > On 17 October 2014 03:50, Pär Karlsson <feino...@gmail.com> wrote: > > 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. > > After taking a closer look, I guess the old implementation is fine. > saved_length[] is used as a buffer for lengths of the first 5 > arguments and there is a bound check with its length. Maybe it's a > false-positive from clang tool? > > Sorry for the noise... > > Regards. > > yousong > > > > > All tests working ok with this patch. > > > > This is my first patch to this list, by the way. I'd be happy to help out > > more in the future. > > > > Best regards, > > > > /Pär Karlsson, Sweden > > > > ---- > > > > commit 2d855670e0e1fbe578506b376cdd40b0e465d3ef > > Author: Pär Karlsson <feino...@gmail.com> > > Date: Thu Oct 16 21:41:36 2014 +0200 > > > > Updated ChangeLog > > > > diff --git a/src/ChangeLog b/src/ChangeLog > > index 1c4e2d5..1e39475 100644 > > --- a/src/ChangeLog > > +++ b/src/ChangeLog > > @@ -1,3 +1,8 @@ > > +2014-10-16 Pär Karlsson <feino...@gmail.com> > > + > > + * utils.c (concat_strings): fixed arbitrary limit of 5 arguments > to > > + function > > + > > 2014-05-03 Tim Ruehsen <tim.rueh...@gmx.de> > > > > * retr.c (retrieve_url): fixed memory leak > > > > commit 1fa9ff274dcb6e5a2dbbbc7d3fe2f139059c47f1 > > Author: Pär Karlsson <feino...@gmail.com> > > Date: Wed Oct 15 00:00:31 2014 +0200 > > > > Generalized concat_strings argument length > > > > The concat_strings function seemed arbitrary to only accept a > maximum > > of 5 arguments (the others were silently ignored). > > > > Also it had a potential garbage read of the values in the array. > > Updated with xmalloc/xrealloc/free > > > > 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); > > + 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); > > total_length += len; > > } > > va_end (args); > > @@ -393,7 +394,7 @@ concat_strings (const char *str0, ...) > > } > > va_end (args); > > *p = '\0'; > > - > > + free(saved_lengths); > > return ret; > > } > > ^L >