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
| ||
|
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
Powered by blists - more mailing lists