From bd14d76835630e092320c8a04300088242dcdc99 Mon Sep 17 00:00:00 2001 From: Philippe Proulx Date: Fri, 12 May 2017 20:34:50 -0400 Subject: [PATCH] Fix possible leaks in graph's current design MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit There was a possible leak when, for example: * The user has a reference to the graph. * The graph holds its child sink component. * The sink component has a reference to a notification iterator. * The notification iterator has a reference to the source component. * The source component has a reference to its parent, the graph, because its reference count is 1 (owned by the notification iterator). This forms a cycle: sink component owns the notification iterator, which owns the source component, which owns the graph, which holds the sink component. To break this cycle and still guarantee that the lifetime of a notification iterator's upstream component is equal to or longer than this iterator's lifetime, we split the notification iterator's destruction and finalization (user's finalization method) phases. In other words, a notification iterator can be finalized without being destroyed. However it is guaranteed that it is always finalized once destroyed. A connection keeps a set of created notification iterators (weak references). There are two possible paths to finalize a notification iterator: 1. The graph still exists and is not being destroyed, and the final reference to the notification iterator is put: the notification is destroyed, and since it has not been finalized yet, it's finalized during that phase. 2. The graph is being destroyed, and an owner (or more) still owns a reference to the notification iterator. The graph's destructor first destroys the graph's connections. Each connection finalizes each notification iterator in its list, but does not destroy them. The notification iterators are marked as finalized. Eventually the graph destroys its components, some of which could put the last reference to a notification iterator: the notification iterator is destroyed, but not finalized twice. When a notification iterator is marked as finalized, calling its "next" method returns the new BT_NOTIFICATION_ITERATOR_STATUS_CANCELED status when its queue is empty (instead of BT_NOTIFICATION_ITERATOR_STATUS_END). Signed-off-by: Philippe Proulx Signed-off-by: Jérémie Galarneau --- .../babeltrace/graph/connection-internal.h | 10 ++ .../graph/notification-iterator-internal.h | 45 ++++- .../babeltrace/graph/notification-iterator.h | 2 + lib/graph/component.c | 10 ++ lib/graph/connection.c | 48 ++++- lib/graph/graph.c | 34 +++- lib/graph/iterator.c | 164 +++++++++++++----- 7 files changed, 259 insertions(+), 54 deletions(-) diff --git a/include/babeltrace/graph/connection-internal.h b/include/babeltrace/graph/connection-internal.h index ac75320d..2a852968 100644 --- a/include/babeltrace/graph/connection-internal.h +++ b/include/babeltrace/graph/connection-internal.h @@ -48,6 +48,12 @@ struct bt_connection { struct bt_port *downstream_port; /* Upstream port. */ struct bt_port *upstream_port; + + /* + * Weak references to all the notification iterators that were + * created on this connection. + */ + GPtrArray *iterators; }; static inline @@ -72,4 +78,8 @@ struct bt_connection *bt_connection_create(struct bt_graph *graph, BT_HIDDEN void bt_connection_disconnect_ports(struct bt_connection *conn); +BT_HIDDEN +void bt_connection_remove_iterator(struct bt_connection *conn, + struct bt_notification_iterator *iterator); + #endif /* BABELTRACE_COMPONENT_CONNECTION_INTERNAL_H */ diff --git a/include/babeltrace/graph/notification-iterator-internal.h b/include/babeltrace/graph/notification-iterator-internal.h index e1f9f211..5903a586 100644 --- a/include/babeltrace/graph/notification-iterator-internal.h +++ b/include/babeltrace/graph/notification-iterator-internal.h @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -45,10 +46,36 @@ enum bt_notification_iterator_notif_type { BT_NOTIFICATION_ITERATOR_NOTIF_TYPE_PACKET_END = (1U << 5), }; +enum bt_notification_iterator_state { + /* Iterator is active, not at the end yet, and not finalized. */ + BT_NOTIFICATION_ITERATOR_STATE_ACTIVE, + + /* + * Iterator is ended, not finalized yet: the "next" method + * returns BT_NOTIFICATION_ITERATOR_STATUS_END. + */ + BT_NOTIFICATION_ITERATOR_STATE_ENDED, + + /* + * Iterator is finalized, but not at the end yet. This means + * that the "next" method can still return queued notifications + * before returning the BT_NOTIFICATION_ITERATOR_STATUS_CANCELED + * status. + */ + BT_NOTIFICATION_ITERATOR_STATE_FINALIZED, + + /* + * Iterator is finalized and ended: the "next" method always + * returns BT_NOTIFICATION_ITERATOR_STATUS_CANCELED. + */ + BT_NOTIFICATION_ITERATOR_STATE_FINALIZED_AND_ENDED, +}; + struct bt_notification_iterator { struct bt_object base; - struct bt_component *upstream_component; /* owned by this */ - struct bt_port *upstream_port; /* owned by this */ + struct bt_component *upstream_component; /* Weak */ + struct bt_port *upstream_port; /* Weak */ + struct bt_connection *connection; /* Weak */ struct bt_notification *current_notification; /* owned by this */ GQueue *queue; /* struct bt_notification * (owned by this) */ @@ -86,7 +113,7 @@ struct bt_notification_iterator { */ uint32_t subscription_mask; - bt_bool is_ended; + enum bt_notification_iterator_state state; void *user_data; }; @@ -115,7 +142,8 @@ BT_HIDDEN struct bt_notification_iterator *bt_notification_iterator_create( struct bt_component *upstream_component, struct bt_port *upstream_port, - const enum bt_notification_type *notification_types); + const enum bt_notification_type *notification_types, + struct bt_connection *connection); /** * Validate a notification iterator. @@ -127,4 +155,13 @@ BT_HIDDEN enum bt_notification_iterator_status bt_notification_iterator_validate( struct bt_notification_iterator *iterator); +BT_HIDDEN +void bt_notification_iterator_finalize( + struct bt_notification_iterator *iterator); + +BT_HIDDEN +void bt_notification_iterator_set_connection( + struct bt_notification_iterator *iterator, + struct bt_connection *connection); + #endif /* BABELTRACE_COMPONENT_NOTIFICATION_ITERATOR_INTERNAL_H */ diff --git a/include/babeltrace/graph/notification-iterator.h b/include/babeltrace/graph/notification-iterator.h index 35f68a83..8646d8e3 100644 --- a/include/babeltrace/graph/notification-iterator.h +++ b/include/babeltrace/graph/notification-iterator.h @@ -40,6 +40,8 @@ struct bt_notification_iterator; * Status code. Errors are always negative. */ enum bt_notification_iterator_status { + /** Canceled. */ + BT_NOTIFICATION_ITERATOR_STATUS_CANCELED = 125, /** No notifications available for now. Try again later. */ BT_NOTIFICATION_ITERATOR_STATUS_AGAIN = 11, /** No more notifications to be delivered. */ diff --git a/lib/graph/component.c b/lib/graph/component.c index c541985b..9ed59944 100644 --- a/lib/graph/component.c +++ b/lib/graph/component.c @@ -78,6 +78,16 @@ void bt_component_destroy(struct bt_object *obj) return; } + /* + * The component's reference count is 0 if we're here. Increment + * it to avoid a double-destroy (possibly infinitely recursive). + * This could happen for example if the component's finalization + * function does bt_get() (or anything that causes bt_get() to + * be called) on itself (ref. count goes from 0 to 1), and then + * bt_put(): the reference count would go from 1 to 0 again and + * this function would be called again. + */ + obj->ref_count.count++; component = container_of(obj, struct bt_component, base); /* Call destroy listeners in reverse registration order */ diff --git a/lib/graph/connection.c b/lib/graph/connection.c index a9a5f9db..feed5955 100644 --- a/lib/graph/connection.c +++ b/lib/graph/connection.c @@ -43,6 +43,37 @@ void bt_connection_destroy(struct bt_object *obj) { struct bt_connection *connection = container_of(obj, struct bt_connection, base); + size_t i; + + /* + * Make sure that each notification iterator which was created + * for this connection is finalized before we destroy it. Once a + * notification iterator is finalized, all its method return + * NULL or the BT_NOTIFICATION_ITERATOR_STATUS_CANCELED status. + * + * Because connections are destroyed before components within a + * graph, this ensures that notification iterators are always + * finalized before their upstream component. + */ + if (connection->iterators) { + for (i = 0; i < connection->iterators->len; i++) { + struct bt_notification_iterator *iterator = + g_ptr_array_index(connection->iterators, i); + + bt_notification_iterator_finalize(iterator); + + /* + * Make sure this iterator does not try to + * remove itself from this connection's + * iterators on destruction because this + * connection won't exist anymore. + */ + bt_notification_iterator_set_connection(iterator, + NULL); + } + + g_ptr_array_free(connection->iterators, TRUE); + } /* * No bt_put on ports as a connection only holds _weak_ references @@ -78,6 +109,12 @@ struct bt_connection *bt_connection_create( } bt_object_init(connection, bt_connection_destroy); + connection->iterators = g_ptr_array_new(); + if (!connection->iterators) { + BT_PUT(connection); + goto end; + } + /* Weak references are taken, see comment in header. */ connection->upstream_port = upstream_port; connection->downstream_port = downstream_port; @@ -165,7 +202,6 @@ bt_private_connection_create_notification_iterator( } connection = bt_connection_from_private(private_connection); - if (!connection->upstream_port || !connection->downstream_port) { goto error; } @@ -189,7 +225,7 @@ bt_private_connection_create_notification_iterator( } iterator = bt_notification_iterator_create(upstream_component, - upstream_port, notification_types); + upstream_port, notification_types, connection); if (!iterator) { goto error; } @@ -230,6 +266,7 @@ bt_private_connection_create_notification_iterator( goto error; } + g_ptr_array_add(connection->iterators, iterator); goto end; error: @@ -239,3 +276,10 @@ end: bt_put(upstream_component); return iterator; } + +BT_HIDDEN +void bt_connection_remove_iterator(struct bt_connection *conn, + struct bt_notification_iterator *iterator) +{ + g_ptr_array_remove(conn->iterators, iterator); +} diff --git a/lib/graph/graph.c b/lib/graph/graph.c index 90eb92fc..808296f7 100644 --- a/lib/graph/graph.c +++ b/lib/graph/graph.c @@ -49,12 +49,40 @@ void bt_graph_destroy(struct bt_object *obj) struct bt_graph *graph = container_of(obj, struct bt_graph, base); - if (graph->components) { - g_ptr_array_free(graph->components, TRUE); - } + /* + * The graph's reference count is 0 if we're here. Increment + * it to avoid a double-destroy (possibly infinitely recursive) + * in this situation: + * + * 1. We put and destroy a connection. + * 2. This connection's destructor finalizes its active + * notification iterators. + * 3. A notification iterator's finalization function gets a + * new reference on its component (reference count goes from + * 0 to 1). + * 4. Since this component's reference count goes to 1, it takes + * a reference on its parent (this graph). This graph's + * reference count goes from 0 to 1. + * 5. The notification iterator's finalization function puts its + * component reference (reference count goes from 1 to 0). + * 6. Since this component's reference count goes from 1 to 0, + * it puts its parent (this graph). This graph's reference + * count goes from 1 to 0. + * 7. Since this graph's reference count goes from 1 to 0, + * its destructor is called (this function). + * + * With the incrementation below, the graph's reference count at + * step 4 goes from 1 to 2, and from 2 to 1 at step 6. This + * ensures that this function is not called two times. + */ + obj->ref_count.count++; + if (graph->connections) { g_ptr_array_free(graph->connections, TRUE); } + if (graph->components) { + g_ptr_array_free(graph->components, TRUE); + } if (graph->sinks_to_consume) { g_queue_free(graph->sinks_to_consume); } diff --git a/lib/graph/iterator.c b/lib/graph/iterator.c index 1b1862bd..1bfe3495 100644 --- a/lib/graph/iterator.c +++ b/lib/graph/iterator.c @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -263,44 +264,22 @@ static void bt_notification_iterator_destroy(struct bt_object *obj) { struct bt_notification_iterator *iterator; - struct bt_component_class *comp_class; assert(obj); - iterator = container_of(obj, struct bt_notification_iterator, - base); - assert(iterator->upstream_component); - comp_class = iterator->upstream_component->class; - - /* Call user-defined destroy method */ - switch (comp_class->type) { - case BT_COMPONENT_CLASS_TYPE_SOURCE: - { - struct bt_component_class_source *source_class; - - source_class = container_of(comp_class, struct bt_component_class_source, parent); - - if (source_class->methods.iterator.finalize) { - source_class->methods.iterator.finalize( - bt_private_notification_iterator_from_notification_iterator(iterator)); - } - break; - } - case BT_COMPONENT_CLASS_TYPE_FILTER: - { - struct bt_component_class_filter *filter_class; - filter_class = container_of(comp_class, struct bt_component_class_filter, parent); - - if (filter_class->methods.iterator.finalize) { - filter_class->methods.iterator.finalize( - bt_private_notification_iterator_from_notification_iterator(iterator)); - } - break; - } - default: - /* Unreachable */ - assert(0); - } + /* + * The notification iterator's reference count is 0 if we're + * here. Increment it to avoid a double-destroy (possibly + * infinitely recursive). This could happen for example if the + * notification iterator's finalization function does bt_get() + * (or anything that causes bt_get() to be called) on itself + * (ref. count goes from 0 to 1), and then bt_put(): the + * reference count would go from 1 to 0 again and this function + * would be called again. + */ + obj->ref_count.count++; + iterator = container_of(obj, struct bt_notification_iterator, base); + bt_notification_iterator_finalize(iterator); if (iterator->queue) { struct bt_notification *notif; @@ -338,12 +317,88 @@ void bt_notification_iterator_destroy(struct bt_object *obj) g_array_free(iterator->actions, TRUE); } + if (iterator->connection) { + /* + * Remove ourself from the originating connection so + * that it does not try to finalize a dangling pointer + * later. + */ + bt_connection_remove_iterator(iterator->connection, iterator); + } + bt_put(iterator->current_notification); - bt_put(iterator->upstream_component); - bt_put(iterator->upstream_port); g_free(iterator); } +BT_HIDDEN +void bt_notification_iterator_finalize( + struct bt_notification_iterator *iterator) +{ + struct bt_component_class *comp_class = NULL; + bt_component_class_notification_iterator_finalize_method + finalize_method = NULL; + + assert(iterator); + + switch (iterator->state) { + case BT_NOTIFICATION_ITERATOR_STATE_FINALIZED: + case BT_NOTIFICATION_ITERATOR_STATE_FINALIZED_AND_ENDED: + /* Already finalized */ + return; + default: + break; + } + + assert(iterator->upstream_component); + comp_class = iterator->upstream_component->class; + + /* Call user-defined destroy method */ + switch (comp_class->type) { + case BT_COMPONENT_CLASS_TYPE_SOURCE: + { + struct bt_component_class_source *source_class; + + source_class = container_of(comp_class, struct bt_component_class_source, parent); + finalize_method = source_class->methods.iterator.finalize; + break; + } + case BT_COMPONENT_CLASS_TYPE_FILTER: + { + struct bt_component_class_filter *filter_class; + + filter_class = container_of(comp_class, struct bt_component_class_filter, parent); + finalize_method = filter_class->methods.iterator.finalize; + break; + } + default: + /* Unreachable */ + assert(0); + } + + if (finalize_method) { + finalize_method( + bt_private_notification_iterator_from_notification_iterator(iterator)); + } + + if (iterator->state == BT_NOTIFICATION_ITERATOR_STATE_ENDED) { + iterator->state = BT_NOTIFICATION_ITERATOR_STATE_FINALIZED_AND_ENDED; + } else { + iterator->state = BT_NOTIFICATION_ITERATOR_STATE_FINALIZED; + } + + iterator->upstream_component = NULL; + iterator->upstream_port = NULL; +} + +BT_HIDDEN +void bt_notification_iterator_set_connection( + struct bt_notification_iterator *iterator, + struct bt_connection *connection) +{ + assert(iterator); + iterator->connection = connection; +} + static int create_subscription_mask_from_notification_types( struct bt_notification_iterator *iterator, @@ -400,7 +455,8 @@ BT_HIDDEN struct bt_notification_iterator *bt_notification_iterator_create( struct bt_component *upstream_comp, struct bt_port *upstream_port, - const enum bt_notification_type *notification_types) + const enum bt_notification_type *notification_types, + struct bt_connection *connection) { enum bt_component_class_type type; struct bt_notification_iterator *iterator = NULL; @@ -447,8 +503,10 @@ struct bt_notification_iterator *bt_notification_iterator_create( goto error; } - iterator->upstream_component = bt_get(upstream_comp); - iterator->upstream_port = bt_get(upstream_port); + iterator->upstream_component = upstream_comp; + iterator->upstream_port = upstream_port; + iterator->connection = connection; + iterator->state = BT_NOTIFICATION_ITERATOR_STATE_ACTIVE; goto end; error: @@ -1264,9 +1322,15 @@ enum bt_notification_iterator_status ensure_queue_has_notifications( goto end; } - if (iterator->is_ended) { + switch (iterator->state) { + case BT_NOTIFICATION_ITERATOR_STATE_FINALIZED_AND_ENDED: + status = BT_NOTIFICATION_ITERATOR_STATUS_CANCELED; + goto end; + case BT_NOTIFICATION_ITERATOR_STATE_ENDED: status = BT_NOTIFICATION_ITERATOR_STATUS_END; goto end; + default: + break; } assert(iterator->upstream_component); @@ -1320,11 +1384,21 @@ enum bt_notification_iterator_status ensure_queue_has_notifications( goto end; } - if (iterator->queue->length == 0) { - status = BT_NOTIFICATION_ITERATOR_STATUS_END; - } + if (iterator->state == BT_NOTIFICATION_ITERATOR_STATE_FINALIZED) { + iterator->state = + BT_NOTIFICATION_ITERATOR_STATE_FINALIZED_AND_ENDED; - iterator->is_ended = BT_TRUE; + if (iterator->queue->length == 0) { + status = BT_NOTIFICATION_ITERATOR_STATUS_CANCELED; + } + } else { + iterator->state = + BT_NOTIFICATION_ITERATOR_STATE_ENDED; + + if (iterator->queue->length == 0) { + status = BT_NOTIFICATION_ITERATOR_STATUS_END; + } + } goto end; case BT_NOTIFICATION_ITERATOR_STATUS_AGAIN: status = BT_NOTIFICATION_ITERATOR_STATUS_AGAIN; -- 2.34.1