From: Steven Rostedt <[email protected]>
A discussion came up in the trace triggers thread about converting a
bunch of:
strncmp(str, "const", sizeof("const") - 1)
use cases into a helper macro. It started with:
strncmp(str, const, sizeof(const) - 1)
But then Joe Perches mentioned that if a const is not used, the
sizeof() will be the size of a pointer, which can be bad. And that
gcc will optimize strlen("const") into "sizeof("const") - 1".
Thinking about this more, a quick grep in the kernel tree found several
(thousands!) of cases that use this construct. A quick grep also
revealed that there's probably several bugs in that use case. Some are
that people forgot the "- 1" (which I found) and others could be that
the constant for the sizeof is different than the constant (although, I
haven't found any of those, but I also didn't look hard).
I figured the best thing to do is to create a helper macro and place it
into include/linux/string.h. And go around and fix all the open coded
versions of it later.
I plan on only applying this patch and updating the tracing hooks for
this merge window. And perhaps use it to fix some of the bugs that were
found.
I was going to just use:
strncmp(str, prefix, strlen(prefix))
but then realized that "prefix" is used twice, and will break if
someone does something like:
strncmp_prefix(str, ptr++);
So instead I check with __builtin_constant_p() to see if the second
parameter is truly a constant, which I use sizeof() anyway (why bother
gcc to optimize it, if we already know it's a constant), and copy the
parameter into a local variable and use that local variable for the non
constant part (with strlen).
Link: http://lkml.kernel.org/r/e3e754f2bd18e56eaa8baf79bee619316ebf4cfc.1545161087.git.tom.zanussi@linux.intel.com
Link: http://lkml.kernel.org/r/[email protected]
Cc: Joe Perches <[email protected]>
Cc: Tom Zanussi <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Steven Rostedt (VMware) <[email protected]>
---
include/linux/string.h | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h
index 27d0482e5e05..a998a53333ef 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -14,6 +14,28 @@ extern void *memdup_user(const void __user *, size_t);
extern void *vmemdup_user(const void __user *, size_t);
extern void *memdup_user_nul(const void __user *, size_t);
+/*
+ * 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____; \
+ })
+
/*
* Include machine specific inline routines
*/
--
2.19.2
On Fri, Dec 21, 2018 at 9:57 AM Steven Rostedt <[email protected]> wrote:
>
> I figured the best thing to do is to create a helper macro and place it
> into include/linux/string.h. And go around and fix all the open coded
> versions of it later.
>
> I plan on only applying this patch and updating the tracing hooks for
> this merge window. And perhaps use it to fix some of the bugs that were
> found.
I like the helper function concept, but as they say about CS: "There
is only one hard problem in computer science: naming and off-by-one
errors".
And this one has that problem. The name makes absolutely no sense.
Calling it "strncmp_prefix()" when there is no "n" there makes no sense.
So drop the "n" from the name.
And honestly, maybe it would be better to use a different naming
pattern entirely, and avoid the crazy non-boolean "str*cmp()" model.
That thing is useful for search comparisons (where "before/after"
matters), but it's horrible for the actual "is this true or not",
where the code ends up being
if (!strcmp_prefix(a, "prefix")) {
// This is the "Yes, prefix matched" case, despite the
"if !" syntax
which is just confusing.
So I'd really prefer more of a
#define has_prefix(str, prefix) ...
model that actually returns a boolean (true if it has a prefix, false
if it doesn't), rather than use the "str*cmp" naming and return
values.
(But I agree that *if* you use the "strcmp" naming, then you do need
to hold to the traditional strcmp return value semantics).
Hmm?
Finally, I also suspect that your helper might be slightly fragile.
Doing sizeof() seems broken. I could see somebody using some prefix
define for arrays with constant sizes, and doing
#define PFX1 "cpp\0"
#define PFX2 "c\0\0\0"
#define PFX3 "h\0\0\0"
or similar. Also, is there a reason you use "&prefix" for the constant
test? I don't see that pattern anywhere else. Plus it looks entirely
wrong without the parenthesis (ie "&(prefix)" to make sure we group
things right).
So a lot of small problems, but the naming one is big.
Linus
On Fri, 21 Dec 2018 12:01:48 -0800
Linus Torvalds <[email protected]> wrote:
> On Fri, Dec 21, 2018 at 11:40 AM Steven Rostedt <[email protected]> wrote:
> >
> > OK, what about if we just use strlen() and say that this macro is not
> > safe for parameters with side effects.
>
> I think gcc follows simple assignments just fine, and does the
> optimized strlen() for them too.
>
> So why not just do
>
> #define have_prefix(str,prefix) ({ \
> const char *__pfx = prefix; \
> size_t __pfxlen = strlen(__pfx); \
> strncmp(str, __pfx, __pfxlen) ? 0 : __pfxlen); })
>
> and be done with it safely?
>
> The above is ENTIRELY untested.
>
At first I thought this would have issues, but with a slight change...
#define have_prefix(str, prefix) ({ \
const char *__pfx = (const char *)prefix; \
And the rest the same, it appears to work.
Need the cast because if for some reason someone passed in something
like "const unsigned char" then it wouldn't work. But that's just a nit.
So something like this then?
-- Steve
diff --git a/include/linux/string.h b/include/linux/string.h
index 27d0482e5e05..4586fee60194 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -14,6 +14,28 @@ extern void *memdup_user(const void __user *, size_t);
extern void *vmemdup_user(const void __user *, size_t);
extern void *memdup_user_nul(const void __user *, size_t);
+/**
+ * have_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 has_prefix().
+ *
+ * Returns: 0 if @str does not start with @prefix
+ strlen(@prefix) if @str does start with @prefix
+ */
+#define has_prefix(str, prefix) \
+ ({ \
+ const char *____prefix____ = (const char *)(prefix); \
+ int ____len____ = strlen(____prefix____); \
+ strncmp(str, ____prefix____, ____len____) == 0 ? \
+ ____len____ : 0; \
+ })
+
/*
* Include machine specific inline routines
*/
On Fri, 2018-12-21 at 15:35 -0500, Steven Rostedt wrote:
> At first I thought this would have issues, but with a slight change...
>
> #define have_prefix(str, prefix) ({ \
> const char *__pfx = (const char *)prefix; \
>
>
> And the rest the same, it appears to work.
>
> Need the cast because if for some reason someone passed in something
> like "const unsigned char" then it wouldn't work. But that's just a nit.
>
> So something like this then?
>
> -- Steve
>
> diff --git a/include/linux/string.h b/include/linux/string.h
[]
> @@ -14,6 +14,28 @@ extern void *memdup_user(const void __user *, size_t);
> extern void *vmemdup_user(const void __user *, size_t);
> extern void *memdup_user_nul(const void __user *, size_t);
>
> +/**
> + * have_prefix - Test if a string has a given prefix
There is a naming mismatch of have/has here
and has_prefix is probably too generic a name.
How about str_has_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 has_prefix().
> + *
> + * Returns: 0 if @str does not start with @prefix
> + strlen(@prefix) if @str does start with @prefix
> + */
> +#define has_prefix(str, prefix) \
> + ({ \
> + const char *____prefix____ = (const char *)(prefix); \
> + int ____len____ = strlen(____prefix____); \
> + strncmp(str, ____prefix____, ____len____) == 0 ? \
> + ____len____ : 0; \
> + })
I think all the underscores are unnecessary and confusing.
On Fri, 21 Dec 2018 12:46:47 -0800
Joe Perches <[email protected]> wrote:
> > @@ -14,6 +14,28 @@ extern void *memdup_user(const void __user *, size_t);
> > extern void *vmemdup_user(const void __user *, size_t);
> > extern void *memdup_user_nul(const void __user *, size_t);
> >
> > +/**
> > + * have_prefix - Test if a string has a given prefix
>
> There is a naming mismatch of have/has here
> and has_prefix is probably too generic a name.
>
> How about str_has_prefix?
Sure.
>
> > + * @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 has_prefix().
> > + *
> > + * Returns: 0 if @str does not start with @prefix
> > + strlen(@prefix) if @str does start with @prefix
> > + */
> > +#define has_prefix(str, prefix) \
> > + ({ \
> > + const char *____prefix____ = (const char *)(prefix); \
> > + int ____len____ = strlen(____prefix____); \
> > + strncmp(str, ____prefix____, ____len____) == 0 ? \
> > + ____len____ : 0; \
> > + })
>
> I think all the underscores are unnecessary and confusing.
>
Well, perhaps I can just remove the ending ones. I get paranoid with
macro variables, and tend to over do it so that there's no question.
I don't find them confusing ;-) But I tend to use a lot of macros.
-- Steve
On Dez 21 2018, Steven Rostedt <[email protected]> wrote:
> On Fri, 21 Dec 2018 12:46:47 -0800
> Joe Perches <[email protected]> wrote:
>
>> > + * @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 has_prefix().
>> > + *
>> > + * Returns: 0 if @str does not start with @prefix
>> > + strlen(@prefix) if @str does start with @prefix
>> > + */
>> > +#define has_prefix(str, prefix) \
>> > + ({ \
>> > + const char *____prefix____ = (const char *)(prefix); \
>> > + int ____len____ = strlen(____prefix____); \
>> > + strncmp(str, ____prefix____, ____len____) == 0 ? \
>> > + ____len____ : 0; \
>> > + })
>>
>> I think all the underscores are unnecessary and confusing.
>>
>
> Well, perhaps I can just remove the ending ones. I get paranoid with
> macro variables, and tend to over do it so that there's no question.
Why not make it an inline function?
Andreas.
--
Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
On Fri, 21 Dec 2018 21:58:32 +0100
Andreas Schwab <[email protected]> wrote:
> > Well, perhaps I can just remove the ending ones. I get paranoid with
> > macro variables, and tend to over do it so that there's no question.
>
> Why not make it an inline function?
Matters if that removes the strlen(const) optimization. I could try it
and see what happens.
-- Steve
On Fri, 21 Dec 2018 10:51:29 -0800
Linus Torvalds <[email protected]> wrote:
> On Fri, Dec 21, 2018 at 9:57 AM Steven Rostedt <[email protected]> wrote:
> >
> > I figured the best thing to do is to create a helper macro and place it
> > into include/linux/string.h. And go around and fix all the open coded
> > versions of it later.
> >
> > I plan on only applying this patch and updating the tracing hooks for
> > this merge window. And perhaps use it to fix some of the bugs that were
> > found.
>
> I like the helper function concept, but as they say about CS: "There
> is only one hard problem in computer science: naming and off-by-one
> errors".
>
> And this one has that problem. The name makes absolutely no sense.
Good thing I rebased and put these patches at the end before running
my tests :-) (Specifically because I was expecting to have feedback like
this).
>
> Calling it "strncmp_prefix()" when there is no "n" there makes no sense.
>
> So drop the "n" from the name.
I originally did that, but then I thought strcmp() is a full compare,
and 'n' denotes it is partial. But thinking about it more, yeah one
would think it would need a parameter to represent 'n'.
>
> And honestly, maybe it would be better to use a different naming
> pattern entirely, and avoid the crazy non-boolean "str*cmp()" model.
> That thing is useful for search comparisons (where "before/after"
> matters), but it's horrible for the actual "is this true or not",
> where the code ends up being
>
> if (!strcmp_prefix(a, "prefix")) {
> // This is the "Yes, prefix matched" case, despite the
> "if !" syntax
>
> which is just confusing.
>
> So I'd really prefer more of a
>
> #define has_prefix(str, prefix) ...
I like changing the name.
>
> model that actually returns a boolean (true if it has a prefix, false
> if it doesn't), rather than use the "str*cmp" naming and return
> values.
Actually, instead of returning a bool, it can return the length if it
matches.
Reason being, there's several locations that does something like:
while (...) {
len = strlen("const");
if (strncmp(str, "const", len) == 0) {
str += len;
break;
}
}
And I was having trouble thinking about how to deal with these. But if
we have a has_prefix() that returns the length on match then we can do:
if ((len = has_prefix(str, "const")) {
str += len;
>
> (But I agree that *if* you use the "strcmp" naming, then you do need
> to hold to the traditional strcmp return value semantics).
>
> Hmm?
>
> Finally, I also suspect that your helper might be slightly fragile.
> Doing sizeof() seems broken. I could see somebody using some prefix
> define for arrays with constant sizes, and doing
>
> #define PFX1 "cpp\0"
> #define PFX2 "c\0\0\0"
> #define PFX3 "h\0\0\0"
>
> or similar. Also, is there a reason you use "&prefix" for the constant
That was left over in my original tests in userspace. When I first
tried it with __builtin_constant_p() I got an error, and added the '&',
but then fixed something else. The something else was what actually
caused the error, but since it didn't complain (and past all my tests),
I left in the '&'.
> test? I don't see that pattern anywhere else. Plus it looks entirely
> wrong without the parenthesis (ie "&(prefix)" to make sure we group
> things right).
>
> So a lot of small problems, but the naming one is big.
OK, what about if we just use strlen() and say that this macro is not
safe for parameters with side effects.
-- Steve
diff --git a/include/linux/string.h b/include/linux/string.h
index 27d0482e5e05..f9d274a81276 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -14,6 +14,29 @@ extern void *memdup_user(const void __user *, size_t);
extern void *vmemdup_user(const void __user *, size_t);
extern void *memdup_user_nul(const void __user *, size_t);
+/**
+ * have_prefix - Test if a string has a given prefix
+ * @str: The string to test
+ * @prefix: The string to see if @str starts with
+ *
+ * IMPORTANT; @prefix must not have side effects (used more than once here)
+ *
+ * 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 has_prefix().
+ *
+ * Returns: 0 if @str does not start with @prefix
+ strlen(@prefix) if @str does start with @prefix
+ */
+#define has_prefix(str, prefix) \
+ ({ \
+ int ____strcmp_prefix_len____ = strlen(prefix); \
+ strncmp(str, prefix, ____strcmp_prefix_len____) == 0 ? \
+ ____strcmp_prefix_len____ : 0; \
+ })
+
/*
* Include machine specific inline routines
*/
On Fri, Dec 21, 2018 at 11:40 AM Steven Rostedt <[email protected]> wrote:
>
> OK, what about if we just use strlen() and say that this macro is not
> safe for parameters with side effects.
I think gcc follows simple assignments just fine, and does the
optimized strlen() for them too.
So why not just do
#define have_prefix(str,prefix) ({ \
const char *__pfx = prefix; \
size_t __pfxlen = strlen(__pfx); \
strncmp(str, __pfx, __pfxlen) ? 0 : __pfxlen); })
and be done with it safely?
The above is ENTIRELY untested.
Linus
On Fri, 21 Dec 2018 12:41:17 -0800
Linus Torvalds <[email protected]> wrote:
> Parentheses....
?
-- Steve
>
> On Fri, Dec 21, 2018, 12:35 Steven Rostedt <[email protected] wrote:
>
> > On Fri, 21 Dec 2018 12:01:48 -0800
> > Linus Torvalds <[email protected]> wrote:
> >
> > > On Fri, Dec 21, 2018 at 11:40 AM Steven Rostedt <[email protected]>
> > wrote:
> > > >
> > > > OK, what about if we just use strlen() and say that this macro is not
> > > > safe for parameters with side effects.
> > >
> > > I think gcc follows simple assignments just fine, and does the
> > > optimized strlen() for them too.
> > >
> > > So why not just do
> > >
> > > #define have_prefix(str,prefix) ({ \
> > > const char *__pfx = prefix; \
> > > size_t __pfxlen = strlen(__pfx); \
> > > strncmp(str, __pfx, __pfxlen) ? 0 : __pfxlen); })
> > >
> > > and be done with it safely?
> > >
> > > The above is ENTIRELY untested.
> > >
> >
> > At first I thought this would have issues, but with a slight change...
> >
> > #define have_prefix(str, prefix) ({ \
> > const char *__pfx = (const char *)prefix; \
> >
> >
> > And the rest the same, it appears to work.
> >
> > Need the cast because if for some reason someone passed in something
> > like "const unsigned char" then it wouldn't work. But that's just a nit.
> >
> > So something like this then?
> >
> > -- Steve
> >
> > diff --git a/include/linux/string.h b/include/linux/string.h
> > index 27d0482e5e05..4586fee60194 100644
> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -14,6 +14,28 @@ extern void *memdup_user(const void __user *, size_t);
> > extern void *vmemdup_user(const void __user *, size_t);
> > extern void *memdup_user_nul(const void __user *, size_t);
> >
> > +/**
> > + * have_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 has_prefix().
> > + *
> > + * Returns: 0 if @str does not start with @prefix
> > + strlen(@prefix) if @str does start with @prefix
> > + */
> > +#define has_prefix(str, prefix)
> > \
> > + ({ \
> > + const char *____prefix____ = (const char *)(prefix); \
> > + int ____len____ = strlen(____prefix____); \
> > + strncmp(str, ____prefix____, ____len____) == 0 ? \
> > + ____len____ : 0; \
> > + })
> > +
> > /*
> > * Include machine specific inline routines
> > */
> >
On Fri, 21 Dec 2018 12:46:47 -0800
Joe Perches <[email protected]> wrote:
> I think all the underscores are unnecessary and confusing.
Here. I removed a beginning and ending underscore from each variable ;-)
-- Steve
diff --git a/include/linux/string.h b/include/linux/string.h
index 27d0482e5e05..7f88444471f7 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -14,6 +14,28 @@ extern void *memdup_user(const void __user *, size_t);
extern void *vmemdup_user(const void __user *, size_t);
extern void *memdup_user_nul(const void __user *, size_t);
+/**
+ * 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
+ */
+#define str_has_prefix(str, prefix) \
+ ({ \
+ const char *___prefix___ = (const char *)(prefix); \
+ int ___len___ = strlen(___prefix___); \
+ strncmp(str, ___prefix___, ___len___) == 0 ? \
+ ___len___ : 0; \
+ })
+
/*
* Include machine specific inline routines
*/
On Fri, Dec 21, 2018 at 12:55 PM Steven Rostedt <[email protected]> wrote:
>
> On Fri, 21 Dec 2018 12:41:17 -0800
> Linus Torvalds <[email protected]> wrote:
>
> > Parentheses....
>
> ?
Your patch actually had them, but in the body of your email you did
> #define have_prefix(str, prefix) ({ \
> const char *__pfx = (const char *)prefix; \
which is just completely wrong.
Considering your _old_ patch had the exact same bug, I really think
you need to start internalizing the whole "macro arguments *have* to
be properly protected" thing.
And I agree with Joe that using a million underscores just makes code
less legible. Two underscores at the beginning is the standard
namespace protection. Underscores at the end do nothing. And using
*more* than two is just crazy.
Finally, I think the cast is actually bogus and wrong. Why would the
prefix ever be anything but "const char *"? Adding the cast only makes
it more possible that somebody uses a truly wrong type ("unsigned long
*" ?), and then the cast just silently hides it.
If somebody uses "unsigned char *" for this, they'd get the exact same
warning if they were using strncmp and do this by hand.
So why would that helper function act any differently? Particularly
when it then has the huge downside that it will also accept absolute
garbage?
Anyway, that was a long and winding NAK for your patch.
Linus
On Fri, 2018-12-21 at 16:08 -0500, Steven Rostedt wrote:
> On Fri, 21 Dec 2018 21:58:32 +0100
> Andreas Schwab <[email protected]> wrote:
>
>
> > > Well, perhaps I can just remove the ending ones. I get paranoid with
> > > macro variables, and tend to over do it so that there's no question.
> >
> > Why not make it an inline function?
>
> Matters if that removes the strlen(const) optimization. I could try it
> and see what happens.
Using
static inline bool str_has_prefix(const char *str, const char prefix[])
{
return !strncmp(str, prefix, strlen(prefix));
}
eliminates the strlen with gcc 4.8 (oldest I still have)
$ cat lib/test_module.c
* This module emits "Hello, world" on printk when loaded.
*
* It is designed to be used for basic evaluation of the module loading
* subsystem (for example when validating module signing/verification). It
* lacks any extra dependencies, and will not normally be loaded by the
* system unless explicitly requested by name.
*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <linux/init.h>
#include <linux/module.h>
#include <linux/printk.h>
static inline bool str_has_prefix(const char *str, const char prefix[])
{
return !strncmp(str, prefix, strlen(prefix));
}
bool test_str_has_prefix(const char *foo)
{
return str_has_prefix("TomJonesPrefix", foo);
}
bool test_str_has_prefix_TomJones(void)
{
return str_has_prefix("TomJonesPrefix", "TomJones");
}
$ make CC=/usr/bin/gcc-4.8 lib/test_module.o
$ objdump -d -s lib/test_module.o
lib/test_module.o: file format elf64-x86-64
Contents of section .text:
0000 534889fb e8000000 00b90f00 00004883 SH............H.
0010 f80f4889 df480f4e c848c7c6 00000000 ..H..H.N.H......
0020 4839c9f3 a65b0f94 c0c3660f 1f440000 H9...[....f..D..
0030 b8010000 00c3 ......
Contents of section .rodata.str1.1:
0000 546f6d4a 6f6e6573 50726566 697800 TomJonesPrefix.
Contents of section .comment:
0000 00474343 3a202855 62756e74 7520342e .GCC: (Ubuntu 4.
0010 382e352d 34756275 6e747539 2920342e 8.5-4ubuntu9) 4.
0020 382e3500 8.5.
Contents of section .orc_unwind_ip:
0000 00000000 00000000 00000000 00000000 ................
0010 00000000 00000000 ........
Contents of section .orc_unwind:
0000 08000000 05001000 00000500 08000000 ................
0010 05000000 00000000 08000000 05000000 ................
0020 00000000 ....
Disassembly of section .text:
0000000000000000 <test_str_has_prefix>:
0: 53 push %rbx
1: 48 89 fb mov %rdi,%rbx
4: e8 00 00 00 00 callq 9 <test_str_has_prefix+0x9>
9: b9 0f 00 00 00 mov $0xf,%ecx
e: 48 83 f8 0f cmp $0xf,%rax
12: 48 89 df mov %rbx,%rdi
15: 48 0f 4e c8 cmovle %rax,%rcx
19: 48 c7 c6 00 00 00 00 mov $0x0,%rsi
20: 48 39 c9 cmp %rcx,%rcx
23: f3 a6 repz cmpsb %es:(%rdi),%ds:(%rsi)
25: 5b pop %rbx
26: 0f 94 c0 sete %al
29: c3 retq
2a: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1)
0000000000000030 <test_str_has_prefix_TomJones>:
30: b8 01 00 00 00 mov $0x1,%eax
35: c3 retq
On Fri, 21 Dec 2018 14:20:25 -0800
Joe Perches <[email protected]> wrote:
> static inline bool str_has_prefix(const char *str, const char prefix[])
> {
> return !strncmp(str, prefix, strlen(prefix));
> }
>
> eliminates the strlen with gcc 4.8 (oldest I still have)
I tested it a bit more before posting.
I tested it against:
gcc 4.5.1, 4.5.4, 4.6.3, 6.2.0, 7.2.0 and 8.2
And the strlen was eliminated each time.
So this looks like the right approach :-)
-- Steve
On Fri, Dec 21, 2018 at 2:48 PM Steven Rostedt <[email protected]> wrote:
>
> > Your patch actually had them, but in the body of your email you did
> >
> > > #define have_prefix(str, prefix) ({ \
> > > const char *__pfx = (const char *)prefix; \
> >
> > which is just completely wrong.
> >
> > Considering your _old_ patch had the exact same bug, I really think
> > you need to start internalizing the whole "macro arguments *have* to
> > be properly protected" thing.
>
> Well, there's less with assignments that can go wrong than with other
> code. That is, there's little that can happen with "int x = arg;" where
> arg is the macro paramater to cause something really nasty.
What's wrong, Steven?
The assignment is entirely irrelevant.
The problem is the cast.
A type cast has a very high priority, and so if you do
(const char *)prefix
it breaks completely if you might have something like"a+6" as the argument.
Think about what if "a" is of type "unsigned long", for example?
Linus
On Fri, 21 Dec 2018 14:08:11 -0800
Linus Torvalds <[email protected]> wrote:
> On Fri, Dec 21, 2018 at 12:55 PM Steven Rostedt <[email protected]> wrote:
> >
> > On Fri, 21 Dec 2018 12:41:17 -0800
> > Linus Torvalds <[email protected]> wrote:
> >
> > > Parentheses....
> >
> > ?
>
> Your patch actually had them, but in the body of your email you did
>
> > #define have_prefix(str, prefix) ({ \
> > const char *__pfx = (const char *)prefix; \
>
> which is just completely wrong.
>
> Considering your _old_ patch had the exact same bug, I really think
> you need to start internalizing the whole "macro arguments *have* to
> be properly protected" thing.
Well, there's less with assignments that can go wrong than with other
code. That is, there's little that can happen with "int x = arg;" where
arg is the macro paramater to cause something really nasty. The chances
of that happening is up there with using only two underscores causing an
issue.
But they don't hurt to add.
>
> And I agree with Joe that using a million underscores just makes code
> less legible. Two underscores at the beginning is the standard
> namespace protection. Underscores at the end do nothing. And using
> *more* than two is just crazy.
The two are standard for static variables in C code, but that makes me
worry about macros. I usually do at least three for macros. The
underscores at the end, to me, make it more balanced and easier to read:
strncmp(str, ___prefix___, ___len___);
To me looks better than:
strncmp(str, ___prefix, ___len);
But again, no reason to fight this bikeshed.
>
> Finally, I think the cast is actually bogus and wrong. Why would the
> prefix ever be anything but "const char *"? Adding the cast only makes
> it more possible that somebody uses a truly wrong type ("unsigned long
> *" ?), and then the cast just silently hides it.
Actually after I sent the email, I was thinking the same thing, that
the original strncmp() would probably complain about that too, and we
should keep it the same. Not sure why I even suggested that. Perhaps, I
tested too many use cases :-/
>
> If somebody uses "unsigned char *" for this, they'd get the exact same
> warning if they were using strncmp and do this by hand.
>
> So why would that helper function act any differently? Particularly
> when it then has the huge downside that it will also accept absolute
> garbage?
>
> Anyway, that was a long and winding NAK for your patch.
But after all that and said. I tested it as a static __always_inline,
and guess what. It also optimizes out the strlen() for constants!!!
Suggested-by: Andreas Schwab <[email protected]>
-- Steve
diff --git a/include/linux/string.h b/include/linux/string.h
index 27d0482e5e05..538469dfb458 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -456,4 +456,25 @@ static inline void memcpy_and_pad(void *dest, size_t dest_len,
memcpy(dest, src, dest_len);
}
+/**
+ * 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 int str_has_prefix(const char *str, const char *prefix)
+{
+ int len = strlen(prefix);
+
+ return strncmp(str, prefix, len) == 0 ? len : 0;
+}
+
#endif /* _LINUX_STRING_H_ */
On Fri, 21 Dec 2018 14:29:30 -0800
Linus Torvalds <[email protected]> wrote:
> On Fri, Dec 21, 2018 at 2:20 PM Joe Perches <[email protected]> wrote:
> >
> > Using
> >
> > static inline bool str_has_prefix(const char *str, const char prefix[])
> > {
> > return !strncmp(str, prefix, strlen(prefix));
> > }
> >
> > eliminates the strlen with gcc 4.8 (oldest I still have)
>
> Ok, that looks like the right thing to do.
>
Agreed, and I posted a new version. I can start running it through my
test suit (I'll update all the instances in the tracing directory to
use it), and then it will be ready for a pull request by next week.
I'll revert the top two patches from my for-next tree now.
-- Steve
On Fri, 21 Dec 2018 14:57:13 -0800
Linus Torvalds <[email protected]> wrote:
> On Fri, Dec 21, 2018 at 2:48 PM Steven Rostedt <[email protected]> wrote:
> >
> > > Your patch actually had them, but in the body of your email you did
> > >
> > > > #define have_prefix(str, prefix) ({ \
> > > > const char *__pfx = (const char *)prefix; \
> > >
> > > which is just completely wrong.
> > >
> > > Considering your _old_ patch had the exact same bug, I really think
> > > you need to start internalizing the whole "macro arguments *have* to
> > > be properly protected" thing.
> >
> > Well, there's less with assignments that can go wrong than with other
> > code. That is, there's little that can happen with "int x = arg;" where
> > arg is the macro paramater to cause something really nasty.
>
> What's wrong, Steven?
We are miscommunicating here...
I was talking about the missing parenthesis on the original patch,
which you stated was missing as well. And the original patch didn't
have the typecast.
>
> The assignment is entirely irrelevant.
>
> The problem is the cast.
>
> A type cast has a very high priority, and so if you do
>
> (const char *)prefix
>
> it breaks completely if you might have something like"a+6" as the argument.
>
> Think about what if "a" is of type "unsigned long", for example?
Yes, when writing the real code, I noticed that the typecast could
cause issues without the parenthesis, and added them.
The email you are replying to was saying there's not much that can go
wrong with:
#define MACRO(x) { \
int __p = x; \
I definitely can see something wrong with:
#define MACRO(x) { \
int __p = (int)x; \
because exactly what you stated.
There's nothing wrong with adding (x) for the first one, but it's
unlikely anything will cause it harm. The second one the (x) *is* most
definitely required.
This is a long winded "I agree with you" ;-)
-- Steve
On 21/12/2018 18:51, Linus Torvalds wrote:
> On Fri, Dec 21, 2018 at 9:57 AM Steven Rostedt <[email protected]> wrote:
>>
>> I figured the best thing to do is to create a helper macro and place it
>> into include/linux/string.h. And go around and fix all the open coded
>> versions of it later.
>>
>> I plan on only applying this patch and updating the tracing hooks for
>> this merge window. And perhaps use it to fix some of the bugs that were
>> found.
>
> I like the helper function concept, but as they say about CS: "There
> is only one hard problem in computer science: naming and off-by-one
> errors".
>
> And this one has that problem. The name makes absolutely no sense.
>
> Calling it "strncmp_prefix()" when there is no "n" there makes no sense.
>
> So drop the "n" from the name.
>
Being British the 'n' implies 'and' and still being interpreted Boolean.
strncmp = string and compare
Like other of our words Fish'n'Chips, Salt'n'Shake.
I don't think these abbreviations exist in American English.
On Fri, Dec 21, 2018 at 2:20 PM Joe Perches <[email protected]> wrote:
>
> Using
>
> static inline bool str_has_prefix(const char *str, const char prefix[])
> {
> return !strncmp(str, prefix, strlen(prefix));
> }
>
> eliminates the strlen with gcc 4.8 (oldest I still have)
Ok, that looks like the right thing to do.
Side note: in the kernel we disable the whole "unsigned vs signed"
pointer warning entirely, exactly because of the "char *" vs "unsigned
char *" problem.
Linus
On Sat, 22 Dec 2018 10:20:02 +0000
Malcolm Priestley <[email protected]> wrote:
> > Calling it "strncmp_prefix()" when there is no "n" there makes no sense.
> >
> > So drop the "n" from the name.
> >
> Being British the 'n' implies 'and' and still being interpreted Boolean.
>
> strncmp = string and compare
>
> Like other of our words Fish'n'Chips, Salt'n'Shake.
>
> I don't think these abbreviations exist in American English.
No, no, they actually do exist in American English.
But when people ask me which language is my mother tongue, I simply
reply "C". In which case, 'n' means number.
-- Steve
On 21/12/2018 23.20, Joe Perches wrote:
> On Fri, 2018-12-21 at 16:08 -0500, Steven Rostedt wrote:
>> On Fri, 21 Dec 2018 21:58:32 +0100
>> Andreas Schwab <[email protected]> wrote:
>>
>>
>>>> Well, perhaps I can just remove the ending ones. I get paranoid with
>>>> macro variables, and tend to over do it so that there's no question.
>>>
>>> Why not make it an inline function?
>>
>> Matters if that removes the strlen(const) optimization. I could try it
>> and see what happens.
>
> 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().
commit 66f92cf9d415e96a5bdd6c64de8dd8418595d2fc
Author: Rusty Russell <[email protected]>
Date: Tue Mar 31 13:05:36 2009 -0600
strstarts: helper function for !strncmp(str, prefix, strlen(prefix))
Please don't add a copy under another name.
As for converting existing users, go for it. FWIW, I ran a cocci script
a few years ago to find suspicious strncmp() cases, and there were some
(e87c3f, ca957b6), but fewer than I expected. There are some
confused/confusing ones that apparently deliberately do strncmp(a, b,
sizeof(b)) instead of the equivalent to strcmp(a, b) (e.g. 'strncmp(str,
"hwc", 4) == 0')
Rasmus
On Sun, 23 Dec 2018 23:01:52 +0100
Rasmus Villemoes <[email protected]> wrote:
> On 21/12/2018 23.20, Joe Perches wrote:
> > On Fri, 2018-12-21 at 16:08 -0500, Steven Rostedt wrote:
> >> On Fri, 21 Dec 2018 21:58:32 +0100
> >> Andreas Schwab <[email protected]> wrote:
> >>
> >>
> >>>> Well, perhaps I can just remove the ending ones. I get paranoid with
> >>>> macro variables, and tend to over do it so that there's no question.
> >>>
> >>> Why not make it an inline function?
> >>
> >> Matters if that removes the strlen(const) optimization. I could try it
> >> and see what happens.
> >
> > 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.
>
> commit 66f92cf9d415e96a5bdd6c64de8dd8418595d2fc
> Author: Rusty Russell <[email protected]>
> Date: Tue Mar 31 13:05:36 2009 -0600
>
> strstarts: helper function for !strncmp(str, prefix, strlen(prefix))
>
> Please don't add a copy under another name.
>
> As for converting existing users, go for it. FWIW, I ran a cocci script
> a few years ago to find suspicious strncmp() cases, and there were some
> (e87c3f, ca957b6), but fewer than I expected. There are some
> confused/confusing ones that apparently deliberately do strncmp(a, b,
> sizeof(b)) instead of the equivalent to strcmp(a, b) (e.g. 'strncmp(str,
> "hwc", 4) == 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.
Would it be OK to convert strstarts() to return the length of prefix?
-- Steve
On Sun, 2018-12-23 at 23:01 +0100, Rasmus Villemoes wrote:
> On 21/12/2018 23.20, Joe Perches wrote:
[]
> > static inline bool str_has_prefix(const char *str, const char prefix[])
[]
> We already have exactly that function, it's called strstarts().
Heh. Thanks Rasmus. I didn't remember that one.
I think the 'int str_has_prefix' naming and returning the prefix
length may be a bit better use than 'bool strstarts' and perhaps a
treewide conversion of the existing strstarts to str_has_prefix
would be OK as there aren't that many.
$ git grep -w strstarts | wc -l
91
On 23/12/2018 23.56, Steven Rostedt wrote:
> On Sun, 23 Dec 2018 23:01:52 +0100
> Rasmus Villemoes <[email protected]> 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().
>
> 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.
> 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.
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?
Rasmus
On Mon, 24 Dec 2018 00:52:13 +0100
Rasmus Villemoes <[email protected]> wrote:
> On 23/12/2018 23.56, Steven Rostedt wrote:
> > On Sun, 23 Dec 2018 23:01:52 +0100
> > Rasmus Villemoes <[email protected]> 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/[email protected]
+/**
+ * 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