| Message ID | 20250109211358.2619367-1-costa.shul@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] rtla: Fix implicit NULL dereference | expand |
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
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
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)
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(-)