| Message ID | mvmzggxl4n1.fsf@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rtla: fix double free | expand |
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")
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
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
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; }
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(-)