[lttng-dev] [PATCH babeltrace] Fix: Double free in bt_context_remove_trace().

[lttng-dev] [PATCH babeltrace] Fix: Double free in bt_context_remove_trace().

[Jérémie Galarneau]

ctf_close_trace was being called twice when calling bt_context_remove_trace thus
causing free() to be called on an invalid pointer.

Calling bt_context_remove_trace() would call ctf_close_trace() once via the
close_handle callback registered on the ctf format struct and a second call would
take place from bt_trace_handle_destroy() which is registered as the
value_destroy_func on the trace_handles hash table of the current context.

The first explicit call to handle->format->close_trace is unnecessary.

The crash is reproducible by invoking the tests-python.py script.

Signed-off-by: J?r?mie Galarneau <jeremie.galarneau at efficios.com>
---
 lib/context.c      | 8 +-------
 lib/trace-handle.c | 5 ++++-
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/lib/context.c b/lib/context.c
index 5516e49..05fb994 100644
--- a/lib/context.c
+++ b/lib/context.c
@@ -156,7 +156,6 @@ end:
 int bt_context_remove_trace(struct bt_context *ctx, int handle_id)
 {
 	struct bt_trace_handle *handle;
-	int ret;
 
 	if (!ctx)
 		return -EINVAL;
@@ -168,12 +167,7 @@ int bt_context_remove_trace(struct bt_context *ctx, int handle_id)
 
 	/* Remove from containers */
 	trace_collection_remove(ctx->tc, handle->td);
-	/* Close the trace */
-	ret = handle->format->close_trace(handle->td);
-	if (ret) {
-		fprintf(stderr, "Error in close_trace callback\n");
-		return ret;
-	}
+
 	/* Remove and free the handle */
 	g_hash_table_remove(ctx->trace_handles,
 			(gpointer) (unsigned long) handle_id);
diff --git a/lib/trace-handle.c b/lib/trace-handle.c
index 0da565b..17a6e0d 100644
--- a/lib/trace-handle.c
+++ b/lib/trace-handle.c
@@ -49,7 +49,10 @@ struct bt_trace_handle *bt_trace_handle_create(struct bt_context *ctx)
 
 void bt_trace_handle_destroy(struct bt_trace_handle *th)
 {
-	th->format->close_trace(th->td);
+	if (th->format->close_trace(th->td)) {
+		fprintf(stderr, "Error in close_trace callback\n");
+	}
+
 	g_free(th);
 }
 
-- 
1.8.1.1

[lttng-dev] [PATCH babeltrace] Fix: Double free in bt_context_remove_trace().

[Mathieu Desnoyers]

* J?r?mie Galarneau (jeremie.galarneau at efficios.com) wrote:
> ctf_close_trace was being called twice when calling bt_context_remove_trace thus
> causing free() to be called on an invalid pointer.
> 
> Calling bt_context_remove_trace() would call ctf_close_trace() once via the
> close_handle callback registered on the ctf format struct and a second call would
> take place from bt_trace_handle_destroy() which is registered as the
> value_destroy_func on the trace_handles hash table of the current context.
> 
> The first explicit call to handle->format->close_trace is unnecessary.
> 
> The crash is reproducible by invoking the tests-python.py script.

I think we should leave the close_trace calls in both locations, but set
handle->td to -1 when we close it, and test for negative handle ID
before doing the close.

Thanks,

Mathieu

> 
> Signed-off-by: J?r?mie Galarneau <jeremie.galarneau at efficios.com>
> ---
>  lib/context.c      | 8 +-------
>  lib/trace-handle.c | 5 ++++-
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/context.c b/lib/context.c
> index 5516e49..05fb994 100644
> --- a/lib/context.c
> +++ b/lib/context.c
> @@ -156,7 +156,6 @@ end:
>  int bt_context_remove_trace(struct bt_context *ctx, int handle_id)
>  {
>  	struct bt_trace_handle *handle;
> -	int ret;
>  
>  	if (!ctx)
>  		return -EINVAL;
> @@ -168,12 +167,7 @@ int bt_context_remove_trace(struct bt_context *ctx, int handle_id)
>  
>  	/* Remove from containers */
>  	trace_collection_remove(ctx->tc, handle->td);
> -	/* Close the trace */
> -	ret = handle->format->close_trace(handle->td);
> -	if (ret) {
> -		fprintf(stderr, "Error in close_trace callback\n");
> -		return ret;
> -	}
> +
>  	/* Remove and free the handle */
>  	g_hash_table_remove(ctx->trace_handles,
>  			(gpointer) (unsigned long) handle_id);
> diff --git a/lib/trace-handle.c b/lib/trace-handle.c
> index 0da565b..17a6e0d 100644
> --- a/lib/trace-handle.c
> +++ b/lib/trace-handle.c
> @@ -49,7 +49,10 @@ struct bt_trace_handle *bt_trace_handle_create(struct bt_context *ctx)
>  
>  void bt_trace_handle_destroy(struct bt_trace_handle *th)
>  {
> -	th->format->close_trace(th->td);
> +	if (th->format->close_trace(th->td)) {
> +		fprintf(stderr, "Error in close_trace callback\n");
> +	}
> +
>  	g_free(th);
>  }
>  
> -- 
> 1.8.1.1
> 
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

[lttng-dev] [PATCH babeltrace] Fix: Double free in bt_context_remove_trace().

[Julien Desfossez]

On 21/01/13 02:32 PM, Mathieu Desnoyers wrote:
> * J?r?mie Galarneau (jeremie.galarneau at efficios.com) wrote:
>> ctf_close_trace was being called twice when calling bt_context_remove_trace thus
>> causing free() to be called on an invalid pointer.
>>
>> Calling bt_context_remove_trace() would call ctf_close_trace() once via the
>> close_handle callback registered on the ctf format struct and a second call would
>> take place from bt_trace_handle_destroy() which is registered as the
>> value_destroy_func on the trace_handles hash table of the current context.
>>
>> The first explicit call to handle->format->close_trace is unnecessary.
>>
>> The crash is reproducible by invoking the tests-python.py script.
> 
> I think we should leave the close_trace calls in both locations, but set
> handle->td to -1 when we close it, and test for negative handle ID
> before doing the close.

I agree, we need the two calls to close, one is called when we remove a
trace manually and the other one is called when we destroy the context.
Setting and testing the td to -1 will solve the problem and will avoid
calling close_trace at all if not needed.

Thanks,

Julien

> 
> Thanks,
> 
> Mathieu
> 
>>
>> Signed-off-by: J?r?mie Galarneau <jeremie.galarneau at efficios.com>
>> ---
>>  lib/context.c      | 8 +-------
>>  lib/trace-handle.c | 5 ++++-
>>  2 files changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/lib/context.c b/lib/context.c
>> index 5516e49..05fb994 100644
>> --- a/lib/context.c
>> +++ b/lib/context.c
>> @@ -156,7 +156,6 @@ end:
>>  int bt_context_remove_trace(struct bt_context *ctx, int handle_id)
>>  {
>>  	struct bt_trace_handle *handle;
>> -	int ret;
>>  
>>  	if (!ctx)
>>  		return -EINVAL;
>> @@ -168,12 +167,7 @@ int bt_context_remove_trace(struct bt_context *ctx, int handle_id)
>>  
>>  	/* Remove from containers */
>>  	trace_collection_remove(ctx->tc, handle->td);
>> -	/* Close the trace */
>> -	ret = handle->format->close_trace(handle->td);
>> -	if (ret) {
>> -		fprintf(stderr, "Error in close_trace callback\n");
>> -		return ret;
>> -	}
>> +
>>  	/* Remove and free the handle */
>>  	g_hash_table_remove(ctx->trace_handles,
>>  			(gpointer) (unsigned long) handle_id);
>> diff --git a/lib/trace-handle.c b/lib/trace-handle.c
>> index 0da565b..17a6e0d 100644
>> --- a/lib/trace-handle.c
>> +++ b/lib/trace-handle.c
>> @@ -49,7 +49,10 @@ struct bt_trace_handle *bt_trace_handle_create(struct bt_context *ctx)
>>  
>>  void bt_trace_handle_destroy(struct bt_trace_handle *th)
>>  {
>> -	th->format->close_trace(th->td);
>> +	if (th->format->close_trace(th->td)) {
>> +		fprintf(stderr, "Error in close_trace callback\n");
>> +	}
>> +
>>  	g_free(th);
>>  }
>>  
>> -- 
>> 1.8.1.1
>>
>>
>> _______________________________________________
>> lttng-dev mailing list
>> lttng-dev at lists.lttng.org
>> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 

[lttng-dev] [PATCH babeltrace] Fix: Double free in bt_context_remove_trace().

[Mathieu Desnoyers]

* Julien Desfossez (julien.desfossez at polymtl.ca) wrote:
> On 21/01/13 02:32 PM, Mathieu Desnoyers wrote:
> > * J?r?mie Galarneau (jeremie.galarneau at efficios.com) wrote:
> >> ctf_close_trace was being called twice when calling bt_context_remove_trace thus
> >> causing free() to be called on an invalid pointer.
> >>
> >> Calling bt_context_remove_trace() would call ctf_close_trace() once via the
> >> close_handle callback registered on the ctf format struct and a second call would
> >> take place from bt_trace_handle_destroy() which is registered as the
> >> value_destroy_func on the trace_handles hash table of the current context.
> >>
> >> The first explicit call to handle->format->close_trace is unnecessary.
> >>
> >> The crash is reproducible by invoking the tests-python.py script.
> > 
> > I think we should leave the close_trace calls in both locations, but set
> > handle->td to -1 when we close it, and test for negative handle ID
> > before doing the close.
> 
> I agree, we need the two calls to close, one is called when we remove a
> trace manually and the other one is called when we destroy the context.
> Setting and testing the td to -1 will solve the problem and will avoid
> calling close_trace at all if not needed.

Can we ever call bt_trace_handle_destroy() manually on a handle that
needs the trace to be closed ? After careful review, I don't think so...
(see patch v2 from J?r?mie).

Thanks,

Mathieu

> 
> Thanks,
> 
> Julien
> 
> > 
> > Thanks,
> > 
> > Mathieu
> > 
> >>
> >> Signed-off-by: J?r?mie Galarneau <jeremie.galarneau at efficios.com>
> >> ---
> >>  lib/context.c      | 8 +-------
> >>  lib/trace-handle.c | 5 ++++-
> >>  2 files changed, 5 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/lib/context.c b/lib/context.c
> >> index 5516e49..05fb994 100644
> >> --- a/lib/context.c
> >> +++ b/lib/context.c
> >> @@ -156,7 +156,6 @@ end:
> >>  int bt_context_remove_trace(struct bt_context *ctx, int handle_id)
> >>  {
> >>  	struct bt_trace_handle *handle;
> >> -	int ret;
> >>  
> >>  	if (!ctx)
> >>  		return -EINVAL;
> >> @@ -168,12 +167,7 @@ int bt_context_remove_trace(struct bt_context *ctx, int handle_id)
> >>  
> >>  	/* Remove from containers */
> >>  	trace_collection_remove(ctx->tc, handle->td);
> >> -	/* Close the trace */
> >> -	ret = handle->format->close_trace(handle->td);
> >> -	if (ret) {
> >> -		fprintf(stderr, "Error in close_trace callback\n");
> >> -		return ret;
> >> -	}
> >> +
> >>  	/* Remove and free the handle */
> >>  	g_hash_table_remove(ctx->trace_handles,
> >>  			(gpointer) (unsigned long) handle_id);
> >> diff --git a/lib/trace-handle.c b/lib/trace-handle.c
> >> index 0da565b..17a6e0d 100644
> >> --- a/lib/trace-handle.c
> >> +++ b/lib/trace-handle.c
> >> @@ -49,7 +49,10 @@ struct bt_trace_handle *bt_trace_handle_create(struct bt_context *ctx)
> >>  
> >>  void bt_trace_handle_destroy(struct bt_trace_handle *th)
> >>  {
> >> -	th->format->close_trace(th->td);
> >> +	if (th->format->close_trace(th->td)) {
> >> +		fprintf(stderr, "Error in close_trace callback\n");
> >> +	}
> >> +
> >>  	g_free(th);
> >>  }
> >>  
> >> -- 
> >> 1.8.1.1
> >>
> >>
> >> _______________________________________________
> >> lttng-dev mailing list
> >> lttng-dev at lists.lttng.org
> >> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> > 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com