Messages in this thread | | | Date | Sun, 11 Feb 2024 11:48:17 +0100 | From | Willy Tarreau <> | Subject | Re: [PATCH 2/4] tools/nolibc: Fix strlcat() return code and size usage |
| |
Hi Rodrigo,
first, thanks for the series!
On Mon, Jan 29, 2024 at 03:15:14PM +0100, Rodrigo Campos wrote: > The return code should always be strlen(src) + strlen(dst), but dst is > considered shorter if size is less than strlen(dst). > > While we are there, make sure to copy at most size-1 bytes and > null-terminate the dst buffer. > > Signed-off-by: Rodrigo Campos <rodrigo@sdfg.com.ar> > --- > tools/include/nolibc/string.h | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/tools/include/nolibc/string.h b/tools/include/nolibc/string.h > index ed15c22b1b2a..b2149e1342a8 100644 > --- a/tools/include/nolibc/string.h > +++ b/tools/include/nolibc/string.h > @@ -187,23 +187,23 @@ char *strndup(const char *str, size_t maxlen) > static __attribute__((unused)) > size_t strlcat(char *dst, const char *src, size_t size) > { > - size_t len; > char c; > + size_t len = strlen(dst); > + size_t ret = strlen(src) + (size < len? size: len);
From what I'm reading in the man page, ret should *always* be the sum of the two string lengths. I guess it helps for reallocation. It's even explicitly mentioned:
"While this may seem somewhat confusing, it was done to make truncation detection simple."
Above ret will be bound to the existing size so a realloc wouldn't work. Thus I think the correct solution is:
size_t ret = strlen(src) + len;
> - for (len = 0; dst[len]; len++) > - ; > - > - for (;;) { > + for (;len < size;) { > c = *src; > - if (len < size) > + if (len < size - 1) > dst[len] = c; > + if (len == size - 1) > + dst[len] = '\0'; > if (!c) > break; > len++; > src++; > } > > - return len; > + return ret; > }
The test inside the loop is going to make this not very efficient. Same for the fact that we're measuring the length of src twice (once via strlen, a second time through the loop). I've just had a look and it compiles to 77 bytes at -Os. A simpler variant would consist in trying to copy what fits in <size> and once reached, go on just for trailing zero and the size measurement:
size_t strlcat(char *dst, const char *src, size_t size) { size_t len = strlen(dst);
while (len < size) { if (!(dst[len] = *src)) return len; len++; src++; }
/* end of src not found before size */
if (size) dst[size - 1] = '\0';
while (*src++) len++;
return len; }
For me it's 58 bytes, or 19 less / 25% smaller, and at first glance it should do the right thing as well.
What do you think ?
Thanks! Willy
| |