* Re: [PATCH] tools lib traceevent: Fix missing equality check for strcmp
2019-04-09 9:15 [PATCH] tools lib traceevent: Fix missing equality check for strcmp Rikard Falkeborn
@ 2019-04-09 13:21 ` Steven Rostedt
2019-04-09 14:54 ` Arnaldo Carvalho de Melo
2019-04-12 16:39 ` [tip:perf/urgent] " tip-bot for Rikard Falkeborn
2019-04-16 15:30 ` tip-bot for Rikard Falkeborn
2 siblings, 1 reply; 5+ messages in thread
From: Steven Rostedt @ 2019-04-09 13:21 UTC (permalink / raw)
To: Rikard Falkeborn; +Cc: linux-kernel, acme, tstoyanov, Linux Trace Devel
Hi Rikard,
On Tue, 9 Apr 2019 11:15:29 +0200
Rikard Falkeborn <rikard.falkeborn@gmail.com> wrote:
> There was a missing comparison with 0 when checking if type is "s64" or
> "u64". Therefore, the body of the if-statement was entered if "type" was
> "u64" or not "s64", which made the first strcmp() redundant since if
> type is "u64", it's not "s64".
>
> If type is "s64", the body of the if-statement is not entered but since the
> remainder of the function consists of if-statements which will not be
> entered if type is "s64", we will just return "val", which is correct,
> albeit at the cost of a few more calls to strcmp(), i.e., it will behave
> just as if the if-statement was entered.
>
> If type is neither "s64" or "u64", the body of the if-statement will be
> entered incorrectly and "val" returned. This means that any type that is
> checked after "s64" and "u64" is handled the same way as "s64" and
> "u64", i.e., the limiting of "val" to fit in for example "s8" is never
> reached.
>
> This was introduced in the kernel tree when the sources were copied from
> trace-cmd in commit f7d82350e597 ("tools/events: Add files to create
> libtraceevent.a"), and in the trace-cmd repo in 1cdbae6035cei ("Implement
> typecasting in parser") when the function was introduced, i.e., it has
> always behaved the wrong way.
>
> Detected by cppcheck.
Nice. Thanks for this.
>
> Fixes: f7d82350e597 ("tools/events: Add files to create libtraceevent.a")
> Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Arnaldo,
Care to pull this in?
Thanks!
-- Steve
> ---
> I have only compile tested the patch but it should be correct. I don't
> know if any other problems will surface due to this though.
> ---
> tools/lib/traceevent/event-parse.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> index 87494c7c619d..981c6ce2da2c 100644
> --- a/tools/lib/traceevent/event-parse.c
> +++ b/tools/lib/traceevent/event-parse.c
> @@ -2233,7 +2233,7 @@ eval_type_str(unsigned long long val, const char *type, int pointer)
> return val & 0xffffffff;
>
> if (strcmp(type, "u64") == 0 ||
> - strcmp(type, "s64"))
> + strcmp(type, "s64") == 0)
> return val;
>
> if (strcmp(type, "s8") == 0)
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] tools lib traceevent: Fix missing equality check for strcmp
2019-04-09 13:21 ` Steven Rostedt
@ 2019-04-09 14:54 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-04-09 14:54 UTC (permalink / raw)
To: Steven Rostedt
Cc: Rikard Falkeborn, linux-kernel, tstoyanov, Linux Trace Devel
Em Tue, Apr 09, 2019 at 09:21:22AM -0400, Steven Rostedt escreveu:
>
> Hi Rikard,
>
> On Tue, 9 Apr 2019 11:15:29 +0200
> Rikard Falkeborn <rikard.falkeborn@gmail.com> wrote:
>
> > There was a missing comparison with 0 when checking if type is "s64" or
> > "u64". Therefore, the body of the if-statement was entered if "type" was
> > "u64" or not "s64", which made the first strcmp() redundant since if
> > type is "u64", it's not "s64".
> >
> > If type is "s64", the body of the if-statement is not entered but since the
> > remainder of the function consists of if-statements which will not be
> > entered if type is "s64", we will just return "val", which is correct,
> > albeit at the cost of a few more calls to strcmp(), i.e., it will behave
> > just as if the if-statement was entered.
> >
> > If type is neither "s64" or "u64", the body of the if-statement will be
> > entered incorrectly and "val" returned. This means that any type that is
> > checked after "s64" and "u64" is handled the same way as "s64" and
> > "u64", i.e., the limiting of "val" to fit in for example "s8" is never
> > reached.
> >
> > This was introduced in the kernel tree when the sources were copied from
> > trace-cmd in commit f7d82350e597 ("tools/events: Add files to create
> > libtraceevent.a"), and in the trace-cmd repo in 1cdbae6035cei ("Implement
> > typecasting in parser") when the function was introduced, i.e., it has
> > always behaved the wrong way.
> >
> > Detected by cppcheck.
>
> Nice. Thanks for this.
>
> >
> > Fixes: f7d82350e597 ("tools/events: Add files to create libtraceevent.a")
> > Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
>
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>
> Arnaldo,
>
> Care to pull this in?
Sure, thanks.
- Arnaldo
> Thanks!
>
> -- Steve
>
> > ---
> > I have only compile tested the patch but it should be correct. I don't
> > know if any other problems will surface due to this though.
> > ---
> > tools/lib/traceevent/event-parse.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> > index 87494c7c619d..981c6ce2da2c 100644
> > --- a/tools/lib/traceevent/event-parse.c
> > +++ b/tools/lib/traceevent/event-parse.c
> > @@ -2233,7 +2233,7 @@ eval_type_str(unsigned long long val, const char *type, int pointer)
> > return val & 0xffffffff;
> >
> > if (strcmp(type, "u64") == 0 ||
> > - strcmp(type, "s64"))
> > + strcmp(type, "s64") == 0)
> > return val;
> >
> > if (strcmp(type, "s8") == 0)
^ permalink raw reply [flat|nested] 5+ messages in thread
* [tip:perf/urgent] tools lib traceevent: Fix missing equality check for strcmp
2019-04-09 9:15 [PATCH] tools lib traceevent: Fix missing equality check for strcmp Rikard Falkeborn
2019-04-09 13:21 ` Steven Rostedt
@ 2019-04-12 16:39 ` tip-bot for Rikard Falkeborn
2019-04-16 15:30 ` tip-bot for Rikard Falkeborn
2 siblings, 0 replies; 5+ messages in thread
From: tip-bot for Rikard Falkeborn @ 2019-04-12 16:39 UTC (permalink / raw)
To: linux-tip-commits
Cc: rostedt, tglx, linux-kernel, acme, mingo, tstoyanov,
rikard.falkeborn, hpa
Commit-ID: ff4f5471801f879aea8f9e3ace2b9af31efc9178
Gitweb: https://git.kernel.org/tip/ff4f5471801f879aea8f9e3ace2b9af31efc9178
Author: Rikard Falkeborn <rikard.falkeborn@gmail.com>
AuthorDate: Tue, 9 Apr 2019 11:15:29 +0200
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 9 Apr 2019 11:55:30 -0300
tools lib traceevent: Fix missing equality check for strcmp
There was a missing comparison with 0 when checking if type is "s64" or
"u64". Therefore, the body of the if-statement was entered if "type" was
"u64" or not "s64", which made the first strcmp() redundant since if
type is "u64", it's not "s64".
If type is "s64", the body of the if-statement is not entered but since
the remainder of the function consists of if-statements which will not
be entered if type is "s64", we will just return "val", which is
correct, albeit at the cost of a few more calls to strcmp(), i.e., it
will behave just as if the if-statement was entered.
If type is neither "s64" or "u64", the body of the if-statement will be
entered incorrectly and "val" returned. This means that any type that is
checked after "s64" and "u64" is handled the same way as "s64" and
"u64", i.e., the limiting of "val" to fit in for example "s8" is never
reached.
This was introduced in the kernel tree when the sources were copied from
trace-cmd in commit f7d82350e597 ("tools/events: Add files to create
libtraceevent.a"), and in the trace-cmd repo in 1cdbae6035cei
("Implement typecasting in parser") when the function was introduced,
i.e., it has always behaved the wrong way.
Detected by cppcheck.
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Cc: Tzvetomir Stoyanov <tstoyanov@vmware.com>
Fixes: f7d82350e597 ("tools/events: Add files to create libtraceevent.a")
Link: http://lkml.kernel.org/r/20190409091529.2686-1-rikard.falkeborn@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/lib/traceevent/event-parse.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 87494c7c619d..981c6ce2da2c 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -2233,7 +2233,7 @@ eval_type_str(unsigned long long val, const char *type, int pointer)
return val & 0xffffffff;
if (strcmp(type, "u64") == 0 ||
- strcmp(type, "s64"))
+ strcmp(type, "s64") == 0)
return val;
if (strcmp(type, "s8") == 0)
^ permalink raw reply related [flat|nested] 5+ messages in thread* [tip:perf/urgent] tools lib traceevent: Fix missing equality check for strcmp
2019-04-09 9:15 [PATCH] tools lib traceevent: Fix missing equality check for strcmp Rikard Falkeborn
2019-04-09 13:21 ` Steven Rostedt
2019-04-12 16:39 ` [tip:perf/urgent] " tip-bot for Rikard Falkeborn
@ 2019-04-16 15:30 ` tip-bot for Rikard Falkeborn
2 siblings, 0 replies; 5+ messages in thread
From: tip-bot for Rikard Falkeborn @ 2019-04-16 15:30 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, rostedt, hpa, rikard.falkeborn, mingo, acme, tglx,
tstoyanov
Commit-ID: f32c2877bcb068a718bb70094cd59ccc29d4d082
Gitweb: https://git.kernel.org/tip/f32c2877bcb068a718bb70094cd59ccc29d4d082
Author: Rikard Falkeborn <rikard.falkeborn@gmail.com>
AuthorDate: Tue, 9 Apr 2019 11:15:29 +0200
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 16 Apr 2019 11:27:36 -0300
tools lib traceevent: Fix missing equality check for strcmp
There was a missing comparison with 0 when checking if type is "s64" or
"u64". Therefore, the body of the if-statement was entered if "type" was
"u64" or not "s64", which made the first strcmp() redundant since if
type is "u64", it's not "s64".
If type is "s64", the body of the if-statement is not entered but since
the remainder of the function consists of if-statements which will not
be entered if type is "s64", we will just return "val", which is
correct, albeit at the cost of a few more calls to strcmp(), i.e., it
will behave just as if the if-statement was entered.
If type is neither "s64" or "u64", the body of the if-statement will be
entered incorrectly and "val" returned. This means that any type that is
checked after "s64" and "u64" is handled the same way as "s64" and
"u64", i.e., the limiting of "val" to fit in for example "s8" is never
reached.
This was introduced in the kernel tree when the sources were copied from
trace-cmd in commit f7d82350e597 ("tools/events: Add files to create
libtraceevent.a"), and in the trace-cmd repo in 1cdbae6035cei
("Implement typecasting in parser") when the function was introduced,
i.e., it has always behaved the wrong way.
Detected by cppcheck.
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Cc: Tzvetomir Stoyanov <tstoyanov@vmware.com>
Fixes: f7d82350e597 ("tools/events: Add files to create libtraceevent.a")
Link: http://lkml.kernel.org/r/20190409091529.2686-1-rikard.falkeborn@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/lib/traceevent/event-parse.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 87494c7c619d..981c6ce2da2c 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -2233,7 +2233,7 @@ eval_type_str(unsigned long long val, const char *type, int pointer)
return val & 0xffffffff;
if (strcmp(type, "u64") == 0 ||
- strcmp(type, "s64"))
+ strcmp(type, "s64") == 0)
return val;
if (strcmp(type, "s8") == 0)
^ permalink raw reply related [flat|nested] 5+ messages in thread