linux-kernel - Re: [PATCH 3/4] tools/nolibc: Fix strlcpy() 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 16:55:45 +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 3/4] tools/nolibc: Fix strlcpy() return code and size
 usage

On Wed, Feb 14, 2024 at 12:50:53PM -0300, Rodrigo Campos wrote:
> On 2/11/24 12:08, Willy Tarreau wrote:
> > Hi Rodrigo,
> > 
> > It's good, but for the same reason as the previous one, I'm getting
> > smaller code by doing less in the loop. Also calling strlen() here
> > looks expensive, I'm seeing that the compiler inlined it nevertheless
> > and did it in a dep-optimized way due to the asm statement. That
> > results in 67 bytes total while a simpler version gives 47.
> > 
> > If I explicitly mark strlen() __attribute__((noinline)) that prevents
> > it from doing so starting with gcc-10, where it correctly places a jump
> > from strlcpy() to strlen() and ends up with 50 bytes (vs 44 for the alt
> > one). The other one I can propose is directly derived from the other
> > strlcat() variant, which first performs the copy and starts to count:
> > 
> > size_t strlcpy(char *dst, const char *src, size_t size)
> > {
> >          size_t len;
> > 
> >          for (len = 0; len < size; len++) {
> >                  if (!(dst[len] = src[len]))
> >                          return len;
> >          }
> > 
> >          /* end of src not found before size */
> >          if (size)
> >                  dst[size - 1] = '\0';
> > 
> >          while (src[len])
> >                  len++;
> > 
> >          return len;
> > }
> > 
> > Just let me know what you think.
> 
> This is one is very nice, thanks!
> 
> Sorry I didn't think about the size at all when writing the functions :)

Never be sorry, low-level user code like this is never trivial and
that's the goal of the nolibc-test in the first place ;-)

> We can change the loop to be:
> 
>         for (len = 0; len < size; len++) {
>                 dst[len] = src[len];
>                 if (!dst[len])
>                         break;
>         }
> 
> That IMHO it is slightly more readable and makes it only 2 bytes longer
> here.

It's not exactly the same, it will always write a zero at dst[size-1]
due to the break statement. As much as I hate returns in the middle,
this one made sense for this case. A goto to the final return statement
is fine as well.

> What do you think? I'm fine with both, of course.

I'm fine with the more readable part (I also prefer it) but not the use
of break here.

> If I resend, shall I add a suggested-by or directly you as the author?

No need for either, it's your work, my part was just a review and an
addictive temptation to look at asm code ;-)

Cheers,
Willy