From b6e0aac51cd54c83eaefaff1f4c7145732127393 Mon Sep 17 00:00:00 2001 From: Bart Polot Date: Tue, 22 Jul 2014 16:00:36 +0000 Subject: - make sure connection is destroyed and no use-after-free happens --- src/cadet/gnunet-service-cadet_connection.c | 34 +++++++--------- src/cadet/gnunet-service-cadet_peer.c | 63 +++++++++++++++++++---------- src/cadet/gnunet-service-cadet_peer.h | 19 ++++++--- 3 files changed, 68 insertions(+), 48 deletions(-) (limited to 'src/cadet') 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. -- cgit v1.2.3