Most memory allocations in rtla happen during early initialization before any resources are acquired that would require cleanup. In these cases, propagating allocation errors just adds complexity without any benefit. There's nothing to clean up, and the program must exit anyway. This patch introduces fatal allocation wrappers (calloc_fatal, reallocarray_fatal, strdup_fatal) that call fatal() on allocation failure. These wrappers simplify the code by eliminating unnecessary error propagation paths. The patch converts early allocations to use these wrappers in actions_init() and related action functions, osnoise_context_alloc() and osnoise_init_tool(), trace_instance_init() and trace event functions, and parameter structure allocations in main functions. This simplifies the code while maintaining the same behavior: immediate exit on allocation failure during initialization. Allocations that require cleanup, such as those in histogram allocation functions with goto cleanup paths, are left unchanged and continue to return errors. Signed-off-by: Wander Lairson Costa --- tools/tracing/rtla/src/actions.c | 50 ++++++++++++-------------- tools/tracing/rtla/src/actions.h | 8 ++--- tools/tracing/rtla/src/osnoise.c | 22 ++++-------- tools/tracing/rtla/src/osnoise_hist.c | 25 ++++--------- tools/tracing/rtla/src/osnoise_top.c | 25 ++++--------- tools/tracing/rtla/src/timerlat_hist.c | 24 ++++--------- tools/tracing/rtla/src/timerlat_top.c | 25 ++++--------- tools/tracing/rtla/src/trace.c | 30 ++++------------ tools/tracing/rtla/src/trace.h | 4 +-- tools/tracing/rtla/src/utils.c | 35 ++++++++++++++++++ tools/tracing/rtla/src/utils.h | 3 ++ 11 files changed, 108 insertions(+), 143 deletions(-) diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c index 8945aee58d511..ff7811e175930 100644 --- a/tools/tracing/rtla/src/actions.c +++ b/tools/tracing/rtla/src/actions.c @@ -15,7 +15,7 @@ void actions_init(struct actions *self) { self->size = action_default_size; - self->list = calloc(self->size, sizeof(struct action)); + self->list = calloc_fatal(self->size, sizeof(struct action)); self->len = 0; self->continue_flag = false; @@ -50,8 +50,10 @@ static struct action * actions_new(struct actions *self) { if (self->len >= self->size) { - self->size *= 2; - self->list = realloc(self->list, self->size * sizeof(struct action)); + const size_t new_size = self->size * 2; + + self->list = reallocarray_fatal(self->list, new_size, sizeof(struct action)); + self->size = new_size; } return &self->list[self->len++]; @@ -60,25 +62,21 @@ actions_new(struct actions *self) /* * actions_add_trace_output - add an action to output trace */ -int +void actions_add_trace_output(struct actions *self, const char *trace_output) { struct action *action = actions_new(self); self->present[ACTION_TRACE_OUTPUT] = true; action->type = ACTION_TRACE_OUTPUT; - action->trace_output = calloc(strlen(trace_output) + 1, sizeof(char)); - if (!action->trace_output) - return -1; + action->trace_output = calloc_fatal(strlen(trace_output) + 1, sizeof(char)); strcpy(action->trace_output, trace_output); - - return 0; } /* * actions_add_trace_output - add an action to send signal to a process */ -int +void actions_add_signal(struct actions *self, int signal, int pid) { struct action *action = actions_new(self); @@ -87,40 +85,32 @@ actions_add_signal(struct actions *self, int signal, int pid) action->type = ACTION_SIGNAL; action->signal = signal; action->pid = pid; - - return 0; } /* * actions_add_shell - add an action to execute a shell command */ -int +void actions_add_shell(struct actions *self, const char *command) { struct action *action = actions_new(self); self->present[ACTION_SHELL] = true; action->type = ACTION_SHELL; - action->command = calloc(strlen(command) + 1, sizeof(char)); - if (!action->command) - return -1; + action->command = calloc_fatal(strlen(command) + 1, sizeof(char)); strcpy(action->command, command); - - return 0; } /* * actions_add_continue - add an action to resume measurement */ -int +void actions_add_continue(struct actions *self) { struct action *action = actions_new(self); self->present[ACTION_CONTINUE] = true; action->type = ACTION_CONTINUE; - - return 0; } /* @@ -174,7 +164,8 @@ actions_parse(struct actions *self, const char *trigger, const char *tracefn) /* Only one argument allowed */ return -1; } - return actions_add_trace_output(self, trace_output); + actions_add_trace_output(self, trace_output); + break; case ACTION_SIGNAL: /* Takes two arguments, num (signal) and pid */ while (token != NULL) { @@ -197,21 +188,26 @@ actions_parse(struct actions *self, const char *trigger, const char *tracefn) /* Missing argument */ return -1; - return actions_add_signal(self, signal, pid); + actions_add_signal(self, signal, pid); + break; case ACTION_SHELL: if (token == NULL) return -1; - if (strlen(token) > 8 && strncmp(token, "command=", 8) == 0) - return actions_add_shell(self, token + 8); - return -1; + if (strlen(token) > 8 && strncmp(token, "command=", 8)) + return -1; + actions_add_shell(self, token + 8); + break; case ACTION_CONTINUE: /* Takes no argument */ if (token != NULL) return -1; - return actions_add_continue(self); + actions_add_continue(self); + break; default: return -1; } + + return 0; } /* diff --git a/tools/tracing/rtla/src/actions.h b/tools/tracing/rtla/src/actions.h index a4f9b570775b5..fb366d6509b8b 100644 --- a/tools/tracing/rtla/src/actions.h +++ b/tools/tracing/rtla/src/actions.h @@ -44,9 +44,9 @@ struct actions { void actions_init(struct actions *self); void actions_destroy(struct actions *self); -int actions_add_trace_output(struct actions *self, const char *trace_output); -int actions_add_signal(struct actions *self, int signal, int pid); -int actions_add_shell(struct actions *self, const char *command); -int actions_add_continue(struct actions *self); +void actions_add_trace_output(struct actions *self, const char *trace_output); +void actions_add_signal(struct actions *self, int signal, int pid); +void actions_add_shell(struct actions *self, const char *command); +void actions_add_continue(struct actions *self); int actions_parse(struct actions *self, const char *trigger, const char *tracefn); int actions_perform(struct actions *self); diff --git a/tools/tracing/rtla/src/osnoise.c b/tools/tracing/rtla/src/osnoise.c index 312c511fa0044..c5b41ec26b0a4 100644 --- a/tools/tracing/rtla/src/osnoise.c +++ b/tools/tracing/rtla/src/osnoise.c @@ -938,9 +938,7 @@ struct osnoise_context *osnoise_context_alloc(void) { struct osnoise_context *context; - context = calloc(1, sizeof(*context)); - if (!context) - return NULL; + context = calloc_fatal(1, sizeof(*context)); context->orig_stop_us = OSNOISE_OPTION_INIT_VAL; context->stop_us = OSNOISE_OPTION_INIT_VAL; @@ -1017,24 +1015,16 @@ void osnoise_destroy_tool(struct osnoise_tool *top) struct osnoise_tool *osnoise_init_tool(char *tool_name) { struct osnoise_tool *top; - int retval; - - top = calloc(1, sizeof(*top)); - if (!top) - return NULL; + top = calloc_fatal(1, sizeof(*top)); top->context = osnoise_context_alloc(); - if (!top->context) - goto out_err; - retval = trace_instance_init(&top->trace, tool_name); - if (retval) - goto out_err; + if (trace_instance_init(&top->trace, tool_name)) { + osnoise_destroy_tool(top); + return NULL; + } return top; -out_err: - osnoise_destroy_tool(top); - return NULL; } /* diff --git a/tools/tracing/rtla/src/osnoise_hist.c b/tools/tracing/rtla/src/osnoise_hist.c index ff8c231e47c47..af7d8621dd9d7 100644 --- a/tools/tracing/rtla/src/osnoise_hist.c +++ b/tools/tracing/rtla/src/osnoise_hist.c @@ -474,9 +474,7 @@ static struct common_params int c; char *trace_output = NULL; - params = calloc(1, sizeof(*params)); - if (!params) - exit(1); + params = calloc_fatal(1, sizeof(*params)); actions_init(¶ms->common.threshold_actions); actions_init(¶ms->common.end_actions); @@ -564,9 +562,6 @@ static struct common_params break; case 'e': tevent = trace_event_alloc(optarg); - if (!tevent) - fatal("Error alloc trace event"); - if (params->common.events) tevent->next = params->common.events; @@ -631,22 +626,16 @@ static struct common_params params->common.hist.with_zeros = 1; break; case '4': /* trigger */ - if (params->common.events) { - retval = trace_event_add_trigger(params->common.events, optarg); - if (retval) - fatal("Error adding trigger %s", optarg); - } else { + if (params->common.events) + trace_event_add_trigger(params->common.events, optarg); + else fatal("--trigger requires a previous -e"); - } break; case '5': /* filter */ - if (params->common.events) { - retval = trace_event_add_filter(params->common.events, optarg); - if (retval) - fatal("Error adding filter %s", optarg); - } else { + if (params->common.events) + trace_event_add_filter(params->common.events, optarg); + else fatal("--filter requires a previous -e"); - } break; case '6': params->common.warmup = get_llong_from_str(optarg); diff --git a/tools/tracing/rtla/src/osnoise_top.c b/tools/tracing/rtla/src/osnoise_top.c index 04c699bdd7363..31b7e92d76fe4 100644 --- a/tools/tracing/rtla/src/osnoise_top.c +++ b/tools/tracing/rtla/src/osnoise_top.c @@ -327,9 +327,7 @@ struct common_params *osnoise_top_parse_args(int argc, char **argv) int c; char *trace_output = NULL; - params = calloc(1, sizeof(*params)); - if (!params) - exit(1); + params = calloc_fatal(1, sizeof(*params)); actions_init(¶ms->common.threshold_actions); actions_init(¶ms->common.end_actions); @@ -410,9 +408,6 @@ struct common_params *osnoise_top_parse_args(int argc, char **argv) break; case 'e': tevent = trace_event_alloc(optarg); - if (!tevent) - fatal("Error alloc trace event"); - if (params->common.events) tevent->next = params->common.events; params->common.events = tevent; @@ -462,22 +457,16 @@ struct common_params *osnoise_top_parse_args(int argc, char **argv) params->threshold = get_llong_from_str(optarg); break; case '0': /* trigger */ - if (params->common.events) { - retval = trace_event_add_trigger(params->common.events, optarg); - if (retval) - fatal("Error adding trigger %s", optarg); - } else { + if (params->common.events) + trace_event_add_trigger(params->common.events, optarg); + else fatal("--trigger requires a previous -e"); - } break; case '1': /* filter */ - if (params->common.events) { - retval = trace_event_add_filter(params->common.events, optarg); - if (retval) - fatal("Error adding filter %s", optarg); - } else { + if (params->common.events) + trace_event_add_filter(params->common.events, optarg); + else fatal("--filter requires a previous -e"); - } break; case '2': params->common.warmup = get_llong_from_str(optarg); diff --git a/tools/tracing/rtla/src/timerlat_hist.c b/tools/tracing/rtla/src/timerlat_hist.c index 1fb471a787b79..226167c88c8d6 100644 --- a/tools/tracing/rtla/src/timerlat_hist.c +++ b/tools/tracing/rtla/src/timerlat_hist.c @@ -772,9 +772,7 @@ static struct common_params int c; char *trace_output = NULL; - params = calloc(1, sizeof(*params)); - if (!params) - exit(1); + params = calloc_fatal(1, sizeof(*params)); actions_init(¶ms->common.threshold_actions); actions_init(¶ms->common.end_actions); @@ -883,8 +881,6 @@ static struct common_params break; case 'e': tevent = trace_event_alloc(optarg); - if (!tevent) - fatal("Error alloc trace event"); if (params->common.events) tevent->next = params->common.events; @@ -963,22 +959,16 @@ static struct common_params params->common.hist.with_zeros = 1; break; case '6': /* trigger */ - if (params->common.events) { - retval = trace_event_add_trigger(params->common.events, optarg); - if (retval) - fatal("Error adding trigger %s", optarg); - } else { + if (params->common.events) + trace_event_add_trigger(params->common.events, optarg); + else fatal("--trigger requires a previous -e"); - } break; case '7': /* filter */ - if (params->common.events) { - retval = trace_event_add_filter(params->common.events, optarg); - if (retval) - fatal("Error adding filter %s", optarg); - } else { + if (params->common.events) + trace_event_add_filter(params->common.events, optarg); + else fatal("--filter requires a previous -e"); - } break; case '8': params->dma_latency = get_llong_from_str(optarg); diff --git a/tools/tracing/rtla/src/timerlat_top.c b/tools/tracing/rtla/src/timerlat_top.c index 29c2c1f717ed7..31e1514a2528d 100644 --- a/tools/tracing/rtla/src/timerlat_top.c +++ b/tools/tracing/rtla/src/timerlat_top.c @@ -544,9 +544,7 @@ static struct common_params int c; char *trace_output = NULL; - params = calloc(1, sizeof(*params)); - if (!params) - exit(1); + params = calloc_fatal(1, sizeof(*params)); actions_init(¶ms->common.threshold_actions); actions_init(¶ms->common.end_actions); @@ -655,9 +653,6 @@ static struct common_params break; case 'e': tevent = trace_event_alloc(optarg); - if (!tevent) - fatal("Error alloc trace event"); - if (params->common.events) tevent->next = params->common.events; params->common.events = tevent; @@ -713,22 +708,16 @@ static struct common_params params->common.user_data = true; break; case '0': /* trigger */ - if (params->common.events) { - retval = trace_event_add_trigger(params->common.events, optarg); - if (retval) - fatal("Error adding trigger %s", optarg); - } else { + if (params->common.events) + trace_event_add_trigger(params->common.events, optarg); + else fatal("--trigger requires a previous -e"); - } break; case '1': /* filter */ - if (params->common.events) { - retval = trace_event_add_filter(params->common.events, optarg); - if (retval) - fatal("Error adding filter %s", optarg); - } else { + if (params->common.events) + trace_event_add_filter(params->common.events, optarg); + else fatal("--filter requires a previous -e"); - } break; case '2': /* dma-latency */ params->dma_latency = get_llong_from_str(optarg); diff --git a/tools/tracing/rtla/src/trace.c b/tools/tracing/rtla/src/trace.c index 69cbc48d53d3a..b22bb844b71f3 100644 --- a/tools/tracing/rtla/src/trace.c +++ b/tools/tracing/rtla/src/trace.c @@ -192,9 +192,7 @@ void trace_instance_destroy(struct trace_instance *trace) */ int trace_instance_init(struct trace_instance *trace, char *tool_name) { - trace->seq = calloc(1, sizeof(*trace->seq)); - if (!trace->seq) - goto out_err; + trace->seq = calloc_fatal(1, sizeof(*trace->seq)); trace_seq_init(trace->seq); @@ -275,15 +273,9 @@ struct trace_events *trace_event_alloc(const char *event_string) { struct trace_events *tevent; - tevent = calloc(1, sizeof(*tevent)); - if (!tevent) - return NULL; + tevent = calloc_fatal(1, sizeof(*tevent)); - tevent->system = strdup(event_string); - if (!tevent->system) { - free(tevent); - return NULL; - } + tevent->system = strdup_fatal(event_string); tevent->event = strstr(tevent->system, ":"); if (tevent->event) { @@ -297,31 +289,23 @@ struct trace_events *trace_event_alloc(const char *event_string) /* * trace_event_add_filter - record an event filter */ -int trace_event_add_filter(struct trace_events *event, char *filter) +void trace_event_add_filter(struct trace_events *event, char *filter) { if (event->filter) free(event->filter); - event->filter = strdup(filter); - if (!event->filter) - return 1; - - return 0; + event->filter = strdup_fatal(filter); } /* * trace_event_add_trigger - record an event trigger action */ -int trace_event_add_trigger(struct trace_events *event, char *trigger) +void trace_event_add_trigger(struct trace_events *event, char *trigger) { if (event->trigger) free(event->trigger); - event->trigger = strdup(trigger); - if (!event->trigger) - return 1; - - return 0; + event->trigger = strdup_fatal(trigger); } /* diff --git a/tools/tracing/rtla/src/trace.h b/tools/tracing/rtla/src/trace.h index 1e5aee4b828dd..95b911a2228b2 100644 --- a/tools/tracing/rtla/src/trace.h +++ b/tools/tracing/rtla/src/trace.h @@ -45,6 +45,6 @@ void trace_events_destroy(struct trace_instance *instance, int trace_events_enable(struct trace_instance *instance, struct trace_events *events); -int trace_event_add_filter(struct trace_events *event, char *filter); -int trace_event_add_trigger(struct trace_events *event, char *trigger); +void trace_event_add_filter(struct trace_events *event, char *filter); +void trace_event_add_trigger(struct trace_events *event, char *trigger); int trace_set_buffer_size(struct trace_instance *trace, int size); diff --git a/tools/tracing/rtla/src/utils.c b/tools/tracing/rtla/src/utils.c index 9cf5a0098e9aa..acf95afa25b5a 100644 --- a/tools/tracing/rtla/src/utils.c +++ b/tools/tracing/rtla/src/utils.c @@ -1000,3 +1000,38 @@ char *parse_optional_arg(int argc, char **argv) return NULL; } } + +static inline void fatal_alloc(void) +{ + fatal("Error allocating memory\n"); +} + +void *calloc_fatal(size_t n, size_t size) +{ + void *p = calloc(n, size); + + if (!p) + fatal_alloc(); + + return p; +} + +void *reallocarray_fatal(void *p, size_t n, size_t size) +{ + p = reallocarray(p, n, size); + + if (!p) + fatal_alloc(); + + return p; +} + +char *strdup_fatal(const char *s) +{ + char *p = strdup(s); + + if (!p) + fatal_alloc(); + + return p; +} diff --git a/tools/tracing/rtla/src/utils.h b/tools/tracing/rtla/src/utils.h index 091df4ba45877..0ed2c7275f2c5 100644 --- a/tools/tracing/rtla/src/utils.h +++ b/tools/tracing/rtla/src/utils.h @@ -68,6 +68,9 @@ int set_comm_sched_attr(const char *comm_prefix, struct sched_attr *attr); int set_comm_cgroup(const char *comm_prefix, const char *cgroup); int set_pid_cgroup(pid_t pid, const char *cgroup); int set_cpu_dma_latency(int32_t latency); +void *calloc_fatal(size_t n, size_t size); +void *reallocarray_fatal(void *p, size_t n, size_t size); +char *strdup_fatal(const char *s); #ifdef HAVE_LIBCPUPOWER_SUPPORT int save_cpu_idle_disable_state(unsigned int cpu); int restore_cpu_idle_disable_state(unsigned int cpu); -- 2.52.0 The actions_add_trace_output() and actions_add_shell() functions were using calloc() followed by strcpy() to allocate and copy a string. This can be simplified by using strdup(), which allocates memory and copies the string in a single step. Replace the calloc() and strcpy() calls with strdup(), making the code more concise and readable. Signed-off-by: Wander Lairson Costa --- tools/tracing/rtla/src/actions.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c index ff7811e175930..090d514fe4126 100644 --- a/tools/tracing/rtla/src/actions.c +++ b/tools/tracing/rtla/src/actions.c @@ -69,8 +69,7 @@ actions_add_trace_output(struct actions *self, const char *trace_output) self->present[ACTION_TRACE_OUTPUT] = true; action->type = ACTION_TRACE_OUTPUT; - action->trace_output = calloc_fatal(strlen(trace_output) + 1, sizeof(char)); - strcpy(action->trace_output, trace_output); + action->trace_output = strdup_fatal(trace_output); } /* @@ -97,8 +96,7 @@ actions_add_shell(struct actions *self, const char *command) self->present[ACTION_SHELL] = true; action->type = ACTION_SHELL; - action->command = calloc_fatal(strlen(command) + 1, sizeof(char)); - strcpy(action->command, command); + action->command = strdup_fatal(command); } /* -- 2.52.0 The for loop to iterate over the list of actions is used in more than one place. To avoid code duplication and improve readability, introduce a for_each_action() helper macro. Replace the open-coded for loops with the new helper. Signed-off-by: Wander Lairson Costa --- tools/tracing/rtla/src/actions.c | 6 ++++-- tools/tracing/rtla/src/actions.h | 5 +++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c index 090d514fe4126..a4d0dc47e6aa1 100644 --- a/tools/tracing/rtla/src/actions.c +++ b/tools/tracing/rtla/src/actions.c @@ -32,7 +32,9 @@ void actions_destroy(struct actions *self) { /* Free any action-specific data */ - for (struct action *action = self->list; action < self->list + self->len; action++) { + struct action *action; + + for_each_action(self, action) { if (action->type == ACTION_SHELL) free(action->command); if (action->type == ACTION_TRACE_OUTPUT) @@ -217,7 +219,7 @@ actions_perform(struct actions *self) int pid, retval; const struct action *action; - for (action = self->list; action < self->list + self->len; action++) { + for_each_action(self, action) { switch (action->type) { case ACTION_TRACE_OUTPUT: retval = save_trace_to_file(self->trace_output_inst, action->trace_output); diff --git a/tools/tracing/rtla/src/actions.h b/tools/tracing/rtla/src/actions.h index fb366d6509b8b..034048682fefb 100644 --- a/tools/tracing/rtla/src/actions.h +++ b/tools/tracing/rtla/src/actions.h @@ -42,6 +42,11 @@ struct actions { struct tracefs_instance *trace_output_inst; }; +#define for_each_action(actions, action) \ + for ((action) = (actions)->list; \ + (action) < (actions)->list + (actions)->len; \ + (action)++) + void actions_init(struct actions *self); void actions_destroy(struct actions *self); void actions_add_trace_output(struct actions *self, const char *trace_output); -- 2.52.0 The atoi() function does not perform error checking, which can lead to undefined behavior when parsing invalid or out-of-range strings. This can cause issues when parsing user-provided numerical inputs, such as signal numbers, PIDs, or CPU lists. To address this, introduce a new strtoi() helper function that safely converts a string to an integer. This function validates the input and checks for overflows, returning a negative value on failure. Replace all calls to atoi() with the new strtoi() function and add proper error handling to make the parsing more robust and prevent potential issues. Signed-off-by: Wander Lairson Costa --- tools/tracing/rtla/src/actions.c | 7 +++--- tools/tracing/rtla/src/utils.c | 40 ++++++++++++++++++++++++++++---- tools/tracing/rtla/src/utils.h | 2 ++ 3 files changed, 41 insertions(+), 8 deletions(-) diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c index a4d0dc47e6aa1..e933c2c68b208 100644 --- a/tools/tracing/rtla/src/actions.c +++ b/tools/tracing/rtla/src/actions.c @@ -170,12 +170,13 @@ actions_parse(struct actions *self, const char *trigger, const char *tracefn) /* Takes two arguments, num (signal) and pid */ while (token != NULL) { if (strlen(token) > 4 && strncmp(token, "num=", 4) == 0) { - signal = atoi(token + 4); + if (strtoi(token + 4, &signal)) + return -1; } else if (strlen(token) > 4 && strncmp(token, "pid=", 4) == 0) { if (strncmp(token + 4, "parent", 7) == 0) pid = -1; - else - pid = atoi(token + 4); + else if (strtoi(token + 4, &pid)) + return -1; } else { /* Invalid argument */ return -1; diff --git a/tools/tracing/rtla/src/utils.c b/tools/tracing/rtla/src/utils.c index acf95afa25b5a..f3e129d17a82b 100644 --- a/tools/tracing/rtla/src/utils.c +++ b/tools/tracing/rtla/src/utils.c @@ -17,6 +17,7 @@ #include #include #include +#include #include "utils.h" @@ -127,16 +128,18 @@ int parse_cpu_set(char *cpu_list, cpu_set_t *set) nr_cpus = sysconf(_SC_NPROCESSORS_CONF); for (p = cpu_list; *p; ) { - cpu = atoi(p); - if (cpu < 0 || (!cpu && *p != '0') || cpu >= nr_cpus) + if (strtoi(p, &cpu)) + goto err; + if (cpu < 0 || cpu >= nr_cpus) goto err; while (isdigit(*p)) p++; if (*p == '-') { p++; - end_cpu = atoi(p); - if (end_cpu < cpu || (!end_cpu && *p != '0') || end_cpu >= nr_cpus) + if (strtoi(p, &end_cpu)) + goto err; + if (end_cpu < cpu || end_cpu >= nr_cpus) goto err; while (isdigit(*p)) p++; @@ -337,6 +340,7 @@ int set_comm_sched_attr(const char *comm_prefix, struct sched_attr *attr) struct dirent *proc_entry; DIR *procfs; int retval; + int pid; if (strlen(comm_prefix) >= MAX_PATH) { err_msg("Command prefix is too long: %d < strlen(%s)\n", @@ -356,8 +360,12 @@ int set_comm_sched_attr(const char *comm_prefix, struct sched_attr *attr) if (!retval) continue; + if (strtoi(proc_entry->d_name, &pid)) { + err_msg("'%s' is not a valid pid", proc_entry->d_name); + goto out_err; + } /* procfs_is_workload_pid confirmed it is a pid */ - retval = __set_sched_attr(atoi(proc_entry->d_name), attr); + retval = __set_sched_attr(pid, attr); if (retval) { err_msg("Error setting sched attributes for pid:%s\n", proc_entry->d_name); goto out_err; @@ -1035,3 +1043,25 @@ char *strdup_fatal(const char *s) return p; } + +/* + * strtoi - convert string to integer with error checking + * + * Returns 0 on success, -1 if conversion fails or result is out of int range. + */ +int strtoi(const char *s, int *res) +{ + char *end_ptr; + long lres; + + if (!*s) + return -1; + + errno = 0; + lres = strtol(s, &end_ptr, 0); + if (errno || *end_ptr || lres > INT_MAX || lres < INT_MIN) + return -1; + + *res = (int) lres; + return 0; +} diff --git a/tools/tracing/rtla/src/utils.h b/tools/tracing/rtla/src/utils.h index 0ed2c7275f2c5..efbf798650306 100644 --- a/tools/tracing/rtla/src/utils.h +++ b/tools/tracing/rtla/src/utils.h @@ -3,6 +3,7 @@ #include #include #include +#include /* * '18446744073709551615\0' @@ -85,6 +86,7 @@ static inline int set_deepest_cpu_idle_state(unsigned int cpu, unsigned int stat static inline int have_libcpupower_support(void) { return 0; } #endif /* HAVE_LIBCPUPOWER_SUPPORT */ int auto_house_keeping(cpu_set_t *monitored_cpus); +__attribute__((__warn_unused_result__)) int strtoi(const char *s, int *res); #define ns_to_usf(x) (((double)x/1000)) #define ns_to_per(total, part) ((part * 100) / (double)total) -- 2.52.0 The actions_parse() function uses open-coded logic to extract arguments from a string. This includes manual length checks and strncmp() calls, which can be verbose and error-prone. To simplify and improve the robustness of argument parsing, introduce a new extract_arg() helper macro. This macro extracts the value from a "key=value" pair, making the code more concise and readable. Also, introduce STRING_LENGTH() and strncmp_static() macros to perform compile-time calculations of string lengths and safer string comparisons. Refactor actions_parse() to use these new helpers, resulting in cleaner and more maintainable code. Signed-off-by: Wander Lairson Costa --- tools/tracing/rtla/src/actions.c | 57 +++++++++++++++++++++++--------- tools/tracing/rtla/src/utils.h | 14 ++++++-- 2 files changed, 54 insertions(+), 17 deletions(-) diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c index e933c2c68b208..b50bf9f9adf28 100644 --- a/tools/tracing/rtla/src/actions.c +++ b/tools/tracing/rtla/src/actions.c @@ -113,6 +113,29 @@ actions_add_continue(struct actions *self) action->type = ACTION_CONTINUE; } +static inline const char *__extract_arg(const char *token, const char *opt, size_t opt_len) +{ + const size_t tok_len = strlen(token); + + if (tok_len <= opt_len) + return NULL; + + if (strncmp(token, opt, opt_len)) + return NULL; + + return token + opt_len; +} + +/* + * extract_arg - extract argument value from option token + * @token: option token (e.g., "file=trace.txt") + * @opt: option name to match (e.g., "file") + * + * Returns pointer to argument value after "=" if token matches "opt=", + * otherwise returns NULL. + */ +#define extract_arg(token, opt) __extract_arg(token, opt "=", STRING_LENGTH(opt "=")) + /* * actions_parse - add an action based on text specification */ @@ -122,6 +145,7 @@ actions_parse(struct actions *self, const char *trigger, const char *tracefn) enum action_type type = ACTION_NONE; const char *token; char trigger_c[strlen(trigger) + 1]; + const char *arg_value; /* For ACTION_SIGNAL */ int signal = 0, pid = 0; @@ -152,12 +176,10 @@ actions_parse(struct actions *self, const char *trigger, const char *tracefn) if (token == NULL) trace_output = tracefn; else { - if (strlen(token) > 5 && strncmp(token, "file=", 5) == 0) { - trace_output = token + 5; - } else { + trace_output = extract_arg(token, "file"); + if (!trace_output) /* Invalid argument */ return -1; - } token = strtok(NULL, ","); if (token != NULL) @@ -169,17 +191,21 @@ actions_parse(struct actions *self, const char *trigger, const char *tracefn) case ACTION_SIGNAL: /* Takes two arguments, num (signal) and pid */ while (token != NULL) { - if (strlen(token) > 4 && strncmp(token, "num=", 4) == 0) { - if (strtoi(token + 4, &signal)) - return -1; - } else if (strlen(token) > 4 && strncmp(token, "pid=", 4) == 0) { - if (strncmp(token + 4, "parent", 7) == 0) - pid = -1; - else if (strtoi(token + 4, &pid)) + arg_value = extract_arg(token, "num"); + if (arg_value) { + if (strtoi(arg_value, &signal)) return -1; } else { - /* Invalid argument */ - return -1; + arg_value = extract_arg(token, "pid"); + if (arg_value) { + if (strncmp_static(arg_value, "parent") == 0) + pid = -1; + else if (strtoi(arg_value, &pid)) + return -1; + } else { + /* Invalid argument */ + return -1; + } } token = strtok(NULL, ","); @@ -194,9 +220,10 @@ actions_parse(struct actions *self, const char *trigger, const char *tracefn) case ACTION_SHELL: if (token == NULL) return -1; - if (strlen(token) > 8 && strncmp(token, "command=", 8)) + arg_value = extract_arg(token, "command"); + if (!arg_value) return -1; - actions_add_shell(self, token + 8); + actions_add_shell(self, arg_value); break; case ACTION_CONTINUE: /* Takes no argument */ diff --git a/tools/tracing/rtla/src/utils.h b/tools/tracing/rtla/src/utils.h index efbf798650306..7fa3ac5e0bfb6 100644 --- a/tools/tracing/rtla/src/utils.h +++ b/tools/tracing/rtla/src/utils.h @@ -13,8 +13,18 @@ #define MAX_NICE 20 #define MIN_NICE -19 -#define container_of(ptr, type, member)({ \ - const typeof(((type *)0)->member) *__mptr = (ptr); \ +#ifndef ARRAY_SIZE +#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x))) +#endif + +/* Calculate string length at compile time (excluding null terminator) */ +#define STRING_LENGTH(s) (ARRAY_SIZE(s) - sizeof(*(s))) + +/* Compare string with static string, length determined at compile time */ +#define strncmp_static(s1, s2) strncmp(s1, s2, STRING_LENGTH(s2)) + +#define container_of(ptr, type, member)({ \ + const typeof(((type *)0)->member) * __mptr = (ptr); \ (type *)((char *)__mptr - offsetof(type, member)) ; }) extern int config_debug; -- 2.52.0 The recently introduced strncmp_static() helper provides a safer way to compare strings with static strings by determining the length at compile time. Replace several open-coded strncmp() calls with strncmp_static() to improve code readability and robustness. This change affects the parsing of command-line arguments and environment variables. Signed-off-by: Wander Lairson Costa --- tools/tracing/rtla/src/osnoise.c | 2 +- tools/tracing/rtla/src/timerlat.c | 4 ++-- tools/tracing/rtla/src/trace.c | 2 +- tools/tracing/rtla/src/utils.c | 8 ++++---- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tools/tracing/rtla/src/osnoise.c b/tools/tracing/rtla/src/osnoise.c index c5b41ec26b0a4..f2ec2da7b6d3a 100644 --- a/tools/tracing/rtla/src/osnoise.c +++ b/tools/tracing/rtla/src/osnoise.c @@ -1219,7 +1219,7 @@ int osnoise_main(int argc, char *argv[]) if ((strcmp(argv[1], "-h") == 0) || (strcmp(argv[1], "--help") == 0)) { osnoise_usage(0); - } else if (strncmp(argv[1], "-", 1) == 0) { + } else if (strncmp_static(argv[1], "-") == 0) { /* the user skipped the tool, call the default one */ run_tool(&osnoise_top_ops, argc, argv); exit(0); diff --git a/tools/tracing/rtla/src/timerlat.c b/tools/tracing/rtla/src/timerlat.c index df4f9bfe34331..ac2ec89d3b6ba 100644 --- a/tools/tracing/rtla/src/timerlat.c +++ b/tools/tracing/rtla/src/timerlat.c @@ -34,7 +34,7 @@ timerlat_apply_config(struct osnoise_tool *tool, struct timerlat_params *params) * Try to enable BPF, unless disabled explicitly. * If BPF enablement fails, fall back to tracefs mode. */ - if (getenv("RTLA_NO_BPF") && strncmp(getenv("RTLA_NO_BPF"), "1", 2) == 0) { + if (getenv("RTLA_NO_BPF") && strncmp_static(getenv("RTLA_NO_BPF"), "1") == 0) { debug_msg("RTLA_NO_BPF set, disabling BPF\n"); params->mode = TRACING_MODE_TRACEFS; } else if (!tep_find_event_by_name(tool->trace.tep, "osnoise", "timerlat_sample")) { @@ -271,7 +271,7 @@ int timerlat_main(int argc, char *argv[]) if ((strcmp(argv[1], "-h") == 0) || (strcmp(argv[1], "--help") == 0)) { timerlat_usage(0); - } else if (strncmp(argv[1], "-", 1) == 0) { + } else if (strncmp_static(argv[1], "-") == 0) { /* the user skipped the tool, call the default one */ run_tool(&timerlat_top_ops, argc, argv); exit(0); diff --git a/tools/tracing/rtla/src/trace.c b/tools/tracing/rtla/src/trace.c index b22bb844b71f3..45328c5121f79 100644 --- a/tools/tracing/rtla/src/trace.c +++ b/tools/tracing/rtla/src/trace.c @@ -356,7 +356,7 @@ static void trace_event_save_hist(struct trace_instance *instance, return; /* is this a hist: trigger? */ - retval = strncmp(tevent->trigger, "hist:", strlen("hist:")); + retval = strncmp_static(tevent->trigger, "hist:"); if (retval) return; diff --git a/tools/tracing/rtla/src/utils.c b/tools/tracing/rtla/src/utils.c index f3e129d17a82b..e0f31e5cae844 100644 --- a/tools/tracing/rtla/src/utils.c +++ b/tools/tracing/rtla/src/utils.c @@ -211,15 +211,15 @@ long parse_ns_duration(char *val) t = strtol(val, &end, 10); if (end) { - if (!strncmp(end, "ns", 2)) { + if (!strncmp_static(end, "ns")) { return t; - } else if (!strncmp(end, "us", 2)) { + } else if (!strncmp_static(end, "us")) { t *= 1000; return t; - } else if (!strncmp(end, "ms", 2)) { + } else if (!strncmp_static(end, "ms")) { t *= 1000 * 1000; return t; - } else if (!strncmp(end, "s", 1)) { + } else if (!strncmp_static(end, "s")) { t *= 1000 * 1000 * 1000; return t; } -- 2.52.0 A few functions duplicate the logic for handling threshold actions. When a threshold is reached, these functions stop the trace, perform actions, and restart the trace if configured to continue. Create a new helper function, common_restart(), to centralize this shared logic and avoid code duplication. This function now handles the threshold actions and restarts the necessary trace instances. Refactor the affected functions main loops to call the new helper. This makes the code cleaner and more maintainable. Signed-off-by: Wander Lairson Costa --- tools/tracing/rtla/src/common.c | 65 +++++++++++++++++++------- tools/tracing/rtla/src/common.h | 9 ++++ tools/tracing/rtla/src/timerlat_hist.c | 20 ++++---- tools/tracing/rtla/src/timerlat_top.c | 20 ++++---- 4 files changed, 78 insertions(+), 36 deletions(-) diff --git a/tools/tracing/rtla/src/common.c b/tools/tracing/rtla/src/common.c index b197037fc58b3..d608ffe12e7b0 100644 --- a/tools/tracing/rtla/src/common.c +++ b/tools/tracing/rtla/src/common.c @@ -95,6 +95,37 @@ common_apply_config(struct osnoise_tool *tool, struct common_params *params) } +/** + * common_restart - handle threshold actions and optionally restart tracing + * @tool: pointer to the osnoise_tool instance containing trace contexts + * @params: timerlat parameters with threshold action configuration + * + * Return: + * RESTART_OK - Actions executed successfully and tracing restarted + * RESTART_STOP - Actions executed but 'continue' flag not set, stop tracing + * RESTART_ERROR - Failed to restart tracing after executing actions + */ +enum restart_result +common_restart(const struct osnoise_tool *tool, struct common_params *params) +{ + actions_perform(¶ms->threshold_actions); + + if (!params->threshold_actions.continue_flag) + /* continue flag not set, break */ + return RESTART_STOP; + + /* continue action reached, re-enable tracing */ + if (tool->record && trace_instance_start(&tool->record->trace)) + goto err; + if (tool->aa && trace_instance_start(&tool->aa->trace)) + goto err; + return RESTART_OK; + +err: + err_msg("Error restarting trace\n"); + return RESTART_ERROR; +} + int run_tool(struct tool_ops *ops, int argc, char *argv[]) { struct common_params *params; @@ -272,17 +303,16 @@ int top_main_loop(struct osnoise_tool *tool) /* stop tracing requested, do not perform actions */ return 0; - actions_perform(¶ms->threshold_actions); + enum restart_result result; + + result = common_restart(tool, params); - if (!params->threshold_actions.continue_flag) - /* continue flag not set, break */ + if (result == RESTART_STOP) return 0; - /* continue action reached, re-enable tracing */ - if (record) - trace_instance_start(&record->trace); - if (tool->aa) - trace_instance_start(&tool->aa->trace); + if (result == RESTART_ERROR) + return -1; + trace_instance_start(trace); } @@ -323,18 +353,17 @@ int hist_main_loop(struct osnoise_tool *tool) /* stop tracing requested, do not perform actions */ break; - actions_perform(¶ms->threshold_actions); + enum restart_result result; - if (!params->threshold_actions.continue_flag) - /* continue flag not set, break */ - break; + result = common_restart(tool, params); + + if (result == RESTART_STOP) + return 0; - /* continue action reached, re-enable tracing */ - if (tool->record) - trace_instance_start(&tool->record->trace); - if (tool->aa) - trace_instance_start(&tool->aa->trace); - trace_instance_start(&tool->trace); + if (result == RESTART_ERROR) + return -1; + + trace_instance_start(trace); } /* is there still any user-threads ? */ diff --git a/tools/tracing/rtla/src/common.h b/tools/tracing/rtla/src/common.h index 9ec2b7632c376..f2c9e21c03651 100644 --- a/tools/tracing/rtla/src/common.h +++ b/tools/tracing/rtla/src/common.h @@ -143,6 +143,15 @@ struct tool_ops { void (*free)(struct osnoise_tool *tool); }; +enum restart_result { + RESTART_OK, + RESTART_STOP, + RESTART_ERROR = -1, +}; + +enum restart_result +common_restart(const struct osnoise_tool *tool, struct common_params *params); + int osnoise_set_cpus(struct osnoise_context *context, char *cpus); void osnoise_restore_cpus(struct osnoise_context *context); diff --git a/tools/tracing/rtla/src/timerlat_hist.c b/tools/tracing/rtla/src/timerlat_hist.c index 226167c88c8d6..27fc98270da59 100644 --- a/tools/tracing/rtla/src/timerlat_hist.c +++ b/tools/tracing/rtla/src/timerlat_hist.c @@ -17,6 +17,7 @@ #include "timerlat.h" #include "timerlat_aa.h" #include "timerlat_bpf.h" +#include "common.h" struct timerlat_hist_cpu { int *irq; @@ -1100,18 +1101,19 @@ static int timerlat_hist_bpf_main_loop(struct osnoise_tool *tool) if (!stop_tracing) { /* Threshold overflow, perform actions on threshold */ - actions_perform(¶ms->common.threshold_actions); + enum restart_result result; - if (!params->common.threshold_actions.continue_flag) - /* continue flag not set, break */ + result = common_restart(tool, ¶ms->common); + if (result == RESTART_STOP) break; - /* continue action reached, re-enable tracing */ - if (tool->record) - trace_instance_start(&tool->record->trace); - if (tool->aa) - trace_instance_start(&tool->aa->trace); - timerlat_bpf_restart_tracing(); + if (result == RESTART_ERROR) + return -1; + + if (timerlat_bpf_restart_tracing()) { + err_msg("Error restarting BPF trace\n"); + return -1; + } } } timerlat_bpf_detach(); diff --git a/tools/tracing/rtla/src/timerlat_top.c b/tools/tracing/rtla/src/timerlat_top.c index 31e1514a2528d..f7e85dc918ef3 100644 --- a/tools/tracing/rtla/src/timerlat_top.c +++ b/tools/tracing/rtla/src/timerlat_top.c @@ -18,6 +18,7 @@ #include "timerlat.h" #include "timerlat_aa.h" #include "timerlat_bpf.h" +#include "common.h" struct timerlat_top_cpu { unsigned long long irq_count; @@ -869,18 +870,19 @@ timerlat_top_bpf_main_loop(struct osnoise_tool *tool) if (wait_retval != 0) { /* Stopping requested by tracer */ - actions_perform(¶ms->common.threshold_actions); + enum restart_result result; - if (!params->common.threshold_actions.continue_flag) - /* continue flag not set, break */ + result = common_restart(tool, ¶ms->common); + if (result == RESTART_STOP) break; - /* continue action reached, re-enable tracing */ - if (tool->record) - trace_instance_start(&tool->record->trace); - if (tool->aa) - trace_instance_start(&tool->aa->trace); - timerlat_bpf_restart_tracing(); + if (result == RESTART_ERROR) + return -1; + + if (timerlat_bpf_restart_tracing()) { + err_msg("Error restarting BPF trace\n"); + return -1; + } } /* is there still any user-threads ? */ -- 2.52.0 The result enum defines custom values for PASSED, ERROR, and FAILED. These values correspond to standard exit codes EXIT_SUCCESS and EXIT_FAILURE. Update the enum to use the standard macros EXIT_SUCCESS and EXIT_FAILURE to improve readability and adherence to standard C practices. The FAILED value is implicitly assigned EXIT_FAILURE + 1, so there is no need to assign an explicit value. Signed-off-by: Wander Lairson Costa --- tools/tracing/rtla/src/utils.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tools/tracing/rtla/src/utils.h b/tools/tracing/rtla/src/utils.h index 7fa3ac5e0bfb6..5286a4c7165d3 100644 --- a/tools/tracing/rtla/src/utils.h +++ b/tools/tracing/rtla/src/utils.h @@ -4,6 +4,7 @@ #include #include #include +#include /* * '18446744073709551615\0' @@ -102,7 +103,7 @@ __attribute__((__warn_unused_result__)) int strtoi(const char *s, int *res); #define ns_to_per(total, part) ((part * 100) / (double)total) enum result { - PASSED = 0, /* same as EXIT_SUCCESS */ - ERROR = 1, /* same as EXIT_FAILURE, an error in arguments */ - FAILED = 2, /* test hit the stop tracing condition */ + PASSED = EXIT_SUCCESS, + ERROR = EXIT_FAILURE, + FAILED, /* test hit the stop tracing condition */ }; -- 2.52.0 The actions struct is allocated using calloc, which already returns zeroed memory. The subsequent memset call to zero the 'present' member is therefore redundant. Signed-off-by: Wander Lairson Costa --- tools/tracing/rtla/src/actions.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c index b50bf9f9adf28..00bbc94dec1bd 100644 --- a/tools/tracing/rtla/src/actions.c +++ b/tools/tracing/rtla/src/actions.c @@ -19,8 +19,6 @@ actions_init(struct actions *self) self->len = 0; self->continue_flag = false; - memset(&self->present, 0, sizeof(self->present)); - /* This has to be set by the user */ self->trace_output_inst = NULL; } -- 2.52.0 The trace functions use a buffer to manipulate strings that will be written to tracefs files. These buffers are defined with a magic number of 1024, which is a common source of vulnerabilities. Replace the magic number 1024 with the MAX_PATH macro to make the code safer and more readable. While at it, replace other instances of the magic number with ARRAY_SIZE() when the buffer is locally defined. Signed-off-by: Wander Lairson Costa --- tools/tracing/rtla/src/osnoise.c | 4 ++-- tools/tracing/rtla/src/timerlat_u.c | 4 ++-- tools/tracing/rtla/src/trace.c | 20 ++++++++++---------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/tools/tracing/rtla/src/osnoise.c b/tools/tracing/rtla/src/osnoise.c index f2ec2da7b6d3a..68927d799dde5 100644 --- a/tools/tracing/rtla/src/osnoise.c +++ b/tools/tracing/rtla/src/osnoise.c @@ -52,7 +52,7 @@ char *osnoise_get_cpus(struct osnoise_context *context) int osnoise_set_cpus(struct osnoise_context *context, char *cpus) { char *orig_cpus = osnoise_get_cpus(context); - char buffer[1024]; + char buffer[MAX_PATH]; int retval; if (!orig_cpus) @@ -62,7 +62,7 @@ int osnoise_set_cpus(struct osnoise_context *context, char *cpus) if (!context->curr_cpus) return -1; - snprintf(buffer, 1024, "%s\n", cpus); + snprintf(buffer, ARRAY_SIZE(buffer), "%s\n", cpus); debug_msg("setting cpus to %s from %s", cpus, context->orig_cpus); diff --git a/tools/tracing/rtla/src/timerlat_u.c b/tools/tracing/rtla/src/timerlat_u.c index ce68e39d25fde..efe2f72686486 100644 --- a/tools/tracing/rtla/src/timerlat_u.c +++ b/tools/tracing/rtla/src/timerlat_u.c @@ -32,7 +32,7 @@ static int timerlat_u_main(int cpu, struct timerlat_u_params *params) { struct sched_param sp = { .sched_priority = 95 }; - char buffer[1024]; + char buffer[MAX_PATH]; int timerlat_fd; cpu_set_t set; int retval; @@ -83,7 +83,7 @@ static int timerlat_u_main(int cpu, struct timerlat_u_params *params) /* add should continue with a signal handler */ while (true) { - retval = read(timerlat_fd, buffer, 1024); + retval = read(timerlat_fd, buffer, ARRAY_SIZE(buffer)); if (retval < 0) break; } diff --git a/tools/tracing/rtla/src/trace.c b/tools/tracing/rtla/src/trace.c index 45328c5121f79..0a81a2e4667ef 100644 --- a/tools/tracing/rtla/src/trace.c +++ b/tools/tracing/rtla/src/trace.c @@ -314,7 +314,7 @@ void trace_event_add_trigger(struct trace_events *event, char *trigger) static void trace_event_disable_filter(struct trace_instance *instance, struct trace_events *tevent) { - char filter[1024]; + char filter[MAX_PATH]; int retval; if (!tevent->filter) @@ -326,7 +326,7 @@ static void trace_event_disable_filter(struct trace_instance *instance, debug_msg("Disabling %s:%s filter %s\n", tevent->system, tevent->event ? : "*", tevent->filter); - snprintf(filter, 1024, "!%s\n", tevent->filter); + snprintf(filter, ARRAY_SIZE(filter), "!%s\n", tevent->filter); retval = tracefs_event_file_write(instance->inst, tevent->system, tevent->event, "filter", filter); @@ -345,7 +345,7 @@ static void trace_event_save_hist(struct trace_instance *instance, { int retval, index, out_fd; mode_t mode = 0644; - char path[1024]; + char path[MAX_PATH]; char *hist; if (!tevent) @@ -360,7 +360,7 @@ static void trace_event_save_hist(struct trace_instance *instance, if (retval) return; - snprintf(path, 1024, "%s_%s_hist.txt", tevent->system, tevent->event); + snprintf(path, ARRAY_SIZE(path), "%s_%s_hist.txt", tevent->system, tevent->event); printf(" Saving event %s:%s hist to %s\n", tevent->system, tevent->event, path); @@ -392,7 +392,7 @@ static void trace_event_save_hist(struct trace_instance *instance, static void trace_event_disable_trigger(struct trace_instance *instance, struct trace_events *tevent) { - char trigger[1024]; + char trigger[MAX_PATH]; int retval; if (!tevent->trigger) @@ -406,7 +406,7 @@ static void trace_event_disable_trigger(struct trace_instance *instance, trace_event_save_hist(instance, tevent); - snprintf(trigger, 1024, "!%s\n", tevent->trigger); + snprintf(trigger, ARRAY_SIZE(trigger), "!%s\n", tevent->trigger); retval = tracefs_event_file_write(instance->inst, tevent->system, tevent->event, "trigger", trigger); @@ -445,7 +445,7 @@ void trace_events_disable(struct trace_instance *instance, static int trace_event_enable_filter(struct trace_instance *instance, struct trace_events *tevent) { - char filter[1024]; + char filter[MAX_PATH]; int retval; if (!tevent->filter) @@ -457,7 +457,7 @@ static int trace_event_enable_filter(struct trace_instance *instance, return 1; } - snprintf(filter, 1024, "%s\n", tevent->filter); + snprintf(filter, ARRAY_SIZE(filter), "%s\n", tevent->filter); debug_msg("Enabling %s:%s filter %s\n", tevent->system, tevent->event ? : "*", tevent->filter); @@ -480,7 +480,7 @@ static int trace_event_enable_filter(struct trace_instance *instance, static int trace_event_enable_trigger(struct trace_instance *instance, struct trace_events *tevent) { - char trigger[1024]; + char trigger[MAX_PATH]; int retval; if (!tevent->trigger) @@ -492,7 +492,7 @@ static int trace_event_enable_trigger(struct trace_instance *instance, return 1; } - snprintf(trigger, 1024, "%s\n", tevent->trigger); + snprintf(trigger, ARRAY_SIZE(trigger), "%s\n", tevent->trigger); debug_msg("Enabling %s:%s trigger %s\n", tevent->system, tevent->event ? : "*", tevent->trigger); -- 2.52.0 Remove unused includes for and to clean up the code and reduce unnecessary dependencies. Signed-off-by: Wander Lairson Costa --- tools/tracing/rtla/src/osnoise_hist.c | 1 - tools/tracing/rtla/src/timerlat.c | 1 - tools/tracing/rtla/src/timerlat_top.c | 1 - tools/tracing/rtla/src/trace.c | 1 - 4 files changed, 4 deletions(-) diff --git a/tools/tracing/rtla/src/osnoise_hist.c b/tools/tracing/rtla/src/osnoise_hist.c index af7d8621dd9d7..c8e681f732315 100644 --- a/tools/tracing/rtla/src/osnoise_hist.c +++ b/tools/tracing/rtla/src/osnoise_hist.c @@ -9,7 +9,6 @@ #include #include #include -#include #include #include diff --git a/tools/tracing/rtla/src/timerlat.c b/tools/tracing/rtla/src/timerlat.c index ac2ec89d3b6ba..b3d63506c4a62 100644 --- a/tools/tracing/rtla/src/timerlat.c +++ b/tools/tracing/rtla/src/timerlat.c @@ -9,7 +9,6 @@ #include #include #include -#include #include #include #include diff --git a/tools/tracing/rtla/src/timerlat_top.c b/tools/tracing/rtla/src/timerlat_top.c index f7e85dc918ef3..79ee66743bf1d 100644 --- a/tools/tracing/rtla/src/timerlat_top.c +++ b/tools/tracing/rtla/src/timerlat_top.c @@ -11,7 +11,6 @@ #include #include #include -#include #include #include diff --git a/tools/tracing/rtla/src/trace.c b/tools/tracing/rtla/src/trace.c index 0a81a2e4667ef..092fcab77dc4c 100644 --- a/tools/tracing/rtla/src/trace.c +++ b/tools/tracing/rtla/src/trace.c @@ -2,7 +2,6 @@ #define _GNU_SOURCE #include #include -#include #include #include #include -- 2.52.0 The actions_parse() function uses strtok() to tokenize the trigger string, but does not check if the returned token is NULL before passing it to strcmp(). If the trigger parameter is an empty string or contains only delimiter characters, strtok() returns NULL, causing strcmp() to dereference a NULL pointer and crash the program. This issue can be triggered by malformed user input or edge cases in trigger string parsing. Add a NULL check immediately after the strtok() call to validate that a token was successfully extracted before using it. If no token is found, the function now returns -1 to indicate a parsing error. Signed-off-by: Wander Lairson Costa --- tools/tracing/rtla/src/actions.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c index 00bbc94dec1bd..b0d68b5de08db 100644 --- a/tools/tracing/rtla/src/actions.c +++ b/tools/tracing/rtla/src/actions.c @@ -153,6 +153,8 @@ actions_parse(struct actions *self, const char *trigger, const char *tracefn) strcpy(trigger_c, trigger); token = strtok(trigger_c, ","); + if (!token) + return -1; if (strcmp(token, "trace") == 0) type = ACTION_TRACE_OUTPUT; -- 2.52.0 The run_thread_comm and current_comm character arrays in struct timerlat_aa_data are defined with size MAX_COMM (24 bytes), but strncpy() is called with MAX_COMM as the size parameter. If the source string is exactly MAX_COMM bytes or longer, strncpy() will copy exactly MAX_COMM bytes without null termination, potentially causing buffer overruns when these strings are later used. Increase the buffer sizes to MAX_COMM+1 to ensure there is always room for the null terminator. This guarantees that even when strncpy() copies the maximum number of characters, the buffer remains properly null-terminated and safe to use in subsequent string operations. Signed-off-by: Wander Lairson Costa --- tools/tracing/rtla/src/timerlat_aa.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/tracing/rtla/src/timerlat_aa.c b/tools/tracing/rtla/src/timerlat_aa.c index 31e66ea2b144c..d310fe65abace 100644 --- a/tools/tracing/rtla/src/timerlat_aa.c +++ b/tools/tracing/rtla/src/timerlat_aa.c @@ -47,7 +47,7 @@ struct timerlat_aa_data { * note: "unsigned long long" because they are fetch using tep_get_field_val(); */ unsigned long long run_thread_pid; - char run_thread_comm[MAX_COMM]; + char run_thread_comm[MAX_COMM+1]; unsigned long long thread_blocking_duration; unsigned long long max_exit_idle_latency; @@ -88,7 +88,7 @@ struct timerlat_aa_data { /* * Current thread. */ - char current_comm[MAX_COMM]; + char current_comm[MAX_COMM+1]; unsigned long long current_pid; /* -- 2.52.0 The rtla tool generates various output files during testing and execution, including custom trace outputs and histogram data. These files are artifacts of running the tool with different options and should not be tracked in version control. Add gitignore entries for custom_filename.txt, osnoise_irq_noise_hist.txt, osnoise_trace.txt, and timerlat_trace.txt to prevent accidentally committing these generated files. This aligns with the existing pattern of ignoring build artifacts and generated headers like *.skel.h. Signed-off-by: Wander Lairson Costa --- tools/tracing/rtla/.gitignore | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/tracing/rtla/.gitignore b/tools/tracing/rtla/.gitignore index 1a394ad26cc1e..4d39d64ac08c0 100644 --- a/tools/tracing/rtla/.gitignore +++ b/tools/tracing/rtla/.gitignore @@ -5,3 +5,7 @@ fixdep feature FEATURE-DUMP *.skel.h +custom_filename.txt +osnoise_irq_noise_hist.txt +osnoise_trace.txt +timerlat_trace.txt -- 2.52.0 The stop_tracing global variable is accessed from both the signal handler context and the main program flow without synchronization. This creates a potential race condition where compiler optimizations could cache the variable value in registers, preventing the signal handler's updates from being visible to other parts of the program. Add the volatile qualifier to stop_tracing in both common.c and common.h to ensure all accesses to this variable bypass compiler optimizations and read directly from memory. This guarantees that when the signal handler sets stop_tracing, the change is immediately visible to the main program loop, preventing potential hangs or delayed shutdown when termination signals are received. Signed-off-by: Wander Lairson Costa --- tools/tracing/rtla/src/common.c | 2 +- tools/tracing/rtla/src/common.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/tracing/rtla/src/common.c b/tools/tracing/rtla/src/common.c index d608ffe12e7b0..1e6542a1e9630 100644 --- a/tools/tracing/rtla/src/common.c +++ b/tools/tracing/rtla/src/common.c @@ -8,7 +8,7 @@ #include "common.h" struct trace_instance *trace_inst; -int stop_tracing; +volatile int stop_tracing; static void stop_trace(int sig) { diff --git a/tools/tracing/rtla/src/common.h b/tools/tracing/rtla/src/common.h index f2c9e21c03651..283641f3e7c9b 100644 --- a/tools/tracing/rtla/src/common.h +++ b/tools/tracing/rtla/src/common.h @@ -54,7 +54,7 @@ struct osnoise_context { }; extern struct trace_instance *trace_inst; -extern int stop_tracing; +extern volatile int stop_tracing; struct hist_params { char no_irq; -- 2.52.0 Add explicit null termination and buffer initialization for read() operations in procfs_is_workload_pid() and get_self_cgroup() functions. The read() system call does not null-terminate the data it reads, and when the buffer is filled to capacity, subsequent string operations will read past the buffer boundary searching for a null terminator. In procfs_is_workload_pid(), explicitly set buffer[MAX_PATH-1] to '\0' to ensure the buffer is always null-terminated before passing it to strncmp(). In get_self_cgroup(), use memset() to zero the path buffer before reading, which ensures null termination when retval is less than MAX_PATH. Additionally, set path[MAX_PATH-1] to '\0' after the read to handle the case where the buffer is filled completely. These defensive buffer handling practices prevent potential buffer overruns and align with the ongoing buffer safety improvements across the rtla codebase. Signed-off-by: Wander Lairson Costa --- tools/tracing/rtla/src/utils.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/tracing/rtla/src/utils.c b/tools/tracing/rtla/src/utils.c index e0f31e5cae844..508b8891acd86 100644 --- a/tools/tracing/rtla/src/utils.c +++ b/tools/tracing/rtla/src/utils.c @@ -317,6 +317,7 @@ static int procfs_is_workload_pid(const char *comm_prefix, struct dirent *proc_e if (retval <= 0) return 0; + buffer[MAX_PATH-1] = '\0'; retval = strncmp(comm_prefix, buffer, strlen(comm_prefix)); if (retval) return 0; @@ -750,6 +751,7 @@ static int get_self_cgroup(char *self_cg, int sizeof_self_cg) if (fd < 0) return 0; + memset(path, 0, sizeof(path)); retval = read(fd, path, MAX_PATH); close(fd); @@ -757,6 +759,7 @@ static int get_self_cgroup(char *self_cg, int sizeof_self_cg) if (retval <= 0) return 0; + path[MAX_PATH-1] = '\0'; start = path; start = strstr(start, ":"); -- 2.52.0 Correct the return value documentation for parse_cpu_set() function in utils.c. The comment incorrectly stated that the function returns 1 on success and 0 on failure, but the actual implementation returns 0 on success and 1 on failure, following the common error-on-nonzero convention used throughout the codebase. This documentation fix ensures that developers reading the code understand the correct return value semantics and prevents potential misuse of the function's return value in conditional checks. Signed-off-by: Wander Lairson Costa --- tools/tracing/rtla/src/utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/tracing/rtla/src/utils.c b/tools/tracing/rtla/src/utils.c index 508b8891acd86..4093030e446ab 100644 --- a/tools/tracing/rtla/src/utils.c +++ b/tools/tracing/rtla/src/utils.c @@ -113,7 +113,7 @@ void get_duration(time_t start_time, char *output, int output_size) * Receives a cpu list, like 1-3,5 (cpus 1, 2, 3, 5), and then set * filling cpu_set_t argument. * - * Returns 1 on success, 0 otherwise. + * Returns 0 on success, 1 otherwise. */ int parse_cpu_set(char *cpu_list, cpu_set_t *set) { -- 2.52.0 Simplify trace_event_save_hist() and set_comm_cgroup() by computing string lengths once and storing them in local variables, rather than calling strlen() multiple times on the same unchanged strings. This makes the code clearer by eliminating redundant function calls and improving readability. In trace_event_save_hist(), the write loop previously called strlen() on the hist buffer twice per iteration for both the size calculation and loop condition. Store the length in hist_len before entering the loop. In set_comm_cgroup(), strlen() was called on cgroup_path up to three times in succession. Store the result in cg_path_len to use in both the offset calculation and size parameter for subsequent append operations. This simplification makes the code easier to read and maintain without changing program behavior. Signed-off-by: Wander Lairson Costa --- tools/tracing/rtla/src/trace.c | 6 ++++-- tools/tracing/rtla/src/utils.c | 11 +++++++---- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/tools/tracing/rtla/src/trace.c b/tools/tracing/rtla/src/trace.c index 092fcab77dc4c..223ab97e50aed 100644 --- a/tools/tracing/rtla/src/trace.c +++ b/tools/tracing/rtla/src/trace.c @@ -346,6 +346,7 @@ static void trace_event_save_hist(struct trace_instance *instance, mode_t mode = 0644; char path[MAX_PATH]; char *hist; + size_t hist_len; if (!tevent) return; @@ -376,9 +377,10 @@ static void trace_event_save_hist(struct trace_instance *instance, } index = 0; + hist_len = strlen(hist); do { - index += write(out_fd, &hist[index], strlen(hist) - index); - } while (index < strlen(hist)); + index += write(out_fd, &hist[index], hist_len - index); + } while (index < hist_len); free(hist); out_close: diff --git a/tools/tracing/rtla/src/utils.c b/tools/tracing/rtla/src/utils.c index 4093030e446ab..aee7f02b1e9b4 100644 --- a/tools/tracing/rtla/src/utils.c +++ b/tools/tracing/rtla/src/utils.c @@ -870,6 +870,7 @@ int set_comm_cgroup(const char *comm_prefix, const char *cgroup) DIR *procfs; int retval; int cg_fd; + size_t cg_path_len; if (strlen(comm_prefix) >= MAX_PATH) { err_msg("Command prefix is too long: %d < strlen(%s)\n", @@ -883,16 +884,18 @@ int set_comm_cgroup(const char *comm_prefix, const char *cgroup) return 0; } + cg_path_len = strlen(cgroup_path); + if (!cgroup) { - retval = get_self_cgroup(&cgroup_path[strlen(cgroup_path)], - sizeof(cgroup_path) - strlen(cgroup_path)); + retval = get_self_cgroup(&cgroup_path[cg_path_len], + sizeof(cgroup_path) - cg_path_len); if (!retval) { err_msg("Did not find self cgroup\n"); return 0; } } else { - snprintf(&cgroup_path[strlen(cgroup_path)], - sizeof(cgroup_path) - strlen(cgroup_path), "%s/", cgroup); + snprintf(&cgroup_path[cg_path_len], + sizeof(cgroup_path) - cg_path_len, "%s/", cgroup); } snprintf(cgroup_procs, MAX_PATH, "%s/cgroup.procs", cgroup_path); -- 2.52.0