rtla: Code robustness and maintainability improvements

[rtla 01/13] rtla: Check for memory allocation failures

Posted by Wander Lairson Costa 2 weeks ago

The actions_init() and actions_new() functions did not check the
return value of calloc() and realloc() respectively. In a low
memory situation, this could lead to a NULL pointer dereference.

Add checks for the return value of memory allocation functions
and return an error in case of failure. Update the callers to
handle the error properly.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/actions.c       | 26 +++++++++++++++++++++++---
 tools/tracing/rtla/src/actions.h       |  2 +-
 tools/tracing/rtla/src/timerlat_hist.c |  7 +++++--
 tools/tracing/rtla/src/timerlat_top.c  |  7 +++++--
 4 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c
index 8945aee58d511..01648a1425c10 100644
--- a/tools/tracing/rtla/src/actions.c
+++ b/tools/tracing/rtla/src/actions.c
@@ -11,11 +11,13 @@
 /*
  * actions_init - initialize struct actions
  */
-void
+int
 actions_init(struct actions *self)
 {
 	self->size = action_default_size;
 	self->list = calloc(self->size, sizeof(struct action));
+	if (!self->list)
+		return -1;
 	self->len = 0;
 	self->continue_flag = false;
 
@@ -23,6 +25,7 @@ actions_init(struct actions *self)
 
 	/* This has to be set by the user */
 	self->trace_output_inst = NULL;
+	return 0;
 }
 
 /*
@@ -50,8 +53,13 @@ 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;
+		void *p = reallocarray(self->list, new_size, sizeof(struct action));
+
+		if (!p)
+			return NULL;
+		self->list = p;
+		self->size = new_size;
 	}
 
 	return &self->list[self->len++];
@@ -65,6 +73,9 @@ actions_add_trace_output(struct actions *self, const char *trace_output)
 {
 	struct action *action = actions_new(self);
 
+	if (!action)
+		return -1;
+
 	self->present[ACTION_TRACE_OUTPUT] = true;
 	action->type = ACTION_TRACE_OUTPUT;
 	action->trace_output = calloc(strlen(trace_output) + 1, sizeof(char));
@@ -83,6 +94,9 @@ actions_add_signal(struct actions *self, int signal, int pid)
 {
 	struct action *action = actions_new(self);
 
+	if (!action)
+		return -1;
+
 	self->present[ACTION_SIGNAL] = true;
 	action->type = ACTION_SIGNAL;
 	action->signal = signal;
@@ -99,6 +113,9 @@ actions_add_shell(struct actions *self, const char *command)
 {
 	struct action *action = actions_new(self);
 
+	if (!action)
+		return -1;
+
 	self->present[ACTION_SHELL] = true;
 	action->type = ACTION_SHELL;
 	action->command = calloc(strlen(command) + 1, sizeof(char));
@@ -117,6 +134,9 @@ actions_add_continue(struct actions *self)
 {
 	struct action *action = actions_new(self);
 
+	if (!action)
+		return -1;
+
 	self->present[ACTION_CONTINUE] = true;
 	action->type = ACTION_CONTINUE;
 
diff --git a/tools/tracing/rtla/src/actions.h b/tools/tracing/rtla/src/actions.h
index a4f9b570775b5..439bcc58ac93a 100644
--- a/tools/tracing/rtla/src/actions.h
+++ b/tools/tracing/rtla/src/actions.h
@@ -42,7 +42,7 @@ struct actions {
 	struct tracefs_instance *trace_output_inst;
 };
 
-void actions_init(struct actions *self);
+int 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);
diff --git a/tools/tracing/rtla/src/timerlat_hist.c b/tools/tracing/rtla/src/timerlat_hist.c
index 606c1688057b2..09a3da3f58630 100644
--- a/tools/tracing/rtla/src/timerlat_hist.c
+++ b/tools/tracing/rtla/src/timerlat_hist.c
@@ -798,8 +798,11 @@ static struct common_params
 	if (!params)
 		exit(1);
 
-	actions_init(&params->common.threshold_actions);
-	actions_init(&params->common.end_actions);
+	if (actions_init(&params->common.threshold_actions) ||
+	    actions_init(&params->common.end_actions)) {
+		err_msg("Error initializing actions");
+		exit(EXIT_FAILURE);
+	}
 
 	/* disabled by default */
 	params->dma_latency = -1;
diff --git a/tools/tracing/rtla/src/timerlat_top.c b/tools/tracing/rtla/src/timerlat_top.c
index fc479a0dcb597..7679820e72db5 100644
--- a/tools/tracing/rtla/src/timerlat_top.c
+++ b/tools/tracing/rtla/src/timerlat_top.c
@@ -556,8 +556,11 @@ static struct common_params
 	if (!params)
 		exit(1);
 
