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

[Pär Karlsson]

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 <address@hidden>:

> Hi, Pär
>
> On 17 October 2014 03:50, Pär Karlsson <address@hidden> 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 <address@hidden>
> > 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  <address@hidden>
> > +
> > +       * utils.c (concat_strings): fixed arbitrary limit of 5 arguments
> to
> > +       function
> > +
> >  2014-05-03  Tim Ruehsen  <address@hidden>
> >
> >         * retr.c (retrieve_url): fixed memory leak
> >
> > commit 1fa9ff274dcb6e5a2dbbbc7d3fe2f139059c47f1
> > Author: Pär Karlsson <address@hidden>
> > 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
>