- make sure connection is destroyed and no use-after-free happens

3 files changed, 68 insertions, 48 deletions

diff --git a/src/cadet/gnunet-service-cadet_connection.c b/src/cadet/gnunet-service-cadet_connection.c
index d310501bb..5c28f9e4c 100644
--- a/src/cadet/gnunet-service-cadet_connection.c
+++ b/src/cadet/gnunet-service-cadet_connection.c
@@ -564,8 +564,10 @@ send_ack (struct CadetConnection *c, unsigned int buffer, int fwd, int force)
  * @param fwd Was this a FWD going message?
  * @param size Size of the message.
  * @param wait Time spent waiting for core (only the time for THIS message)
+ *
+ * @return #GNUNET_YES if connection was destroyed, #GNUNET_NO otherwise.
  */
-static void
+static int
 conn_message_sent (void *cls,
                    struct CadetConnection *c, int sent,
                    uint16_t type, uint32_t pid, int fwd, size_t size,
@@ -609,7 +611,7 @@ conn_message_sent (void *cls,
       LOG (GNUNET_ERROR_TYPE_ERROR, "Message %s sent on NULL connection!\n",
            GC_m2s (type));
     }
-    return;
+    return GNUNET_NO;
   }
   LOG (GNUNET_ERROR_TYPE_DEBUG, " C_P- %p %u\n", c, c->pending_messages);
   c->pending_messages--;
