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

[Tim Rühsen]

Am Dienstag, 21. Oktober 2014, 16:49:12 schrieb Yousong Zhou:
> On 21 October 2014 16:17, Pär Karlsson <feino...@gmail.com> wrote:
> > 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.
> 
> Yes. Grepping through the code shows that currently there is no
> invocation of concat_strings() having more than 5 arguments.
> 
> > 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? :-)
> 
> I think concat_strings() is more tight and readable than multiple
> strlen() + malloc() + snprintf().
> 
> Regards.
> 
>                yousong

Reading the answers here tells me, something is wrong with this function. 
There is misunderstanding / misinterpretation regarding the code.
Not only the developers here are affected, but also clang (version 3.3 upto 
3.6).

Here is my approach - introducing a helper function (strlcpy, which exists in 
BSD for a long time). It seems very straight forward to me.

How you like it ?


char *
concat_strings (const char *str0, ...)
{
  va_list args;
  const char *arg;
  size_t length = 0, pos = 0;
  char *s;

  if (!str0)
    return NULL;

  va_start (args, str0);
  for (arg = str0; arg; arg = va_arg (args, const char *))
    length += strlen(arg);
  va_end (args);

  s = xmalloc (length + 1);

  va_start (args, str0);
  for (arg = str0; arg; arg = va_arg (args, const char *))
    pos += strlcpy(s + pos, arg, length - pos + 1);
  va_end (args);

  return s;
}


strlcpy() can often be used as replacement for
  len = snprintf(buf,"%s",string);
but is ways faster.

FYI, my strlcpy() code is


#ifndef HAVE_STRLCPY
size_t
strlcpy (char *dst, const char *src, size_t size)
{
  const char *old = src;

  /* Copy as many bytes as will fit */
  if (size)
    {
      while (--size)
        {
          if (!(*dst++ = *src++))
            return src - old - 1;
        }

      *dst = 0;
    }

  while (*src++);
  return src - old - 1;
}
#endif


BTW, I already tested this in my local copy of Wget. Passes all tests (as 
expected ;-)

Tim

signature.asc
Description: This is a digitally signed message part.