Re: [PATCH v2 1/2] string: Add strprefix(), and deprecate str_has_prefix()

[Alejandro Colomar <alx+linux-hardening@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4792 bytes --]

Hi Geert,

> Date: 2026-11-05 13:51:54+0100
> From: Geert Uytterhoeven <geert@linux-m68k.org>
>
> 	Hi Alejandro,
> 
> On Thu, 5 Nov 2026 13:46:29, Alejandro Colomar wrote:
> > As Bartosz reported, str_has_prefix() has a bug: it can't differentiate
> > between an empty prefix matching an empty string from a non-matching
> > prefix.  This is a fundamental design flaw in this API, and not
> > something that can be fixed.
> > 
> > As always, this is a consequence of choosing sizes instead of pointers.
> > Sizes are often the wrong choice.  In this specific case, pointers are
> > more capable of differentiating these cases, as NULL is a special
> > sentinel value that doesn't need to overload a valid value.
> > 
> > Let's implement strprefix(), a function that returns a pointer
> > right after the prefix if found, and a null pointer if not found.
> > 
> > We'll slowly migrate existing code from str_has_prefix() to strprefix().
> > 
> > Reported-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> > Cc: Andy Shevchenko <andy@kernel.org>
> > Signed-off-by: Alejandro Colomar <alx@kernel.org>
> 
> Thanks for your patch!

:-)

> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -545,16 +545,33 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
> >  	       sizeof(*(obj)) - offsetof(typeof(*(obj)), member));	\
> >  })
> >  
> > +/**
> > + * strprefix - string prefix
> 
> Naive me assumes this function must return the prefix of the string,
> so IMO is it named badly.

To return the prefix, it'd have to return the second parameter, but the
called already has that, so I expect users would be smart enough to not
expect that.

One could expect that it maybe returns the prefix within the string, but
that would mean returning the string itself, since the prefix must
necessarily start in the first byte.  So, I also expect users to be
smart enough to not expect that.

A plausible name to avoid the pathological interpretations of the name
could be strafterprefix(), but I tend to dislike the unnecessary 'after'
in the name, perhaps because I think the interpretations above are
pathologically wrong.

Do you think strafterprefix() would be better?  Do you have any other
suggestions?

I would prefer keeping the name strprefix(), and perhaps improving
documentation.

> > + * @s: The string to be tested
> > + * @prefix: The string to see if @s starts with
> > + *
> > + * Returns:
> > + * *  @s+strlen(@prefix) if @s starts with @prefix
> > + * *  NULL if @s does not start with @prefix
> > + */
> > +static __always_inline char *strprefix(const char *s, const char *prefix)
> 
> This returns a non-const substring of the original const parameter, thus
> allowing the caller to modify the original even when it is const.
> 
> While this is not uncommon behavior for existing standard string
> helpers, I think it would be good to make new functions safer, by not
> allowing to drop constness.  Using _Generic() can be used to support
> both const and non-const strings in a safe way.

Agree.  That's in fact what we do in shadow-utils.  I didn't do it here,
because I didn't know if that kind of magic would be appreciated here.

Here's what we do in shadow-utils:

	#define strprefix(s, prefix)                                  \
	(                                                             \
		const_cast(CQChar_of(s) *, strprefix_(s, prefix))     \
	)

	#define const_cast(T, p)  _Generic(p, const T: (T) (p), default: (p))

	#define CQChar_of(s)    QChar_of(const, s)
	#define QChar_of(Q, s)  typeof                                \
	(                                                             \
		_Generic((QVoid_of(s) *){0},                          \
			Q void *:  (Q char){0},                       \
			void *:    (char){0}                          \
		)                                                     \
	)
	#define QVoid_of(p)     typeof((QVoidptrof_(p)){0}[0])
	#define QVoidptrof_(p)  typeof(1?(p):(void*){0})

	const char *
	strprefix_(const char *s, const char *prefix)
	{
		if (strncmp(s, prefix, strlen(prefix)) != 0)
			return NULL;

		return s + strlen(prefix);
	}

I could do the same here.  Do we want a const_cast() macro?  Or do we
want an open-coded cast?  We use const_cast() in shadow-utils to prevent
accidents in casts that may do very bad stuff.  const_cast() makes sure
we only drop (or add) const at most.

Also, do we want to add the old-style strprefix() now and improve the
const-correctness in a separate patch set?  Or do we want to do all at
once?  I don't mind either way.


Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]