-	actions_init(&params->common.threshold_actions);
-	actions_init(&params->common.end_actions);
+	if (actions_init(&params->common.threshold_actions) ||
+	    actions_init(&params->common.end_actions)) {
+		err_msg("Error initializing actions");
+		exit(EXIT_FAILURE);
+	}
 
 	/* disabled by default */
 	params->dma_latency = -1;
-- 
2.51.1

Re: [rtla 01/13] rtla: Check for memory allocation failures

Posted by Costa Shulyupin 3 days, 13 hours ago

On Mon, 17 Nov 2025 at 20:54, Wander Lairson Costa <wander@redhat.com> wrote:
> Add checks for the return value of memory allocation functions
> and return an error in case of failure. Update the callers to
> handle the error properly.

Would you like to consider using fatal("Out of memory") instead of
returning an error code?
Anyway there is no work around for out of memory.

Costa

Re: [rtla 01/13] rtla: Check for memory allocation failures

Posted by Wander Lairson Costa 3 days, 12 hours ago

On Fri, Nov 28, 2025 at 10:30 AM Costa Shulyupin <costa.shul@redhat.com> wrote:
>
> On Mon, 17 Nov 2025 at 20:54, Wander Lairson Costa <wander@redhat.com> wrote:
> > Add checks for the return value of memory allocation functions
> > and return an error in case of failure. Update the callers to
> > handle the error properly.
>
> Would you like to consider using fatal("Out of memory") instead of
> returning an error code?
> Anyway there is no work around for out of memory.
>

Good idea. I am going to update the patches.

> Costa
>

Re: [rtla 01/13] rtla: Check for memory allocation failures

Posted by Masami Hiramatsu (Google) 2 weeks ago

On Mon, 17 Nov 2025 15:41:08 -0300
Wander Lairson Costa <wander@redhat.com> wrote:

