Re: [PATCH] tools lib traceevent: Fix missing equality check for strcmp

Re: [PATCH] tools lib traceevent: Fix missing equality check for strcmp

[Steven Rostedt]

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)

Re: [PATCH] tools lib traceevent: Fix missing equality check for strcmp

[Arnaldo Carvalho de Melo]

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)