Re: [PATCH v6 02/19] string: Add strprefix()

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

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

Hi Joe,

> Date: 2026-11-14 07:53:25-0800
> From: Joe Perches <joe@perches.com>
>
[...]
> > +/**
> > + * strprefix - string prefix
> > + * @s: The string to be tested
> > + * @prefix: The string to see if @s starts with
> > + *
> > + * A common way to test a prefix of a string is to do:
> > + *  strncmp(s, prefix, sizeof(prefix) - 1)
> > + *
> > + * But this can lead to bugs due to typos, or if prefix is a pointer
> > + * and not a constant.  Instead use strprefix().
> > + *
> > + * Returns:
> > + * *  @s + strlen(@prefix) if @s starts with @prefix
> > + * *  NULL if @s does not start with @prefix
> > + */
> > +__attribute__((nonnull(1, 2)))
> 
> __nonnull_args(1, 2)

Thanks!

> A negative of this is that it implies the return should
> always be tested even though it can be known that the
> return is effectively bool.

I'm not sure I follow.  The attribute is about the arguments, not the
return value.  Perhaps I don't understand what you mean by "this".

> 
> Also the block
> 
> > + * A common way to test a prefix of a string is to do:
> > + *  strncmp(s, prefix, sizeof(prefix) - 1)
> 
> IMO isn't useful

I tend to agree.  I think that is just one example of code this API
replaces, but there are many more.  I think I'd prefer a description of
the API, without comparing it to existing patterns (because there might
be too many).

> and the description doesn't say what happens
> when prefix is an empty string (which IMO should match).

It doesn't, because the general behavior naturally extends to that
extreme.  That is, it's the non-surprising result.

For any non-empty prefix, the function succeeds if all the non-null
characters from the prefix are found sequentially at the begining of the
string.  In other words, it matches if and only if strstr(s,prefix)==s.

For the empty prefix, it still behaves like that.  If all the non-null
characters from the prefix are found sequentially at the begining of the
string, it succeeds.  This always happens, of course, so strprefix(s,"")
always succeeds.  Similarly, strstr(s,"")==s.

Since there's nothing different, I don't see a strong reason to document
it at all.  And that's also the reason why I think the issue in
str_has_prefix() is not just a different design decision, but rather
a bug.  If the name of str_has_prefix() had been strprefix_len() --which
BTW, I'm tempted to rename in this patch set--, then it would be just a
design decision, as it would behave correctly for what the name implies,
and it would be an incorrect use if one tries to use it for determining
whether a strings starts with a possibly-empty prefix or not.  But the
function is called str_has_prefix(), which implies a boolean behavior,
and that boolean behavior is wrong for the case of the empty string.
That is, while any string starts with an empty string, str_has_prefix()
returns false, and that is wrong.

> And the name strprefix could be confused with a function
> that actually prefixes a string to another string.

This would be a stretch.  <string.h> functions, with the exception of
the *dup() functions, do not allocate memory.  They're a collection of
functions that only perform byte operations on strings, or in general,
on character arrays.

Prefixing a string to another string would require either knowing the
size of the buffer (and thus have a size_t parameter), or allocate new
memory.  Since this function doesn't have a size_t, the former can be
ruled out.  And allocating memory would be very weird, so we can also
discard that.

> maybe strprefixed or str_is_prefixed ?

strprefixed() might be okay-ish.  I still prefer strprefix(), because
common sense can rule those interpretations out, and also because it's
widely implemented with that name.

> And these are all candidates for converted uses to
> whatever is the function name or steq:
> 
> $ git grep -P '[^w]strncmp\s*\(\s*[\w\.\-\>\s]*,\s*\"\w*"\s*,\s*\d+\s*\)' -- '*.[ch]' | wc -l
> 946

Yup, there's a lot of code that could be converted.  For now, I find it
more pressing to transform the existing calls to strstarts(), and as
many of str_has_prefix() as reasonable, to minimize drift, but I can
also work later on converting the strncmp(3) calls.

> [^w] because of prom_strncmp

Since there's no anchor before the 's', that doesn't change anything:

$ git grep -P '[^w]strncmp\s*\(\s*[\w\.\-\>\s]*,\s*\"\w*"\s*,\s*\d+\s*\)' -- '*.[ch]' | wc -l
946
$ git grep -P 'strncmp\s*\(\s*[\w\.\-\>\s]*,\s*\"\w*"\s*,\s*\d+\s*\)' -- '*.[ch]' | wc -l
946

> 

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

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