[PATCH v5 0/7] Add strprefix() and fix a bug with it

[Alejandro Colomar <alx@kernel.org>]

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

Hi!

str_has_prefix() is based on sizes and lengths, which means it must
overload the valid length 0 to mean not found.  This is a typical
consequence of choosing to design an API around sizes and lengths, and
vanishes when moving to pointer-based similar APIs.

According to its author (Steven R.), str_has_prefix() was never meant to
accept an empty prefix, but that's not documented, and there's no
obvious reason why that would be acceptable for a generic string
function (as opposed to a function very specific to its original use).
Anyway, as suggested by Steven, I've added a WARN_ON_ONCE() to catch
such uses, and also added documentation stating this limitation.

Users of strings need a function whose behavior extends naturally to the
empty string, and we can do better, by returning a pointer.  And indeed,
users have ended up using str_has_prefix() with empty prefix, expecting
it to work (as is natural of a generic string API).

This patch set adds strprefix(), deprecates str_has_prefix(), and uses
strprefix() in one place to fix a bug reported by Bartosz.

We can later work on replacing the other uses of str_has_prefix(), which
might hide similar bugs.

This patch set also adds infrastructure to make string functions return
a pointer that is as qualified as the input pointer.

Changes in v5
~~~~~~~~~~~~~

-  [1/7]
   -  Fix .rst formatting.  [sashiko]
-  [2/7]
   -  Move the function to "lib/string.h" to be able to use
      WARN_ON_ONCE().  [Andy, Bart]
-  [3/7]  -- no changes --
-  [4/7]
   -  Simplify const_cast(), which makes it more robust.  'T:' didn't
      work, because in case const T is the same type as T, there's
      ambiguity in which branch should be taken, and the compiler
      rejects that.  We could go back to 'default:' (as in v3), but that
      is only needed for casting 'const char **' to 'char **'; since we
      don't need that at the moment, let's keep it simple, and we'll be
      able to add it in the future if we ever want that.
-  [5/7]  -- no changes --
-  [6/7]  -- no changes --
-  [7/7]
   -  New patch; use strprefix() to implement strstarts().

I didn't apply some changes requrested by reviewers:

-  I didn't move strprefix() to "lib/string.h" implementing it as a loop
   with manual byte operations.  The only reason for that seems to be
   performance, but I expect by being inline, the compiler should be
   able to completely drop the strlen(3) call in many cases, making the
   inline function faster.

See range-diff below.

v1: <https://lore.kernel.org/linux-hardening/cover.1793876837.git.alx@kernel.org/>
v2: <https://lore.kernel.org/linux-hardening/cover.1793882260.git.alx@kernel.org/>
v3: <https://lore.kernel.org/linux-hardening/cover.1794323750.git.alx@kernel.org/>
v4: <https://lore.kernel.org/linux-hardening/cover.1794425425.git.alx@kernel.org/>

Testing
~~~~~~~

