Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 25 Jul 2022 10:56:39 -0400
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Andreas Schwab <schwab@suse.de>
Cc:     Daniel Bristot de Oliveira <bristot@kernel.org>,
        linux-trace-devel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] rtla: fix double free
Message-ID: <20220725105639.295a7d7d@gandalf.local.home>
In-Reply-To: <mvmv8rll2yn.fsf@suse.de>
References: <mvmzggxl4n1.fsf@suse.de>
        <fd0888a4-099c-95fe-7e20-82be1489061f@kernel.org>
        <mvmv8rll2yn.fsf@suse.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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