Re: [PATCH 2/4] tools/nolibc: Fix strlcat() return code and size usage

From: Rodrigo Campos
Date: Tue Feb 13 2024 - 01:20:29 EST

On 2/13/24 00:16, Rodrigo Campos wrote:
On 2/11/24 11:48, Willy Tarreau wrote:
Hi Rodrigo,

first, thanks for the series!

Thank you, for your time and review! :)

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@xxxxxxxxxxx>
  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."

Yes, that was my *first* understanding of the manpage too. But it doesn't seem to be the correct interpretation.

Also, this is exactly what I tried to say in the cover letter, with:

    I thought the manpage was clear, but when checking against that,
    I noted a few twists (like the manpage says the return code of
    strlcat is strlen(src) + strlen(dst), but it was not clear it is
    not that if size < strlen(dst). When looking at the libbsd
    implementation and re-reading the manpage, I understood what it
    really meant).

Sorry if I wasn't clear. Let me try again.

My first interpretation of the manpage was also that, and I think it would make sense to be that one. However, it is wrong, they also say this, that is what made me add the ternary operator:

    Note, however, that if strlcat() traverses size characters
    without finding a NUL, the length of the string is considered
    to be  size and the destination string will not be NUL
    terminated (since there was no space for the NUL)

So, my interpretation is that it is the sum of both, except when we can't find the NUL in that size, in which case we conside "size" to be the len of dst.

If you compare it with the output of libbsd, the return code seems to be exactly that. I was surprised too, as the manpage seem so clear... :-/

I'm not convinced now if that is the right interpretation. I'll check the libbsd code, I don't remember it now, it's been a few days already.

My memory is that it was not something so sane.