[PATCH v1] rtla: Fix implicit NULL dereference

[PATCH v1] rtla: Fix implicit NULL dereference

[Costa Shulyupin]

The `record` variable is NULL when tracing is not requested:

struct osnoise_tool *record = NULL;

if (params->trace_output) {
        record = osnoise_init_trace_tool("osnoise");
....

Value of `&record->trace` in this case is NULL just because
the `trace` member is the first member `struct osnoise_tool` with offset 0.
`&record->trace` just returns the offset.

Explicit dereference `record->trace' would cause segmentation fault.

Add explicit check for zero `record`.

Signed-off-by: Costa Shulyupin <costa.shul@redhat.com>
---
 tools/tracing/rtla/src/osnoise_hist.c  | 4 ++--
 tools/tracing/rtla/src/osnoise_top.c   | 4 ++--
 tools/tracing/rtla/src/timerlat_hist.c | 4 ++--
 tools/tracing/rtla/src/timerlat_top.c  | 6 +++---
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/tools/tracing/rtla/src/osnoise_hist.c b/tools/tracing/rtla/src/osnoise_hist.c
index 214e2c93fde01..46add229967b1 100644
--- a/tools/tracing/rtla/src/osnoise_hist.c
+++ b/tools/tracing/rtla/src/osnoise_hist.c
@@ -970,7 +970,7 @@ int osnoise_hist_main(int argc, char *argv[])
 			goto out_hist;
 		}
 
-		if (trace_is_off(&tool->trace, &record->trace))
+		if (trace_is_off(&tool->trace, record ? &record->trace : NULL))
 			break;
 	}
 
@@ -980,7 +980,7 @@ int osnoise_hist_main(int argc, char *argv[])
 
 	return_value = 0;
 
-	if (trace_is_off(&tool->trace, &record->trace)) {
+	if (trace_is_off(&tool->trace, record ? &record->trace : NULL)) {
 		printf("rtla osnoise hit stop tracing\n");
 		if (params->trace_output) {
 			printf("  Saving trace to %s\n", params->trace_output);
diff --git a/tools/tracing/rtla/src/osnoise_top.c b/tools/tracing/rtla/src/osnoise_top.c
index 45647495ce3bd..a0302b30da122 100644
--- a/tools/tracing/rtla/src/osnoise_top.c
+++ b/tools/tracing/rtla/src/osnoise_top.c
@@ -801,7 +801,7 @@ int osnoise_top_main(int argc, char **argv)
 		if (!params->quiet)
 			osnoise_print_stats(params, tool);
 
-		if (trace_is_off(&tool->trace, &record->trace))
+		if (trace_is_off(&tool->trace, record ? &record->trace : NULL))
 			break;
 
 	}
@@ -810,7 +810,7 @@ int osnoise_top_main(int argc, char **argv)
 
 	return_value = 0;
 
-	if (trace_is_off(&tool->trace, &record->trace)) {
+	if (trace_is_off(&tool->trace, record ? &record->trace : NULL)) {
 		printf("osnoise hit stop tracing\n");
 		if (params->trace_output) {
 			printf("  Saving trace to %s\n", params->trace_output);
diff --git a/tools/tracing/rtla/src/timerlat_hist.c b/tools/tracing/rtla/src/timerlat_hist.c
index 4403cc4eba302..d92a894fecc00 100644
--- a/tools/tracing/rtla/src/timerlat_hist.c
+++ b/tools/tracing/rtla/src/timerlat_hist.c
@@ -1342,7 +1342,7 @@ int timerlat_hist_main(int argc, char *argv[])
 			goto out_hist;
 		}
 
-		if (trace_is_off(&tool->trace, &record->trace))
+		if (trace_is_off(&tool->trace, record ? &record->trace : NULL))
 			break;
 
 		/* is there still any user-threads ? */
@@ -1363,7 +1363,7 @@ int timerlat_hist_main(int argc, char *argv[])
 
 	return_value = 0;
 
-	if (trace_is_off(&tool->trace, &record->trace)) {
+	if (trace_is_off(&tool->trace, record ? &record->trace : NULL)) {
 		printf("rtla timerlat hit stop tracing\n");
 
 		if (!params->no_aa)
diff --git a/tools/tracing/rtla/src/timerlat_top.c b/tools/tracing/rtla/src/timerlat_top.c
index 059b468981e4d..f05ef7aadf515 100644
--- a/tools/tracing/rtla/src/timerlat_top.c
+++ b/tools/tracing/rtla/src/timerlat_top.c
@@ -1093,7 +1093,7 @@ int timerlat_top_main(int argc, char *argv[])
 	while (!stop_tracing) {
 		sleep(params->sleep_time);
 
-		if (params->aa_only && !trace_is_off(&top->trace, &record->trace))
+		if (params->aa_only && !trace_is_off(&top->trace, record ? &record->trace : NULL))
 			continue;
 
 		retval = tracefs_iterate_raw_events(trace->tep,
@@ -1110,7 +1110,7 @@ int timerlat_top_main(int argc, char *argv[])
 		if (!params->quiet)
 			timerlat_print_stats(params, top);
 
-		if (trace_is_off(&top->trace, &record->trace))
+		if (trace_is_off(&top->trace, record ? &record->trace : NULL))
 			break;
 
 		/* is there still any user-threads ? */
@@ -1131,7 +1131,7 @@ int timerlat_top_main(int argc, char *argv[])
 
 	return_value = 0;
 
-	if (trace_is_off(&top->trace, &record->trace)) {
+	if (trace_is_off(&top->trace, record ? &record->trace : NULL)) {
 		printf("rtla timerlat hit stop tracing\n");
 
 		if (!params->no_aa)
-- 
2.47.0

Re: [PATCH v1] rtla: Fix implicit NULL dereference

[Dan Carpenter]

The subject is bad because it says "Fix" when this is a clean up
and it says "NULL dereference" when there isn't any NULL dereference.

On Thu, Jan 09, 2025 at 11:13:26PM +0200, Costa Shulyupin wrote:
> The `record` variable is NULL when tracing is not requested:
> 
> struct osnoise_tool *record = NULL;
> 
> if (params->trace_output) {
>         record = osnoise_init_trace_tool("osnoise");
> ....
> 
> Value of `&record->trace` in this case is NULL just because
> the `trace` member is the first member `struct osnoise_tool` with offset 0.
> `&record->trace` just returns the offset.
> 
> Explicit dereference `record->trace' would cause segmentation fault.
> 
> Add explicit check for zero `record`.
> 

This commit message is very confusing.  I would normally not send a
patch like this, but if I did send it the commit message would say
something like:

  The "record" pointer can be NULL in this code.  When we're calling
  trace_is_off(&tool->trace, &record->trace) and "record" is NULL then
  it kind of looks like a NULL dereference.  It turns out that it's
  fine when you look at it more closely, but at first glance it looks
  sketchy.  Add an explicit NULL check to make the code more clear.

Tracing code is generally fast path code so maybe we don't want to add
a NULL check?  If we were really bothered by the existing code then a
better fix would be to add an inline function to do it.

regards,
dan carpenter

Re: [PATCH v1] rtla: Fix implicit NULL dereference

[Steven Rostedt]

On Fri, 10 Jan 2025 08:38:35 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:

> Tracing code is generally fast path code so maybe we don't want to add
> a NULL check?  If we were really bothered by the existing code then a
> better fix would be to add an inline function to do it.

Note, this code is to the user space tooling (see the "tools/" directory)
and in the slow path of that.

-- Steve