Don't call trace_instance_destroy in trace_instance_init when it fails,
this is done by the caller.
Signed-off-by: Andreas Schwab <[email protected]>
---
tools/tracing/rtla/src/trace.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/tools/tracing/rtla/src/trace.c b/tools/tracing/rtla/src/trace.c
index 5784c9f9e570..7c27fc2a52cb 100644
--- a/tools/tracing/rtla/src/trace.c
+++ b/tools/tracing/rtla/src/trace.c
@@ -179,7 +179,6 @@ int trace_instance_init(struct trace_instance *trace, char *tool_name)
return 0;
out_err:
- trace_instance_destroy(trace);
return 1;
}
--
2.37.1
--
Andreas Schwab, SUSE Labs, [email protected]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Hi Andreas
On 7/25/22 15:10, Andreas Schwab wrote:
> Don't call trace_instance_destroy in trace_instance_init when it fails,
> this is done by the caller.
Regarding the Subject, are you seeing a double-free error, or it is just an
optimization?
AFAICS, trace_instance_destroy() checks the pointers before calling free().
Why am I asking? because if it is a double-free bug, we need to add the "Fixes:"
tag, otherwise, we can think about a Subject that better describes the patch, like:
"rtla: Do not call trace_instance_destroy() twice on error"
Also, after the "subsystem:," i.e., "rlta:" we need to use capital: e.g.,
"rtla: Fix double free"
Anyways, I see that the code makes sense. I am just trying to improve the
description.
Thanks!
-- Daniel
On Jul 25 2022, Daniel Bristot de Oliveira wrote:
> Hi Andreas
>
> On 7/25/22 15:10, Andreas Schwab wrote:
>> Don't call trace_instance_destroy in trace_instance_init when it fails,
>> this is done by the caller.
>
> Regarding the Subject, are you seeing a double-free error, or it is just an
> optimization?
A double free nowadays is almost always an error, due to better malloc
checking.
> AFAICS, trace_instance_destroy() checks the pointers before calling free().
That doesn't help when the pointer is not cleared afterwards. Do you
prefer that?
> Why am I asking? because if it is a double-free bug, we need to add the "Fixes:"
> tag,
It's the first time I tried running rtla, so I don't know whether it is
a regression, but from looking at the history it appears to have been
introduced already in commit 0605bf009f18 ("rtla: Add osnoise tool")
--
Andreas Schwab, SUSE Labs, [email protected]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
On Mon, 25 Jul 2022 15:46:40 +0200
Andreas Schwab <[email protected]> wrote:
> On Jul 25 2022, Daniel Bristot de Oliveira wrote:
>
> > Hi Andreas
> >
> > On 7/25/22 15:10, Andreas Schwab wrote:
> >> Don't call trace_instance_destroy in trace_instance_init when it fails,
> >> this is done by the caller.
> >
> > Regarding the Subject, are you seeing a double-free error, or it is just an
> > optimization?
>
> A double free nowadays is almost always an error, due to better malloc
> checking.
>
> > AFAICS, trace_instance_destroy() checks the pointers before calling free().
>
> That doesn't help when the pointer is not cleared afterwards. Do you
> prefer that?
>
> > Why am I asking? because if it is a double-free bug, we need to add the "Fixes:"
> > tag,
>
> It's the first time I tried running rtla, so I don't know whether it is
> a regression, but from looking at the history it appears to have been
> introduced already in commit 0605bf009f18 ("rtla: Add osnoise tool")
>
I think the real fix is to make trace_instance_destroy() be able to be
called more than once.
void trace_instance_destroy(struct trace_instance *trace)
{
if (trace->inst) {
disable_tracer(trace->inst);
destroy_instance(trace->inst);
trace->inst = NULL;
}
if (trace->seq) {
free(trace->seq);
trace->seq = NULL;
}
if (trace->tep) {
tep_free(trace->tep);
trace->tep = NULL;
}
}
As trace_instance_init() is doing the above allocations, it should clean it
up on error. But I also agree, this will lead to double free without
changing trace_instance_destroy() to be the above and then calling it twice.
-- Steve
Avoid double free by making trace_instance_destroy indempotent. When
trace_instance_init fails, it calls trace_instance_destroy, but its only
caller osnoise_destroy_tool calls it again.
Fixes: 0605bf009f18 ("rtla: Add osnoise tool")
Signed-off-by: Andreas Schwab <[email protected]>
---
tools/tracing/rtla/src/trace.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/tools/tracing/rtla/src/trace.c b/tools/tracing/rtla/src/trace.c
index 5784c9f9e570..e1ba6d9f4265 100644
--- a/tools/tracing/rtla/src/trace.c
+++ b/tools/tracing/rtla/src/trace.c
@@ -134,13 +134,18 @@ void trace_instance_destroy(struct trace_instance *trace)
if (trace->inst) {
disable_tracer(trace->inst);
destroy_instance(trace->inst);
+ trace->inst = NULL;
}
- if (trace->seq)
+ if (trace->seq) {
free(trace->seq);
+ trace->seq = NULL;
+ }
- if (trace->tep)
+ if (trace->tep) {
tep_free(trace->tep);
+ trace->tep = NULL;
+ }
}
/*
--
2.37.1
--
Andreas Schwab, SUSE Labs, [email protected]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
On 7/25/22 16:56, Steven Rostedt wrote:
> On Mon, 25 Jul 2022 15:46:40 +0200
> Andreas Schwab <[email protected]> wrote:
>
>> On Jul 25 2022, Daniel Bristot de Oliveira wrote:
>>
>>> Hi Andreas
>>>
>>> On 7/25/22 15:10, Andreas Schwab wrote:
>>>> Don't call trace_instance_destroy in trace_instance_init when it fails,
>>>> this is done by the caller.
>>>
>>> Regarding the Subject, are you seeing a double-free error, or it is just an
>>> optimization?
>>
>> A double free nowadays is almost always an error, due to better malloc
>> checking.
>>
>>> AFAICS, trace_instance_destroy() checks the pointers before calling free().
>>
>> That doesn't help when the pointer is not cleared afterwards. Do you
>> prefer that?
>>
>>> Why am I asking? because if it is a double-free bug, we need to add the "Fixes:"
>>> tag,
>>
>> It's the first time I tried running rtla, so I don't know whether it is
>> a regression, but from looking at the history it appears to have been
>> introduced already in commit 0605bf009f18 ("rtla: Add osnoise tool")
>>
>
> I think the real fix is to make trace_instance_destroy() be able to be
> called more than once.
>
> void trace_instance_destroy(struct trace_instance *trace)
> {
> if (trace->inst) {
> disable_tracer(trace->inst);
> destroy_instance(trace->inst);
> trace->inst = NULL
ah! right, it was missing this... ^^^
-- Daniel
On 7/25/22 17:12, Andreas Schwab wrote:
> Avoid double free by making trace_instance_destroy indempotent. When
> trace_instance_init fails, it calls trace_instance_destroy, but its only
> caller osnoise_destroy_tool calls it again.
>
> Fixes: 0605bf009f18 ("rtla: Add osnoise tool")
> Signed-off-by: Andreas Schwab <[email protected]>
Acked-by: Daniel Bristot de Oliveira <[email protected]>
Thanks!
-- Daniel
On Jul 25 2022, Andreas Schwab wrote:
> Avoid double free by making trace_instance_destroy indempotent. When
s/indempotent/idempotent/
--
Andreas Schwab, SUSE Labs, [email protected]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."