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]

Date: Wed, 14 Feb 2024 12:34:46 -0300
From: Rodrigo Campos <rodrigo@...g.com.ar>
To: Willy Tarreau <w@....eu>
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

On 2/11/24 11:48, Willy Tarreau wrote:
> On Mon, Jan 29, 2024 at 03:15:14PM +0100, Rodrigo Campos wrote:
> 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

The sizes here don't match that, even using gcc 9.5.0 from debian sid 
(your example in the other email was calling gcc 9.5).

Here I see bigger sizes, even using the same line you share to compile.

> For me it's 58 bytes, or 19 less / 25% smaller, and at first glance it
> should do the right thing as well.

I see 69 bytes for that func (nm --size says 45, that is in hex).
The function I posted in the patchset I see it as 101 bytes, so that is 
here is 32 bytes less here.

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.

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.

This one is 90 bytes here.

What do you think? Can you make them shorter?

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