[PATCH] rtla: fix double free

[PATCH] rtla: fix double free

[Andreas Schwab]

Don't call trace_instance_destroy in trace_instance_init when it fails,
this is done by the caller.

Signed-off-by: Andreas Schwab <schwab@suse.de>
---
 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, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] rtla: fix double free

[Daniel Bristot de Oliveira]

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

Re: [PATCH] rtla: fix double free

[Andreas Schwab]

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, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] rtla: fix double free

[Steven Rostedt]

On Mon, 25 Jul 2022 15:46:40 +0200
Andreas Schwab <schwab@suse.de> 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

[PATCH v2] rtla: Fix double free

[Andreas Schwab]

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 <schwab@suse.de>
---
 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, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v2] rtla: Fix double free

[Andreas Schwab]

On Jul 25 2022, Andreas Schwab wrote:

> Avoid double free by making trace_instance_destroy indempotent.  When

s/indempotent/idempotent/

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] rtla: fix double free

[Daniel Bristot de Oliveira]

On 7/25/22 16:56, Steven Rostedt wrote:
> On Mon, 25 Jul 2022 15:46:40 +0200
> Andreas Schwab <schwab@suse.de> 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

Re: [PATCH v2] rtla: Fix double free

[Daniel Bristot de Oliveira]

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 <schwab@suse.de>

Acked-by: Daniel Bristot de Oliveira <bristot@kernel.org>

Thanks!
-- Daniel