(I've indented the output, to better see the commands.)

$ git log --oneline --graph v7.4-rc1^..strprefix5
	* 382a44f7c686 (HEAD -> strprefix, tag: strprefix5) string: Implement strstarts() with strprefix()
	* 0ca0b7aabcfc string: Make strprefix() const-qualifier-generic
	* 1294b3d64440 string: Add [C]QChar_of() and QVoid_of()
	* b615b5afb05e stddef: Add const_cast()
	* 49252e78c9b6 device property: Fix fwnode_name_eq() for empty node names
	* df3a35c469c6 string: Warn if the prefix is the empty string
	* f25bf43d7f18 string: Add strprefix(), and deprecate str_has_prefix()
	* 6628e7d4c6b2 (tag: v7.4-rc1, master) Linux 7.4-rc1
$ cat .kunitconfig
	CONFIG_KUNIT=y
	CONFIG_STRING_KUNIT_TEST=y
$ mkdir -p ~/tmp/strprefix5
$ export TMPDIR=$HOME/tmp/strprefix5/
$ ./tools/testing/kunit/kunit.py run "string*"
	[13:27:47] Configuring KUnit Kernel ...
	[13:27:47] Building KUnit Kernel ...
	Populating config with:
	$ make ARCH=um O=.kunit olddefconfig
	Building with:
	$ make all compile_commands.json scripts_gdb ARCH=um O=.kunit --jobs=24
	[13:28:02] Starting KUnit Kernel (1/1)...
	[13:28:02] ============================================================
	Running tests with:
	$ .kunit/linux 'kunit.filter_glob=string*' kunit.enable=1 mem=1G console=tty kunit_shutdown=halt
	[13:28:02] ============= string-stream-test (12 subtests) =============
	[13:28:02] [PASSED] string_stream_managed_init_test
	[13:28:02] [PASSED] string_stream_unmanaged_init_test
	[13:28:02] [PASSED] string_stream_managed_free_test
	[13:28:02] [PASSED] string_stream_resource_free_test
	[13:28:02] [PASSED] string_stream_line_add_test
	[13:28:02] [PASSED] string_stream_variable_length_line_test
	[13:28:02] [PASSED] string_stream_append_test
	[13:28:02] [PASSED] string_stream_append_auto_newline_test
	[13:28:02] [PASSED] string_stream_append_empty_string_test
	[13:28:02] [PASSED] string_stream_no_auto_newline_test
	[13:28:02] [PASSED] string_stream_auto_newline_test
	[13:28:02] [PASSED] string_stream_performance_test
	[13:28:02] =============== [PASSED] string-stream-test ================
	[13:28:02] =================== string (29 subtests) ===================
	[13:28:02] [PASSED] string_test_memset16
	[13:28:02] [PASSED] string_test_memset32
	[13:28:02] [PASSED] string_test_memset64
	[13:28:02] [PASSED] string_test_strlen
	[13:28:02] [PASSED] string_test_strnlen
	[13:28:02] [PASSED] string_test_strchr
	[13:28:02] [PASSED] string_test_strnchr
	[13:28:02] [PASSED] string_test_strrchr
	[13:28:02] [PASSED] string_test_strspn
	[13:28:02] [PASSED] string_test_strprefix
	[13:28:02] [PASSED] string_test_strcmp
	[13:28:02] [PASSED] string_test_strcmp_long_strings
	[13:28:02] [PASSED] string_test_strncmp
	[13:28:02] [PASSED] string_test_strncmp_long_strings
	[13:28:02] [PASSED] string_test_strcasecmp
	[13:28:02] [PASSED] string_test_strcasecmp_long_strings
	[13:28:02] [PASSED] string_test_strncasecmp
	[13:28:02] [PASSED] string_test_strncasecmp_long_strings
	[13:28:02] [PASSED] string_test_strscpy
	[13:28:02] [PASSED] string_test_strcat
	[13:28:02] [PASSED] string_test_strncat
	[13:28:02] [PASSED] string_test_strlcat
	[13:28:02] [PASSED] string_test_strtomem
	[13:28:02] [PASSED] string_test_memtostr
	[13:28:02] [PASSED] string_test_strends
	[13:28:02] [SKIPPED] string_bench_strlen (not enabled)
	[13:28:02] [SKIPPED] string_bench_strnlen (not enabled)
	[13:28:02] [SKIPPED] string_bench_strchr (not enabled)
	[13:28:02] [SKIPPED] string_bench_strrchr (not enabled)
	[13:28:02] ===================== [PASSED] string ======================
	[13:28:02] =============== string_helpers (4 subtests) ================
	[13:28:02] [PASSED] test_get_size
	[13:28:02] [PASSED] test_upper_lower
	[13:28:02] [PASSED] test_unescape
	[13:28:02] [PASSED] test_escape
	[13:28:02] ================= [PASSED] string_helpers ==================
	[13:28:02] ============================================================
	[13:28:02] Testing complete. Ran 45 tests: passed: 41, skipped: 4
	[13:28:02] Elapsed time: 15.583s total, 0.000s configuring, 15.366s building, 0.172s running

$ make allmodconfig
	  HOSTCC  scripts/basic/fixdep
	  HOSTCC  scripts/kconfig/conf.o
	  HOSTCC  scripts/kconfig/confdata.o
	  HOSTCC  scripts/kconfig/expr.o
	  LEX     scripts/kconfig/lexer.lex.c
	  YACC    scripts/kconfig/parser.tab.[ch]
	  HOSTCC  scripts/kconfig/lexer.lex.o
	  HOSTCC  scripts/kconfig/menu.o
	  HOSTCC  scripts/kconfig/parser.tab.o
	  HOSTCC  scripts/kconfig/preprocess.o
	  HOSTCC  scripts/kconfig/symbol.o
	  HOSTCC  scripts/kconfig/util.o
	  HOSTLD  scripts/kconfig/conf
	#
	# configuration written to .config
	#
$ time make -j24 | ts -s
	00:00:01   GEN     arch/x86/include/generated/asm/orc_hash.h
	00:00:01   UPD     arch/x86/include/generated/uapi/asm/bpf_perf_event.h
	00:00:01   UPD     arch/x86/include/generated/uapi/asm/errno.h
...
	00:21:49   LD [M]  net/qrtr/qrtr-tun.ko
	00:21:49   LD [M]  net/qrtr/qrtr-mhi.ko
	00:21:49   LD [M]  virt/lib/irqbypass.ko

	real	21m48.882s
	user	439m26.908s
	sys	25m20.132s


Have a lovely day!
Alex


Alejandro Colomar (7):
  string: Add strprefix(), and deprecate str_has_prefix()
  string: Warn if the prefix is the empty string
  device property: Fix fwnode_name_eq() for empty node names
  stddef: Add const_cast()
  string: Add [C]QChar_of() and QVoid_of()
  string: Make strprefix() const-qualifier-generic
  string: Implement strstarts() with strprefix()

 drivers/base/property.c  |  5 +---
 include/linux/stddef.h   | 11 +++++++++
 include/linux/string.h   | 52 ++++++++++++++++++++++++++++++++--------
 lib/string.c             | 24 +++++++++++++++++++
 lib/tests/string_kunit.c |  9 +++++++
 5 files changed, 87 insertions(+), 14 deletions(-)

Range-diff against v4:
1:  a74fabd985a8 ! 1:  f25bf43d7f18 string: Add strprefix(), and deprecate str_has_prefix()
    @@ include/linux/string.h: void memcpy_and_pad(void *dest, size_t dest_len, const v
     + * and not a constant.  Instead use strprefix().
     + *
     + * Returns:
    -+ * *  @s+strlen(@prefix) if @s starts with @prefix
    ++ * *  @s + strlen(@prefix) if @s starts with @prefix
     + * *  NULL if @s does not start with @prefix
     + */
     +static inline char *strprefix(const char *s, const char *prefix)
2:  cd09427148a0 ! 2:  df3a35c469c6 string: Warn if the prefix is the empty string
    @@ Commit message
         Warn if it happens.  Also, document that if such corner case
         is needed, strprefix() is preferable.
     
    +    We need to move str_has_prefix() to "lib/string.h" to be able to use
    +    WARN_ON_ONCE().
    +
         Reported-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
         Link: https://lore.kernel.org/linux-hardening/20261104-fwnode-for-each-prop-v1-1-6e28fe1c72c4@oss.qualcomm.com/
         Suggested-by: Steven Rostedt <rostedt@goodmis.org>
    +    Cc: Andy Shevchenko <andriy.shevchenko@intel.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## include/linux/string.h ##
    +@@ include/linux/string.h: extern char * strstr(const char *, const char *);
    + #ifndef __HAVE_ARCH_STRNSTR
    + extern char * strnstr(const char *, const char *, size_t);
    + #endif
    ++extern size_t str_has_prefix(const char *str, const char *prefix);
    + #ifndef __HAVE_ARCH_STRLEN
    + extern __kernel_size_t strlen(const char *);
    + #endif
     @@ include/linux/string.h: static inline char *strprefix(const char *s, const char *prefix)
    -  * This is similar to strprefix(), but returns an offset instead of
    -  * a pointer.
    -  *
    + 	return strncmp(s, prefix, len) == 0 ? (char *) s + len : NULL;
    + }
    + 
    +-/**
    +- * str_has_prefix - Test if a string has a given prefix
    +- * @str: The string to test
    +- * @prefix: The string to see if @str starts with
    +- *
    +- * This is similar to strprefix(), but returns an offset instead of
    +- * a pointer.
    +- *
    +- * Returns:
    +- * * strlen(@prefix) if @str starts with @prefix
    +- * * 0 if @str does not start with @prefix
    +- */
    +-static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
    +-{
    +-	size_t len = strlen(prefix);
    +-	return strncmp(str, prefix, len) == 0 ? len : 0;
    +-}
    +-
    + /**
    +  * strstarts - does @str start with @prefix?
    +  * @str: string to examine
    +
    + ## lib/string.c ##
    +@@ lib/string.c: char *strnstr(const char *s1, const char *s2, size_t len)
    + EXPORT_SYMBOL(strnstr);
    + #endif
    + 
    ++/**
    ++ * str_has_prefix - Test if a string has a given prefix
    ++ * @str: The string to test
    ++ * @prefix: The string to see if @str starts with
    ++ *
    ++ * This is similar to strprefix(), but returns an offset instead of
    ++ * a pointer.
    ++ *
     + * Unlike for strprefix(), if the prefix is an empty string, the
     + * behavior is undefined.  If you need to support arbitrary prefixes,
     + * use strprefix().
     + *
    -  * Returns:
    -  * * strlen(@prefix) if @str starts with @prefix
    -  * * 0 if @str does not start with @prefix
    -@@ include/linux/string.h: static inline char *strprefix(const char *s, const char *prefix)
    - static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
    - {
    - 	size_t len = strlen(prefix);
    ++ * Returns:
    ++ * * strlen(@prefix) if @str starts with @prefix
    ++ * * 0 if @str does not start with @prefix
    ++ */
    ++size_t str_has_prefix(const char *str, const char *prefix)
    ++{
    ++	size_t len = strlen(prefix);
     +	WARN_ON_ONCE(len == 0);
    - 	return strncmp(str, prefix, len) == 0 ? len : 0;
    - }
    - 
    ++	return strncmp(str, prefix, len) == 0 ? len : 0;
    ++}
    ++EXPORT_SYMBOL(str_has_prefix);
    ++
    + #ifndef __HAVE_ARCH_MEMCHR
    + /**
    +  * memchr - Find a character in an area of memory.
3:  0058b60f662c = 3:  49252e78c9b6 device property: Fix fwnode_name_eq() for empty node names
4:  ce623fe0432b ! 4:  b615b5afb05e stddef: Add const_cast()
    @@ Commit message
         It's similar to C++'s const_cast(), but it is more restrictive: it can
         only cast away 'const', and not arbitrary qualifiers.
     
    -    David made it a bit more robust.
    -
    -    Link: https://lore.kernel.org/linux-hardening/20261111102535.434c2eee@pumpkin/
    -    Co-developed-by: David Laight <david.laight.linux@gmail.com>
    +    Cc: David Laight <david.laight.linux@gmail.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## include/linux/stddef.h ##
    @@ include/linux/stddef.h: enum {
     + * @T: Type for the cast.
     + * @p: Pointer to be casted.
     + *
    -+ * If @p is of type const @T, then it is casted to @T.
    -+ * If @p is of type @T, then it is returned without any casts.
    ++ * Cast @p to type @T.  The macro verifies that @p is of type const @T.
     + * This avoids mistakes that change anything other than 'const' in a cast.
     + */
    -+#define const_cast(T, p)  _Generic(p, const T: (T) (p), T: (p))
    ++#define const_cast(T, p)  _Generic(p, const T: (T) (p))
     +
      #endif
5:  206d2045abcf = 5:  1294b3d64440 string: Add [C]QChar_of() and QVoid_of()
6:  69d1792e5edf ! 6:  0ca0b7aabcfc string: Make strprefix() const-qualifier-generic
    @@ Commit message
     
      ## include/linux/string.h ##
     @@ include/linux/string.h: void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
    -  * *  @s+strlen(@prefix) if @s starts with @prefix
    +  * *  @s + strlen(@prefix) if @s starts with @prefix
       * *  NULL if @s does not start with @prefix
       */
     -static inline char *strprefix(const char *s, const char *prefix)
    @@ include/linux/string.h: void memcpy_and_pad(void *dest, size_t dest_len, const v
     +#define strprefix(s, pfx)  const_cast(CQChar_of(s) *, strprefix(s, pfx))
      
      /**
    -  * str_has_prefix - Test if a string has a given prefix
    +  * strstarts - does @str start with @prefix?
-:  ------------ > 7:  382a44f7c686 string: Implement strstarts() with strprefix()
-- 
2.55.0


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