Fix possible leaks in graph's current design
authorPhilippe Proulx <eeppeliteloop@gmail.com>
Sat, 13 May 2017 00:34:50 +0000 (20:34 -0400)
committerJérémie Galarneau <jeremie.galarneau@efficios.com>
Sun, 28 May 2017 16:57:43 +0000 (12:57 -0400)
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 <eeppeliteloop@gmail.com>
Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
include/babeltrace/graph/connection-internal.h
include/babeltrace/graph/notification-iterator-internal.h
include/babeltrace/graph/notification-iterator.h
lib/graph/component.c
lib/graph/connection.c
lib/graph/graph.c
lib/graph/iterator.c

index ac75320d6cf4480f397eca1780129f068a89866f..2a85296831089a06e3f46bc1d4326b5efbca7abd 100644 (file)
@@ -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 */
index e1f9f211a6817e8adc9ef2bac66cc97ef690432a..5903a586ecae7e3bb9fcba3f88737ecb90ca0711 100644 (file)
@@ -29,6 +29,7 @@
 #include <babeltrace/babeltrace-internal.h>
 #include <babeltrace/object-internal.h>
 #include <babeltrace/ref-internal.h>
+#include <babeltrace/graph/connection.h>
 #include <babeltrace/graph/notification.h>
 #include <babeltrace/graph/notification-iterator.h>
 #include <babeltrace/graph/private-notification-iterator.h>
@@ -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 */
index 35f68a8398d805950f5732b3d86fb32cd5366e9e..8646d8e39439dafaf7ac4f84b73537db24e12473 100644 (file)
@@ -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. */
index c541985b3326b52f374829f877ba162f3aa835db..9ed599448880b647d4d122fd9c50c2d8bb297cde 100644 (file)
@@ -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 */
index a9a5f9db667b2a86b0e8e8e117ab1aefe518f4a1..feed59556247c562c40d2149cf38cae7e20263f7 100644 (file)
@@ -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);
+}
index 90eb92fcdd2c0f257dbdd28ab1035da0ec82dcfd..808296f74f52c0828bf4c95468158d18d71506a9 100644 (file)
@@ -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);
        }
index 1b1862bd1b87d4299c98ed60c2cb1c94da3db11a..1bfe3495051be8caa8d22d15b3532cceea794b65 100644 (file)
@@ -30,6 +30,7 @@
 #include <babeltrace/ctf-ir/event-internal.h>
 #include <babeltrace/ctf-ir/packet-internal.h>
 #include <babeltrace/ctf-ir/stream-internal.h>
+#include <babeltrace/graph/connection-internal.h>
 #include <babeltrace/graph/component.h>
 #include <babeltrace/graph/component-source-internal.h>
 #include <babeltrace/graph/component-class-internal.h>
@@ -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;
This page took 0.034345 seconds and 4 git commands to generate.