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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240218102003.GA32375@1wt.eu>
Date: Sun, 18 Feb 2024 11:20:03 +0100
From: Willy Tarreau <w@....eu>
To: Rodrigo Campos <rodrigo@...g.com.ar>
Cc: Thomas Weißschuh <linux@...ssschuh.net>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] tools/nolibc: Fix strlcat() return code and size
 usage

Hi Rodrigo,

On Wed, Feb 14, 2024 at 12:34:46PM -0300, Rodrigo Campos wrote:
> Here are two versions that are significantly shorter than the 101 bytes,
> that pass the tests (I added more to account for the src vs dst mismatch
> that was easy to pass tests when both buffers have the same size as they did
> before).
> 
> size_t strlcat_rata(char *dst, const char *src, size_t size)
> {
>         const char *orig_src = src;
>         size_t len = 0;
>         for (;len < size; len++) {
>                 if (dst[len] == '\0')
>                         break;
>         }
> 
>         /* Let's copy len < n < size-1 times from src.
>          * size is unsigned, so instead of size-1, that can wrap around,
>          * let's use len + 1 */
>         while (len + 1 < size) {
>                 dst[len] = *src;
>                 if (*src == '\0')
>                         break;
>                 len++;
>                 src++;
>         }
> 
>         if (src != orig_src)
>                 dst[len] = '\0';
> 
>         while (*src++)
>                 len++;
> 
>         return len;
> }
> 
> This one compiles to 81 bytes here using gcc 13.2.0 and to 83 using gcc
> 9.5.0. Compared to the one posted in the patchset, it is significantly
> smaller.

OK this looks good to me. I think your test on src != orig_src is not
trivial and that testing instead if (len < size) would be better, and
possibly even shorter.

> One based in the version you posted (uses strlen(dst) instead), is this one:
> 
> size_t strlcat_willy_fixed(char *dst, const char *src, size_t size)
> {
>         const char *orig_src = src;
>         size_t len = strlen(dst);
>         if (size < len)
>                 len = size;
> 
>         for (;len + 1 < size; len++, src++) {
>                 dst[len] = *src;
>                 if (*src == '\0')
>                         break;
>         }
> 
>         if (orig_src != src)
>                 dst[len] = '\0';
> 
>         while (*src++)
>                 len++;
> 
>         return len;
> }
> 
> 
> Note the "if size < len, then len=size", I couldn't get rid of it because we
> need to account for the smaller size of dst if we don't get passed it for
> the return code.

Please no, again as I mentioned earlier, it's wrong to use strlen(dst) in
this case: the only situation where we'd accept size < len is if dst is
already not zero-terminated, which means that strlen() cannot be used, or
you'd need strnlen() for that, I'm just seeing that we have it, I thought
we didn't.

> This one is 90 bytes here.

See the point I was making? Sometimes the amount of fat added around a
function call is important compared to just an increment and a comparison.
Of course that's not always true but that's one such example.

> What do you think? Can you make them shorter?

I don't want to enter that activity again this week-end ;-)  It's sufficient
here.

> If you like one of these, I can repost with the improved tests too.

Yeah please check the few points above for your version, make sure to
clean it up according to the kernel's coding style (empty line after
variable declaration, */ on the next line for the multi-line comment
etc) and that's fine.

Thanks,
Willy