linux-kernel - Re: [PATCH 4/4] tools/nolibc: add missing memchr() to string.h

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] [day] [month] [year] [list]

Message-ID: <20250623030132.GB29015@1wt.eu>
Date: Mon, 23 Jun 2025 05:01:32 +0200
From: Willy Tarreau <w@....eu>
To: Thomas Weißschuh <thomas@...ch.de>
Cc: Thomas Weißschuh <thomas.weissschuh@...utronix.de>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] tools/nolibc: add missing memchr() to string.h

On Sun, Jun 22, 2025 at 09:56:35PM +0200, Thomas Weißschuh wrote:
(...)
> > > > +#ifndef NOLIBC_ARCH_HAS_MEMCHR
> > > 
> > > So far we only have added these guards when necessary,
> > > which they aren't here. Can we drop them?
> > 
> > I intentionally placed them so that we can easily override them,
> > as we did for the other ones on x86 where string operations are
> > super short (repnz scasb is two bytes once you have the registers
> > already loaded).
> 
> Okay.
> 
> We do have different override mechanisms.
> Both NOLIBC_ARCH_HAS_* and for example the mechanism for sys_fork.
> Not sure if it is worth aligning them.

I don't know either, because we're speaking about doing it with standard
name functions (e.g. memchr), contrary to sys_* that we're bringing with
nolibc. I think it requires a bit more thinking to be sure we're not going
to cause trouble (e.g. with compiler builtin ones etc). At least whatever
the outcome, I agree that trying to align all definitions using the same
approach would be desirable, even if it means changing all of them.

> > > > +static __attribute__((unused))
> > > > +void *memchr(const void *s, int c, size_t len)
> > > > +{
> > > > +	char *p = (char *)s;
> > > 
> > > The docs say that they are interpreted as "unsigned char".
> > 
> > It does not change anything here, except adding an extra
> > modifier (since we'll then also have to do it in the loop
> > when comparing against c), thus IMHO it's extra noise.
> 
> Fair enough.
> 
> > > Also, can we keep the const?
> > 
> > It's memchr()'s definition which requires to return a void* so the
> > const needs to be dropped somewhere. Here I found visually cleaner to
> > have a single cast during the variable assignment rather than have a
> > second one on the return statement. But it's a matter of taste. I
> > tend to hate casts as they confuse the reader and remove the ability
> > of the compiler to produce relevant warnings, so for me the less the
> > better.
> 
> Ditto.

OK, then I'll push it.

Thank you!
Willy