[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Bug-wget] [PATCH] Small fix for limited number of strings (and pote
From: |
Pär Karlsson |
Subject: |
Re: [Bug-wget] [PATCH] Small fix for limited number of strings (and potential garbage value) in arguments to concat_strings |
Date: |
Mon, 20 Oct 2014 23:47:26 +0200 |
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 <address@hidden>
+
+ * 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 <address@hidden>
* 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 <address@hidden>:
> 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 <address@hidden>
> +
> + * 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 <address@hidden>
>
> * 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 <address@hidden>:
>
>> Pär Karlsson <address@hidden> 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 <address@hidden>
>> > 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
>>
>
>
- [Bug-wget] [PATCH] Small fix for limited number of strings (and potential garbage value) in arguments to concat_strings, Pär Karlsson, 2014/10/16
- Re: [Bug-wget] [PATCH] Small fix for limited number of strings (and potential garbage value) in arguments to concat_strings, Tim Rühsen, 2014/10/17
- Re: [Bug-wget] [PATCH] Small fix for limited number of strings (and potential garbage value) in arguments to concat_strings, Giuseppe Scrivano, 2014/10/19
- Re: [Bug-wget] [PATCH] Small fix for limited number of strings (and potential garbage value) in arguments to concat_strings, Pär Karlsson, 2014/10/20
- Re: [Bug-wget] [PATCH] Small fix for limited number of strings (and potential garbage value) in arguments to concat_strings,
Pär Karlsson <=
- Re: [Bug-wget] [PATCH] Small fix for limited number of strings (and potential garbage value) in arguments to concat_strings, Yousong Zhou, 2014/10/20
- Re: [Bug-wget] [PATCH] Small fix for limited number of strings (and potential garbage value) in arguments to concat_strings, Yousong Zhou, 2014/10/20
- Re: [Bug-wget] [PATCH] Small fix for limited number of strings (and potential garbage value) in arguments to concat_strings, Micah Cowan, 2014/10/21
- Re: [Bug-wget] [PATCH] Small fix for limited number of strings (and potential garbage value) in arguments to concat_strings, Pär Karlsson, 2014/10/21
- Re: [Bug-wget] [PATCH] Small fix for limited number of strings (and potential garbage value) in arguments to concat_strings, Micah Cowan, 2014/10/21
Re: [Bug-wget] [PATCH] Small fix for limited number of strings (and potential garbage value) in arguments to concat_strings, Yousong Zhou, 2014/10/20