@@ -617,7 +619,7 @@ conn_message_sent (void *cls,
   {
     LOG (GNUNET_ERROR_TYPE_DEBUG, "!  destroying connection!\n");
     GCC_destroy (c);
-    return;
+    return GNUNET_YES;
   }
   /* Send ACK if needed, after accounting for sent ID in fc->queue_n */
   switch (type)
@@ -674,7 +676,7 @@ conn_message_sent (void *cls,
   LOG (GNUNET_ERROR_TYPE_DEBUG, "!  message sent!\n");
 
   if (NULL == c->perf)
-    return; /* Only endpoints are interested in timing. */
+    return GNUNET_NO; /* Only endpoints are interested in timing. */
 
   p = c->perf;
   usecsperbyte = ((double) wait.rel_value_us) / size;
@@ -695,6 +697,7 @@ conn_message_sent (void *cls,
     p->avg /= p->size;
   }
   p->idx = (p->idx + 1) % AVG_MSGS;
+  return GNUNET_NO;
 }
 
 
@@ -1209,8 +1212,7 @@ poll_sent (void *cls,
     LOG (GNUNET_ERROR_TYPE_DEBUG, " *** POLL canceled on shutdown\n");
     return;
   }
-  LOG (GNUNET_ERROR_TYPE_DEBUG,
-       " *** POLL sent for , scheduling new one!\n");
+  LOG (GNUNET_ERROR_TYPE_DEBUG, " *** POLL sent for , scheduling new one!\n");
   fc->poll_msg = NULL;
   fc->poll_time = GNUNET_TIME_STD_BACKOFF (fc->poll_time);
   fc->poll_task = GNUNET_SCHEDULER_add_delayed (fc->poll_time,
@@ -1806,8 +1808,7 @@ GCC_handle_broken (void* cls,
   struct GNUNET_CADET_ConnectionBroken *msg;
   struct CadetConnection *c;
   struct CadetTunnel *t;
-  unsigned int del;
-  int pending;
+  int destroyed;
   int fwd;
 
   msg = (struct GNUNET_CADET_ConnectionBroken *) message;
@@ -1845,28 +1846,21 @@ GCC_handle_broken (void* cls,
     c->state = CADET_CONNECTION_BROKEN;
     GCT_remove_connection (t, c);
     c->t = NULL;
-    pending = c->pending_messages;
+    destroyed = GNUNET_NO;
 
-    /* GCP_connection_pop will destroy the connection when the last message
-     * is popped! Do not use 'c' after the call. */
-    while (NULL != (out_msg = GCP_connection_pop (neighbor, c, &del)))
+    /* GCP_connection_pop could destroy the connection! */
+    while (NULL != (out_msg = GCP_connection_pop (neighbor, c, &destroyed)))
     {
-      pending -= del + 1; /* Substract the deleted messages + the popped one */
       GCT_resend_message (out_msg, t);
     }
     /* All pending messages should have been popped,
      * and the connection destroyed by the continuation.
-     * If last message was just deleted, then continuation wasn't called.
      */
-    if (0 < pending || 0 < del)
+    if (GNUNET_YES != destroyed)
     {
-      GNUNET_break (0 == pending);
+      GNUNET_break (0);
       GCC_destroy (c);
     }
-    else
-    {
-      GNUNET_break (0 == pending); /* If negative: counter error! */
-    }
   }
   else
   {
diff --git a/src/cadet/gnunet-service-cadet_peer.c b/src/cadet/gnunet-service-cadet_peer.c
index 90b70aa8e..ce22c20da 100644
--- a/src/cadet/gnunet-service-cadet_peer.c
+++ b/src/cadet/gnunet-service-cadet_peer.c
@@ -1074,8 +1074,8 @@ queue_send (void *cls, size_t size, void *buf)
          queue->payload_id, GCC_2s (c), c, GC_f2s (queue->fwd), data_size);
   }
 
-  /* Free queue, but cls was freed by send_core_* */
-  GCP_queue_destroy (queue, GNUNET_NO, GNUNET_YES, pid);
+  /* Free queue, but cls was freed by send_core_*. */
+  (void) GCP_queue_destroy (queue, GNUNET_NO, GNUNET_YES, pid);
 
   /* If more data in queue, send next */
   queue = peer_get_first_message (peer);
@@ -1121,16 +1121,23 @@ queue_send (void *cls, size_t size, void *buf)
  * Free a transmission that was already queued with all resources
  * associated to the request.
  *
+ * If connection was marked to be destroyed, and this was the last queued
+ * message on it, the connection will be free'd as a result.
+ *
  * @param queue Queue handler to cancel.
  * @param clear_cls Is it necessary to free associated cls?
  * @param sent Was it really sent? (Could have been canceled)
  * @param pid PID, if relevant (was sent and was a payload message).
+ *
+ * @return #GNUNET_YES if connection was destroyed as a result,
+ *         #GNUNET_NO otherwise.
  */
-void
+int
 GCP_queue_destroy (struct CadetPeerQueue *queue, int clear_cls,
                    int sent, uint32_t pid)
 {
   struct CadetPeer *peer;
+  int connection_destroyed;
 
   peer = queue->peer;
   LOG (GNUNET_ERROR_TYPE_DEBUG, "queue destroy %s\n", GC_m2s (queue->type));
@@ -1168,11 +1175,18 @@ GCP_queue_destroy (struct CadetPeerQueue *queue, int clear_cls,
 
   if (NULL != queue->callback)
   {
+    struct GNUNET_TIME_Relative core_wait_time;
+
     LOG (GNUNET_ERROR_TYPE_DEBUG, " calling callback\n");
-    queue->callback (queue->callback_cls,
-                     queue->c, sent, queue->type, pid,
-                     queue->fwd, queue->size,
-                     GNUNET_TIME_absolute_get_duration (queue->start_waiting));
+    core_wait_time = GNUNET_TIME_absolute_get_duration (queue->start_waiting);
+    connection_destroyed = queue->callback (queue->callback_cls,
+                                            queue->c, sent, queue->type, pid,
+                                            queue->fwd, queue->size,
+                                            core_wait_time);
+  }
+  else
+  {
+    connection_destroyed = GNUNET_NO;
   }
 
   if (NULL == peer_get_first_message (peer) && NULL != peer->core_transmit)
@@ -1182,6 +1196,7 @@ GCP_queue_destroy (struct CadetPeerQueue *queue, int clear_cls,
   }
 
   GNUNET_free (queue);
+  return connection_destroyed;
 }
 
 
@@ -1309,20 +1324,23 @@ GCP_queue_cancel (struct CadetPeer *peer, struct CadetConnection *c)
   struct CadetPeerQueue *q;
   struct CadetPeerQueue *next;
   struct CadetPeerQueue *prev;
+  int connection_destroyed;
 
+  connection_destroyed = GNUNET_NO;
   for (q = peer->queue_head; NULL != q; q = next)
   {
     prev = q->prev;
     if (q->c == c)
     {
       LOG (GNUNET_ERROR_TYPE_DEBUG, "GMP queue cancel %s\n", GC_m2s (q->type));
+      GNUNET_break (GNUNET_NO == connection_destroyed);
       if (GNUNET_MESSAGE_TYPE_CADET_CONNECTION_DESTROY == q->type)
       {
         q->c = NULL;
       }
       else
       {
-        GCP_queue_destroy (q, GNUNET_YES, GNUNET_NO, 0);
+        connection_destroyed = GCP_queue_destroy (q, GNUNET_YES, GNUNET_NO, 0);
       }
 
       /* Get next from prev, q->next might be already freed:
@@ -1338,13 +1356,11 @@ GCP_queue_cancel (struct CadetPeer *peer, struct CadetConnection *c)
       next = q->next;
     }
   }
-  if (NULL == peer->queue_head)
+
+  if (NULL == peer->queue_head && NULL != peer->core_transmit)
   {
-    if (NULL != peer->core_transmit)
-    {
-      GNUNET_CORE_notify_transmit_ready_cancel (peer->core_transmit);
-      peer->core_transmit = NULL;
-    }
+    GNUNET_CORE_notify_transmit_ready_cancel (peer->core_transmit);
+    peer->core_transmit = NULL;
   }
 }
 
@@ -1382,28 +1398,28 @@ connection_get_first_message (struct CadetPeer *peer, struct CadetConnection *c)
  * Get the first message for a connection and unqueue it.
  *
  * Only tunnel (or higher) level messages are unqueued. Connection specific
- * messages are destroyed and the count given to the caller.
+ * messages are silently destroyed upon encounter.
  *
  * @param peer Neighboring peer.
  * @param c Connection.
- * @param del[out] How many messages have been deleted without returning.
- *                 Can be NULL.
+ * @param destroyed[in/out] Was the connection destroyed (prev/as a result)?.
  *
  * @return First message for this connection.
  */
 struct GNUNET_MessageHeader *
 GCP_connection_pop (struct CadetPeer *peer,
                     struct CadetConnection *c,
-                    unsigned int *del)
+                    int *destroyed)
 {
   struct CadetPeerQueue *q;
   struct CadetPeerQueue *next;
   struct GNUNET_MessageHeader *msg;
+  int dest;
 
-  if (NULL != del) *del = 0;
   LOG (GNUNET_ERROR_TYPE_DEBUG, "Connection pop on connection %p\n", c);
   for (q = peer->queue_head; NULL != q; q = next)
   {
+    GNUNET_break (NULL == destroyed || GNUNET_NO == *destroyed);
     next = q->next;
     if (q->c != c)
       continue;
@@ -1415,14 +1431,17 @@ GCP_connection_pop (struct CadetPeer *peer,
       case GNUNET_MESSAGE_TYPE_CADET_CONNECTION_BROKEN:
       case GNUNET_MESSAGE_TYPE_CADET_ACK:
       case GNUNET_MESSAGE_TYPE_CADET_POLL:
-        GCP_queue_destroy (q, GNUNET_YES, GNUNET_NO, 0);
-        if (NULL != del) *del = *del + 1;
+        dest = GCP_queue_destroy (q, GNUNET_YES, GNUNET_NO, 0);
+        if (NULL != destroyed && GNUNET_YES == dest)
+          *destroyed = GNUNET_YES;
         continue;
 
       case GNUNET_MESSAGE_TYPE_CADET_KX:
       case GNUNET_MESSAGE_TYPE_CADET_ENCRYPTED:
         msg = (struct GNUNET_MessageHeader *) q->cls;
-        GCP_queue_destroy (q, GNUNET_NO, GNUNET_NO, 0);
+        dest = GCP_queue_destroy (q, GNUNET_NO, GNUNET_NO, 0);
+        if (NULL != destroyed && GNUNET_YES == dest)
+          *destroyed = GNUNET_YES;
         return msg;
 
       default:
diff --git a/src/cadet/gnunet-service-cadet_peer.h b/src/cadet/gnunet-service-cadet_peer.h
index e581685fa..ea64665d4 100644
--- a/src/cadet/gnunet-service-cadet_peer.h
+++ b/src/cadet/gnunet-service-cadet_peer.h
@@ -63,8 +63,10 @@ struct CadetPeerQueue;
  * @param fwd Was this a FWD going message?
  * @param size Size of the message.
  * @param wait Time spent waiting for core (only the time for THIS message)
+ *
+ * @return #GNUNET_YES if connection was destroyed, #GNUNET_NO otherwise.
  */
-typedef void (*GCP_sent) (void *cls,
+typedef int (*GCP_sent) (void *cls,
                           struct CadetConnection *c, int sent,
                           uint16_t type, uint32_t pid, int fwd, size_t size,
                           struct GNUNET_TIME_Relative wait);
@@ -125,12 +127,18 @@ GCP_connect (struct CadetPeer *peer);
  * Free a transmission that was already queued with all resources
  * associated to the request.
  *
+ * If connection was marked to be destroyed, and this was the last queued
+ * message on it, the connection will be free'd as a result.
+ *
  * @param queue Queue handler to cancel.
  * @param clear_cls Is it necessary to free associated cls?
  * @param sent Was it really sent? (Could have been canceled)
  * @param pid PID, if relevant (was sent and was a payload message).
+ *
+ * @return #GNUNET_YES if connection was destroyed as a result,
+ *         #GNUNET_NO otherwise.
  */
-void
+int
 GCP_queue_destroy (struct CadetPeerQueue *queue, int clear_cls,
                    int sent, uint32_t pid);
 
@@ -170,19 +178,18 @@ GCP_queue_cancel (struct CadetPeer *peer, struct CadetConnection *c);
  * Get the first message for a connection and unqueue it.
  *
  * Only tunnel (or higher) level messages are unqueued. Connection specific
- * messages are destroyed and the count given to the caller.
+ * messages are silently destroyed upon encounter.
  *
  * @param peer Neighboring peer.
  * @param c Connection.
- * @param del[out] How many messages have been deleted without returning.
- *                 Can be NULL.
+ * @param destroyed[out] Was the connection destroyed as a result?.
  *
  * @return First message for this connection.
  */
 struct GNUNET_MessageHeader *
 GCP_connection_pop (struct CadetPeer *peer,
                     struct CadetConnection *c,
-                    unsigned int *del);
+                    int *destroyed);
 
 /**
  * Unlock a possibly locked queue for a connection.