From d5d629b58bff4ded0814e352d7dbdda6d1b6c9c2 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Fri, 14 Jun 2013 14:37:50 -0400 Subject: [PATCH] Fix: memleak in the UST registry Signed-off-by: David Goulet --- src/bin/lttng-sessiond/ust-app.c | 35 +++++++++++++++++++++++---- src/bin/lttng-sessiond/ust-registry.c | 17 +++++++------ 2 files changed, 40 insertions(+), 12 deletions(-) diff --git a/src/bin/lttng-sessiond/ust-app.c b/src/bin/lttng-sessiond/ust-app.c index 7e7c89a48..07c75031a 100644 --- a/src/bin/lttng-sessiond/ust-app.c +++ b/src/bin/lttng-sessiond/ust-app.c @@ -4473,6 +4473,7 @@ static int reply_ust_register_channel(int sock, int sobjd, int cobjd, DBG("Application socket %d is being teardown. Abort event notify", sock); ret = 0; + free(fields); goto error_rcu_unlock; } @@ -4481,6 +4482,7 @@ static int reply_ust_register_channel(int sock, int sobjd, int cobjd, if (!ua_chan) { DBG("Application channel is being teardown. Abort event notify"); ret = 0; + free(fields); goto error_rcu_unlock; } @@ -4517,6 +4519,9 @@ static int reply_ust_register_channel(int sock, int sobjd, int cobjd, } else { /* Get current already assigned values. */ type = chan_reg->header_type; + free(fields); + /* Set to NULL so the error path does not do a double free. */ + fields = NULL; } /* Channel id is set during the object creation. */ chan_id = chan_reg->chan_id; @@ -4552,6 +4557,9 @@ error: pthread_mutex_unlock(®istry->lock); error_rcu_unlock: rcu_read_unlock(); + if (ret) { + free(fields); + } return ret; } @@ -4584,6 +4592,9 @@ static int add_event_ust_registry(int sock, int sobjd, int cobjd, char *name, DBG("Application socket %d is being teardown. Abort event notify", sock); ret = 0; + free(sig); + free(fields); + free(model_emf_uri); goto error_rcu_unlock; } @@ -4592,6 +4603,9 @@ static int add_event_ust_registry(int sock, int sobjd, int cobjd, char *name, if (!ua_chan) { DBG("Application channel is being teardown. Abort event notify"); ret = 0; + free(sig); + free(fields); + free(model_emf_uri); goto error_rcu_unlock; } @@ -4609,6 +4623,11 @@ static int add_event_ust_registry(int sock, int sobjd, int cobjd, char *name, pthread_mutex_lock(®istry->lock); + /* + * From this point on, this call acquires the ownership of the sig, fields + * and model_emf_uri meaning any free are done inside it if needed. These + * three variables MUST NOT be read/write after this. + */ ret_code = ust_registry_create_event(registry, chan_reg_key, sobjd, cobjd, name, sig, nr_fields, fields, loglevel, model_emf_uri, ua_sess->buffer_type, &event_id); @@ -4685,13 +4704,15 @@ int ust_app_recv_notify(int sock) goto error; } - /* Add event to the UST registry coming from the notify socket. */ + /* + * Add event to the UST registry coming from the notify socket. This + * call will free if needed the sig, fields and model_emf_uri. This + * code path loses the ownsership of these variables and transfer them + * to the this function. + */ ret = add_event_ust_registry(sock, sobjd, cobjd, name, sig, nr_fields, fields, loglevel, model_emf_uri); if (ret < 0) { - free(sig); - free(model_emf_uri); - free(fields); goto error; } @@ -4716,10 +4737,14 @@ int ust_app_recv_notify(int sock) goto error; } + /* + * The fields ownership are transfered to this function call meaning + * that if needed it will be freed. After this, it's invalid to access + * fields or clean it up. + */ ret = reply_ust_register_channel(sock, sobjd, cobjd, nr_fields, fields); if (ret < 0) { - free(fields); goto error; } diff --git a/src/bin/lttng-sessiond/ust-registry.c b/src/bin/lttng-sessiond/ust-registry.c index 682da2b48..298ddbb6a 100644 --- a/src/bin/lttng-sessiond/ust-registry.c +++ b/src/bin/lttng-sessiond/ust-registry.c @@ -198,34 +198,34 @@ int ust_registry_create_event(struct ust_registry_session *session, assert(sig); assert(event_id_p); + rcu_read_lock(); + /* * This should not happen but since it comes from the UST tracer, an * external party, don't assert and simply validate values. */ if (session_objd < 0 || channel_objd < 0) { ret = -EINVAL; - goto error; + goto error_free; } - rcu_read_lock(); - chan = ust_registry_channel_find(session, chan_key); if (!chan) { ret = -EINVAL; - goto error_unlock; + goto error_free; } /* Check if we've reached the maximum possible id. */ if (ust_registry_is_max_id(chan->used_event_id)) { ret = -ENOENT; - goto error_unlock; + goto error_free; } event = alloc_event(session_objd, channel_objd, name, sig, nr_fields, fields, loglevel, model_emf_uri); if (!event) { ret = -ENOMEM; - goto error_unlock; + goto error_free; } DBG3("UST registry creating event with event: %s, sig: %s, id: %u, " @@ -279,9 +279,12 @@ int ust_registry_create_event(struct ust_registry_session *session, rcu_read_unlock(); return 0; +error_free: + free(sig); + free(fields); + free(model_emf_uri); error_unlock: rcu_read_unlock(); -error: destroy_event(event); return ret; } -- 2.34.1