linux-kernel - Re: [for-next][PATCH 23/24] string.h: Add strncmp

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: <20181223190544.6112f598@gandalf.local.home>
Date:   Sun, 23 Dec 2018 19:05:44 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:     Joe Perches <joe@...ches.com>,
        Andreas Schwab <schwab@...ux-m68k.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Tom Zanussi <zanussi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper
 macro

On Mon, 24 Dec 2018 00:52:13 +0100
Rasmus Villemoes <linux@...musvillemoes.dk> wrote:

> On 23/12/2018 23.56, Steven Rostedt wrote:
> > On Sun, 23 Dec 2018 23:01:52 +0100
> > Rasmus Villemoes <linux@...musvillemoes.dk> wrote:
> >   
> >> On 21/12/2018 23.20, Joe Perches wrote:  
> >>>
> >>> Using
> >>>
> >>> static inline bool str_has_prefix(const char *str, const char prefix[])
> >>> {
> >>> 	return !strncmp(str, prefix, strlen(prefix));
> >>> }
> >>>     
> >>
> >> We already have exactly that function, it's called strstarts().  
> > 
> > It's not exact.  
> 
> Huh? The str_has_prefix() I quoted is exactly strstarts().

The the implemented str_has_prefix() that you replied to is:

+/*
+ * A common way to test a prefix of a string is to do:
+ *  strncmp(str, 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 strncmp_prefix().
+ */
+#define strncmp_prefix(str, prefix)					\
+	({								\
+		int ____strcmp_prefix_ret____;				\
+		if (__builtin_constant_p(&prefix)) {			\
+			____strcmp_prefix_ret____ =			\
+				strncmp(str, prefix, sizeof(prefix) - 1); \
+		} else {						\
+			typeof(prefix) ____strcmp_prefix____ = prefix;	\
+			____strcmp_prefix_ret____ =			\
+				strncmp(str, ____strcmp_prefix____,	\
+					strlen(____strcmp_prefix____));	\
+		}							\
+		____strcmp_prefix_ret____;				\
+	})
+

Note, this has turned into a nice function:

 http://lkml.kernel.org/r/20181222162856.518489380@goodmis.org

+/**
+ * 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
+ *
+ * A common way to test a prefix of a string is to do:
+ *  strncmp(str, 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 str_has_prefix().
+ *
+ * Returns: 0 if @str does not start with @prefix
+         strlen(@prefix) if @str does 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;
+}
+


> 
> > 
> > Well, one thing that str_has_prefix() does that strstarts() does not,
> > is to return the prefix length which gets rid of the duplication.  
> 
> I hadn't seen the patches containing that version of str_has_prefix().
> Anyway, I just wanted to point out that strstarts() exists, so that we
> at least do not add a copy of that.

That's because you didn't read the patch that you quoted, just the
change log.

> 
> > Would it be OK to convert strstarts() to return the length of prefix?  
> 
> Dunno. By far, most users of the strncmp() idiom only seem to be
> interested in the boolean result. It's true that there are some that
> then want to skip the prefix and do further parsing, and I can see how
> avoiding duplicating the prefix length is useful. But the mathematician
> in me can't help consider the corner case of 'the empty string is always
> a prefix of any other string', and having str_has_prefix(str, "") be
> false seems wrong - obviously, nobody would ever use a literal "" there,
> but nothing in str_has_prefix() _requires_ the prefix to be a constant.

Which would be a useless use case. And if you define that it returns
the length of prefix on return, then it both matches and doesn't
match ;-)

> 
> Maybe 'bool str_skip_prefix(const char *s, const char *p, const char
> **out)' where *out is set to s+len on success, and set to s on failure
> (just to allow passing &s and continue parsing in elseifs)? That would
> make your 4/5 "tracing: Have the historgram use the result of
> str_has_prefix() for len of prefix" do
> 
>   if (str_skip_prefix(str, "onmatch(", &action_str)) {
> 
> hoisting the action_str declaration to the top, replacing the len variable?
> 

The use cases I've used in the final patch series uses the len for
indexing and other cases.

I think I'm keeping the str_has_prefix() and change the other users to
use it in the kernel. Most of the git grep strstarts() is tools
and scripts anyway.

-- Steve