> The actions_init() and actions_new() functions did not check the
> return value of calloc() and realloc() respectively. In a low
> memory situation, this could lead to a NULL pointer dereference.
> 
> Add checks for the return value of memory allocation functions
> and return an error in case of failure. Update the callers to
> handle the error properly.
> 
> Signed-off-by: Wander Lairson Costa <wander@redhat.com>
> ---
>  tools/tracing/rtla/src/actions.c       | 26 +++++++++++++++++++++++---
>  tools/tracing/rtla/src/actions.h       |  2 +-
>  tools/tracing/rtla/src/timerlat_hist.c |  7 +++++--
>  tools/tracing/rtla/src/timerlat_top.c  |  7 +++++--
>  4 files changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c
> index 8945aee58d511..01648a1425c10 100644
> --- a/tools/tracing/rtla/src/actions.c
> +++ b/tools/tracing/rtla/src/actions.c
> @@ -11,11 +11,13 @@
>  /*
>   * actions_init - initialize struct actions
>   */
> -void
> +int
>  actions_init(struct actions *self)
>  {
>  	self->size = action_default_size;
>  	self->list = calloc(self->size, sizeof(struct action));
> +	if (!self->list)
> +		return -1;

Can you return -ENOMEM?

>  	self->len = 0;
>  	self->continue_flag = false;
>  
> @@ -23,6 +25,7 @@ actions_init(struct actions *self)
>  
>  	/* This has to be set by the user */
>  	self->trace_output_inst = NULL;
> +	return 0;
>  }
>  
>  /*
> @@ -50,8 +53,13 @@ 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;
> +		void *p = reallocarray(self->list, new_size, sizeof(struct action));
> +
> +		if (!p)
> +			return NULL;
> +		self->list = p;
> +		self->size = new_size;
>  	}
>  
>  	return &self->list[self->len++];
> @@ -65,6 +73,9 @@ actions_add_trace_output(struct actions *self, const char *trace_output)
>  {
>  	struct action *action = actions_new(self);
>  
> +	if (!action)
> +		return -1;

I think !action should return -ENOMEM too.

> +
>  	self->present[ACTION_TRACE_OUTPUT] = true;
>  	action->type = ACTION_TRACE_OUTPUT;
>  	action->trace_output = calloc(strlen(trace_output) + 1, sizeof(char));
> @@ -83,6 +94,9 @@ actions_add_signal(struct actions *self, int signal, int pid)
>  {
>  	struct action *action = actions_new(self);
>  
> +	if (!action)
> +		return -1;
> +
>  	self->present[ACTION_SIGNAL] = true;
>  	action->type = ACTION_SIGNAL;
>  	action->signal = signal;
> @@ -99,6 +113,9 @@ actions_add_shell(struct actions *self, const char *command)
>  {
>  	struct action *action = actions_new(self);
>  
> +	if (!action)
> +		return -1;
> +
>  	self->present[ACTION_SHELL] = true;
>  	action->type = ACTION_SHELL;
>  	action->command = calloc(strlen(command) + 1, sizeof(char));
> @@ -117,6 +134,9 @@ actions_add_continue(struct actions *self)
>  {
>  	struct action *action = actions_new(self);
>  
> +	if (!action)
> +		return -1;
> +
>  	self->present[ACTION_CONTINUE] = true;
>  	action->type = ACTION_CONTINUE;
>  

The above same patterns too.

Thank you,



-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [rtla 01/13] rtla: Check for memory allocation failures

Posted by Steven Rostedt 1 week, 6 days ago

On Tue, 18 Nov 2025 11:09:46 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Mon, 17 Nov 2025 15:41:08 -0300
> Wander Lairson Costa <wander@redhat.com> wrote:
> 
> > The actions_init() and actions_new() functions did not check the
> > return value of calloc() and realloc() respectively. In a low
> > memory situation, this could lead to a NULL pointer dereference.
> > 
> > Add checks for the return value of memory allocation functions
> > and return an error in case of failure. Update the callers to
> > handle the error properly.
> > 
> > Signed-off-by: Wander Lairson Costa <wander@redhat.com>
> > ---
> >  tools/tracing/rtla/src/actions.c       | 26 +++++++++++++++++++++++---
> >  tools/tracing/rtla/src/actions.h       |  2 +-
> >  tools/tracing/rtla/src/timerlat_hist.c |  7 +++++--
> >  tools/tracing/rtla/src/timerlat_top.c  |  7 +++++--
> >  4 files changed, 34 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c
> > index 8945aee58d511..01648a1425c10 100644
> > --- a/tools/tracing/rtla/src/actions.c
> > +++ b/tools/tracing/rtla/src/actions.c
> > @@ -11,11 +11,13 @@
> >  /*
> >   * actions_init - initialize struct actions
> >   */
> > -void
> > +int
> >  actions_init(struct actions *self)
> >  {
> >  	self->size = action_default_size;
> >  	self->list = calloc(self->size, sizeof(struct action));
> > +	if (!self->list)
> > +		return -1;  
> 
> Can you return -ENOMEM?

Does it need to? This is user space not the kernel. Errno is already
set by calloc() failing.

-- Steve

Re: [rtla 01/13] rtla: Check for memory allocation failures

Posted by Masami Hiramatsu (Google) 1 week, 6 days ago

On Mon, 17 Nov 2025 22:06:15 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 18 Nov 2025 11:09:46 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > On Mon, 17 Nov 2025 15:41:08 -0300
> > Wander Lairson Costa <wander@redhat.com> wrote:
> > 
> > > The actions_init() and actions_new() functions did not check the
> > > return value of calloc() and realloc() respectively. In a low
> > > memory situation, this could lead to a NULL pointer dereference.
> > > 
> > > Add checks for the return value of memory allocation functions
> > > and return an error in case of failure. Update the callers to
> > > handle the error properly.
> > > 
> > > Signed-off-by: Wander Lairson Costa <wander@redhat.com>
> > > ---
> > >  tools/tracing/rtla/src/actions.c       | 26 +++++++++++++++++++++++---
> > >  tools/tracing/rtla/src/actions.h       |  2 +-
> > >  tools/tracing/rtla/src/timerlat_hist.c |  7 +++++--
> > >  tools/tracing/rtla/src/timerlat_top.c  |  7 +++++--
> > >  4 files changed, 34 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c
> > > index 8945aee58d511..01648a1425c10 100644
> > > --- a/tools/tracing/rtla/src/actions.c
> > > +++ b/tools/tracing/rtla/src/actions.c
> > > @@ -11,11 +11,13 @@
> > >  /*
> > >   * actions_init - initialize struct actions
> > >   */
> > > -void
> > > +int
> > >  actions_init(struct actions *self)
> > >  {
> > >  	self->size = action_default_size;
> > >  	self->list = calloc(self->size, sizeof(struct action));
> > > +	if (!self->list)
> > > +		return -1;  
> > 
> > Can you return -ENOMEM?
> 
> Does it need to? This is user space not the kernel. Errno is already
> set by calloc() failing.

Ah, indeed! I agree to just return -1.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thank you,

> 
> -- Steve
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>