[PATCH linux-next] tools/tracing: fix application of sizeof to pointer

[PATCH linux-next] tools/tracing: fix application of sizeof to pointer

[CGEL]

From: jing yangyang <jing.yangyang@zte.com.cn>

sizeof when applied to a pointer typed expression gives the size of
the pointer.

./tools/tracing/latency/latency-collector.c:1541:10-16:ERROR application of sizeof to pointer

This issue was detected with the help of Coccinelle.

Reported-by: Zeal Robot <zealci@zte.com.cn>
Signed-off-by: jing yangyang <jing.yangyang@zte.com.cn>
---
 tools/tracing/latency/latency-collector.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/tracing/latency/latency-collector.c b/tools/tracing/latency/latency-collector.c
index 3a2e6bb..64d531d 100644
--- a/tools/tracing/latency/latency-collector.c
+++ b/tools/tracing/latency/latency-collector.c
@@ -1538,7 +1538,7 @@ static void tracing_loop(void)
 				mutex_lock(&print_mtx);
 				check_signals();
 				write_or_die(fd_stdout, queue_full_warning,
-					     sizeof(queue_full_warning));
+					     sizeof(*queue_full_warning));
 				mutex_unlock(&print_mtx);
 			}
 			modified--;
-- 
1.8.3.1

Re: [PATCH linux-next] tools/tracing: fix application of sizeof to pointer

[Viktor.Rosendahl]

Hi Yangyang,

On Thu, 2021-08-19 at 23:00 -0700, CGEL wrote:
> From: jing yangyang <jing.yangyang@zte.com.cn>
> 
> sizeof when applied to a pointer typed expression gives the size of
> the pointer.
> 
> ./tools/tracing/latency/latency-collector.c:1541:10-16:ERROR application of
> sizeof to pointer
> 
> This issue was detected with the help of Coccinelle.
> 
> Reported-by: Zeal Robot <zealci@zte.com.cn>
> Signed-off-by: jing yangyang <jing.yangyang@zte.com.cn>
> ---
>  tools/tracing/latency/latency-collector.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/tracing/latency/latency-collector.c
> b/tools/tracing/latency/latency-collector.c
> index 3a2e6bb..64d531d 100644
> --- a/tools/tracing/latency/latency-collector.c
> +++ b/tools/tracing/latency/latency-collector.c
> @@ -1538,7 +1538,7 @@ static void tracing_loop(void)
>  				mutex_lock(&print_mtx);
>  				check_signals();
>  				write_or_die(fd_stdout, queue_full_warning,
> -					     sizeof(queue_full_warning));
> +					     sizeof(*queue_full_warning));

The old code would give a size of 8, i.e. the size of the pointer. Your
suggestion will give a size of 1, i.e. the size of the first character of the
error message. So instead of ouputing "Could no" we would only write out "C".

What we want is the length of the error message. This could be achieved in two
ways:

1. By changing the sizeof(queue_full_warning) to strlen(queue_full_warning).

2. By changing the definition of queue_full_warning to be an array, in that case
we would like to use sizeof(queue_full_warning) - 1, the "- 1" comes from the
fact that we don't want to write out the terminating null character.

I think the first approach with strlen() is the better solution because it's
shorter and modern compilers will do the strlen() calculation of constant
strings at compile time anyway.

best regards,

Viktor 


>  				mutex_unlock(&print_mtx);
>  			}
>  			modified--;

Re: [PATCH linux-next] tools/tracing: fix application of sizeof to pointer

[Steven Rostedt]

On Fri, 20 Aug 2021 09:00:09 +0000
<Viktor.Rosendahl@bmw.de> wrote:

> > Reported-by: Zeal Robot <zealci@zte.com.cn>
> > Signed-off-by: jing yangyang <jing.yangyang@zte.com.cn>
> > ---
> >  tools/tracing/latency/latency-collector.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/tracing/latency/latency-collector.c
> > b/tools/tracing/latency/latency-collector.c
> > index 3a2e6bb..64d531d 100644
> > --- a/tools/tracing/latency/latency-collector.c
> > +++ b/tools/tracing/latency/latency-collector.c
> > @@ -1538,7 +1538,7 @@ static void tracing_loop(void)
> >  				mutex_lock(&print_mtx);
> >  				check_signals();
> >  				write_or_die(fd_stdout, queue_full_warning,
> > -					     sizeof(queue_full_warning));
> > +					     sizeof(*queue_full_warning));  
> 
> The old code would give a size of 8, i.e. the size of the pointer. Your
> suggestion will give a size of 1, i.e. the size of the first character of the
> error message. So instead of ouputing "Could no" we would only write out "C".

Which is obviously incorrect to use sizeof(*queue_full_warning), and
just makes the current bug into an even worse bug.

> 
> What we want is the length of the error message. This could be achieved in two
> ways:
> 
> 1. By changing the sizeof(queue_full_warning) to strlen(queue_full_warning).
> 
> 2. By changing the definition of queue_full_warning to be an array, in that case
> we would like to use sizeof(queue_full_warning) - 1, the "- 1" comes from the
> fact that we don't want to write out the terminating null character.
> 
> I think the first approach with strlen() is the better solution because it's
> shorter and modern compilers will do the strlen() calculation of constant
> strings at compile time anyway.

Either approach is fine. But it needs to fix the issue, and not just
blindly follow what Coccinelle tells you. Tools like Coccinelle can
help point you where a problem is. But people still need to use their
brain to actually fix the issue.

-- Steve