| Message ID | f964ed1f-64d2-4fde-ad3e-708331f8f358@stanley.mountain (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 90574d2a675947858b47008df8d07f75ea50d0d0 |
| Headers | show |
| Series | rtla/osnoise: prevent NULL dereference in error handling | expand |
On Fri, 9 Aug 2024, Dan Carpenter wrote: > If the "tool->data" allocation fails then there is no need to call > osnoise_free_top() and, in fact, doing so will lead to a NULL dereference. > > Fixes: 1eceb2fc2ca5 ("rtla/osnoise: Add osnoise top mode") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > tools/tracing/rtla/src/osnoise_top.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/tools/tracing/rtla/src/osnoise_top.c b/tools/tracing/rtla/src/osnoise_top.c > index f594a44df840..2f756628613d 100644 > --- a/tools/tracing/rtla/src/osnoise_top.c > +++ b/tools/tracing/rtla/src/osnoise_top.c > @@ -651,8 +651,10 @@ struct osnoise_tool *osnoise_init_top(struct osnoise_top_params *params) > return NULL; > > tool->data = osnoise_alloc_top(nr_cpus); > - if (!tool->data) > - goto out_err; > + if (!tool->data) { > + osnoise_destroy_tool(tool); > + return NULL; > + } > > tool->params = params; > > @@ -660,11 +662,6 @@ struct osnoise_tool *osnoise_init_top(struct osnoise_top_params *params) > osnoise_top_handler, NULL); > > return tool; > - > -out_err: > - osnoise_free_top(tool->data); > - osnoise_destroy_tool(tool); > - return NULL; > } > > static int stop_tracing; > -- Although your fix appears to be correct, I wonder if it would be better to create a second error label, such as out_destroy_tool: as described in section 7 of the coding-style.rst Thanks John Kacur
On Fri, 9 Aug 2024 13:34:28 -0400 (EDT) John Kacur <jkacur@redhat.com> wrote: > On Fri, 9 Aug 2024, Dan Carpenter wrote: > > > If the "tool->data" allocation fails then there is no need to call > > osnoise_free_top() and, in fact, doing so will lead to a NULL dereference. > > > > Fixes: 1eceb2fc2ca5 ("rtla/osnoise: Add osnoise top mode") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > tools/tracing/rtla/src/osnoise_top.c | 11 ++++------- > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > diff --git a/tools/tracing/rtla/src/osnoise_top.c b/tools/tracing/rtla/src/osnoise_top.c > > index f594a44df840..2f756628613d 100644 > > --- a/tools/tracing/rtla/src/osnoise_top.c > > +++ b/tools/tracing/rtla/src/osnoise_top.c > > @@ -651,8 +651,10 @@ struct osnoise_tool *osnoise_init_top(struct osnoise_top_params *params) > > return NULL; > > > > tool->data = osnoise_alloc_top(nr_cpus); > > - if (!tool->data) > > - goto out_err; > > + if (!tool->data) { > > + osnoise_destroy_tool(tool); > > + return NULL; > > + } > > > > tool->params = params; > > > > @@ -660,11 +662,6 @@ struct osnoise_tool *osnoise_init_top(struct osnoise_top_params *params) > > osnoise_top_handler, NULL); > > > > return tool; > > - > > -out_err: > > - osnoise_free_top(tool->data); > > - osnoise_destroy_tool(tool); > > - return NULL; > > } > > > > static int stop_tracing; > > -- > > Although your fix appears to be correct, I wonder if it would be better to > create a second error label, such as out_destroy_tool: as described in > section 7 of the coding-style.rst > There's no reason for that. It's the only error path. That is, nothing would jump to the original out_err: And for a single error, an if statement is good enough. -- Steve
On Fri, 9 Aug 2024, Steven Rostedt wrote: > On Fri, 9 Aug 2024 13:34:28 -0400 (EDT) > John Kacur <jkacur@redhat.com> wrote: > > > On Fri, 9 Aug 2024, Dan Carpenter wrote: > > > > > If the "tool->data" allocation fails then there is no need to call > > > osnoise_free_top() and, in fact, doing so will lead to a NULL dereference. > > > > > > Fixes: 1eceb2fc2ca5 ("rtla/osnoise: Add osnoise top mode") > > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > > --- > > > tools/tracing/rtla/src/osnoise_top.c | 11 ++++------- > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > diff --git a/tools/tracing/rtla/src/osnoise_top.c b/tools/tracing/rtla/src/osnoise_top.c > > > index f594a44df840..2f756628613d 100644 > > > --- a/tools/tracing/rtla/src/osnoise_top.c > > > +++ b/tools/tracing/rtla/src/osnoise_top.c > > > @@ -651,8 +651,10 @@ struct osnoise_tool *osnoise_init_top(struct osnoise_top_params *params) > > > return NULL; > > > > > > tool->data = osnoise_alloc_top(nr_cpus); > > > - if (!tool->data) > > > - goto out_err; > > > + if (!tool->data) { > > > + osnoise_destroy_tool(tool); > > > + return NULL; > > > + } > > > > > > tool->params = params; > > > > > > @@ -660,11 +662,6 @@ struct osnoise_tool *osnoise_init_top(struct osnoise_top_params *params) > > > osnoise_top_handler, NULL); > > > > > > return tool; > > > - > > > -out_err: > > > - osnoise_free_top(tool->data); > > > - osnoise_destroy_tool(tool); > > > - return NULL; > > > } > > > > > > static int stop_tracing; > > > -- > > > > Although your fix appears to be correct, I wonder if it would be better to > > create a second error label, such as out_destroy_tool: as described in > > section 7 of the coding-style.rst > > > > There's no reason for that. It's the only error path. That is, nothing > would jump to the original out_err: > > And for a single error, an if statement is good enough. > > -- Steve > > Ah, right of course. Okay in that case, Signed-off-by: John Kacur <jkacur@redhat.com> (applied the patch, built and ran)
On Fri, 9 Aug 2024 13:53:33 -0400 (EDT) John Kacur <jkacur@redhat.com> wrote: > > > Although your fix appears to be correct, I wonder if it would be better to > > > create a second error label, such as out_destroy_tool: as described in > > > section 7 of the coding-style.rst > > > > > > > There's no reason for that. It's the only error path. That is, nothing > > would jump to the original out_err: > > > > And for a single error, an if statement is good enough. > > > > -- Steve > > > > > > Ah, right of course. > Okay in that case, Signed-off-by: John Kacur <jkacur@redhat.com> > (applied the patch, built and ran) Note, "Signed-off-by" is for the author of a patch or someone pushing it through their tree. I believe you want either "Acked-by" or "Reviewed-by", and since you ran it you could also add "Tested-by". -- Steve
On Fri, 9 Aug 2024, Steven Rostedt wrote: > On Fri, 9 Aug 2024 13:53:33 -0400 (EDT) > John Kacur <jkacur@redhat.com> wrote: > > > > > > Although your fix appears to be correct, I wonder if it would be better to > > > > create a second error label, such as out_destroy_tool: as described in > > > > section 7 of the coding-style.rst > > > > > > > > > > There's no reason for that. It's the only error path. That is, nothing > > > would jump to the original out_err: > > > > > > And for a single error, an if statement is good enough. > > > > > > -- Steve > > > > > > > > > > Ah, right of course. > > Okay in that case, Signed-off-by: John Kacur <jkacur@redhat.com> > > (applied the patch, built and ran) > > Note, "Signed-off-by" is for the author of a patch or someone pushing it > through their tree. I believe you want either "Acked-by" or "Reviewed-by", > and since you ran it you could also add "Tested-by". > > -- Steve Thanks Steve, Reviewed-by: John Kacur <jkacur@redhat.com> Tested-by: John Kacur <jkacur@redhat.com>
On Fri, Aug 09, 2024 at 03:34:30PM +0300, Dan Carpenter wrote: > If the "tool->data" allocation fails then there is no need to call > osnoise_free_top() and, in fact, doing so will lead to a NULL dereference. > > Fixes: 1eceb2fc2ca5 ("rtla/osnoise: Add osnoise top mode") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > tools/tracing/rtla/src/osnoise_top.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) Looks good to me. Reviewed-by: Luis Claudio R. Goncalves <lgoncalv@redhat.com> > > diff --git a/tools/tracing/rtla/src/osnoise_top.c b/tools/tracing/rtla/src/osnoise_top.c > index f594a44df840..2f756628613d 100644 > --- a/tools/tracing/rtla/src/osnoise_top.c > +++ b/tools/tracing/rtla/src/osnoise_top.c > @@ -651,8 +651,10 @@ struct osnoise_tool *osnoise_init_top(struct osnoise_top_params *params) > return NULL; > > tool->data = osnoise_alloc_top(nr_cpus); > - if (!tool->data) > - goto out_err; > + if (!tool->data) { > + osnoise_destroy_tool(tool); > + return NULL; > + } > > tool->params = params; > > @@ -660,11 +662,6 @@ struct osnoise_tool *osnoise_init_top(struct osnoise_top_params *params) > osnoise_top_handler, NULL); > > return tool; > - > -out_err: > - osnoise_free_top(tool->data); > - osnoise_destroy_tool(tool); > - return NULL; > } > > static int stop_tracing; > -- > 2.43.0 > > ---end quoted text---
diff --git a/tools/tracing/rtla/src/osnoise_top.c b/tools/tracing/rtla/src/osnoise_top.c index f594a44df840..2f756628613d 100644 --- a/tools/tracing/rtla/src/osnoise_top.c +++ b/tools/tracing/rtla/src/osnoise_top.c @@ -651,8 +651,10 @@ struct osnoise_tool *osnoise_init_top(struct osnoise_top_params *params) return NULL; tool->data = osnoise_alloc_top(nr_cpus); - if (!tool->data) - goto out_err; + if (!tool->data) { + osnoise_destroy_tool(tool); + return NULL; + } tool->params = params; @@ -660,11 +662,6 @@ struct osnoise_tool *osnoise_init_top(struct osnoise_top_params *params) osnoise_top_handler, NULL); return tool; - -out_err: - osnoise_free_top(tool->data); - osnoise_destroy_tool(tool); - return NULL; } static int stop_tracing;
If the "tool->data" allocation fails then there is no need to call osnoise_free_top() and, in fact, doing so will lead to a NULL dereference. Fixes: 1eceb2fc2ca5 ("rtla/osnoise: Add osnoise top mode") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- tools/tracing/rtla/src/osnoise_top.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)