From 75f1032d17f5660afeec30943c0c7c0ab37d16a9 Mon Sep 17 00:00:00 2001 From: Christian Grothoff Date: Thu, 4 Aug 2011 06:43:15 +0000 Subject: LRN: Fix a rare arm-interceptor double-free bug --- src/arm/gnunet-service-arm_interceptor.c | 85 +++++++++++++++++++++----------- 1 file changed, 56 insertions(+), 29 deletions(-) (limited to 'src') diff --git a/src/arm/gnunet-service-arm_interceptor.c b/src/arm/gnunet-service-arm_interceptor.c index 237b9bf95..8f4a4731c 100644 --- a/src/arm/gnunet-service-arm_interceptor.c +++ b/src/arm/gnunet-service-arm_interceptor.c @@ -194,6 +194,10 @@ struct ForwardedConnection */ int first_write_done; + /** + * Reference count (the structure is freed when it reaches zero) + */ + int reference_count; }; /** @@ -289,14 +293,8 @@ closeClientAndServiceSockets (struct ForwardedConnection *fc, { #if DEBUG_SERVICE_MANAGER GNUNET_log (GNUNET_ERROR_TYPE_DEBUG, - "Stopping forwarding from service to client\n", - fc->listen_info->serviceName); + "Stopping forwarding from service to client\n"); #endif - if (fc->service_to_client_task != GNUNET_SCHEDULER_NO_TASK) - { - GNUNET_SCHEDULER_cancel (fc->service_to_client_task); - fc->service_to_client_task = GNUNET_SCHEDULER_NO_TASK; - } if (fc->armClientSocket != NULL) GNUNET_NETWORK_socket_shutdown (fc->armClientSocket, SHUT_WR); @@ -308,14 +306,8 @@ closeClientAndServiceSockets (struct ForwardedConnection *fc, { #if DEBUG_SERVICE_MANAGER GNUNET_log (GNUNET_ERROR_TYPE_DEBUG, - "Stopping forwarding from client to service\n", - fc->listen_info->serviceName); + "Stopping forwarding from client to service\n"); #endif - if (fc->client_to_service_task != GNUNET_SCHEDULER_NO_TASK) - { - GNUNET_SCHEDULER_cancel (fc->client_to_service_task); - fc->client_to_service_task = GNUNET_SCHEDULER_NO_TASK; - } if (fc->armClientSocket != NULL) GNUNET_NETWORK_socket_shutdown (fc->armClientSocket, SHUT_RD); @@ -323,27 +315,44 @@ closeClientAndServiceSockets (struct ForwardedConnection *fc, GNUNET_NETWORK_socket_shutdown (fc->armServiceSocket, SHUT_WR); } - if ( (fc->client_to_service_task != GNUNET_SCHEDULER_NO_TASK) || - (fc->service_to_client_task != GNUNET_SCHEDULER_NO_TASK) ) - return; #if DEBUG_SERVICE_MANAGER GNUNET_log (GNUNET_ERROR_TYPE_DEBUG, "Closing forwarding connection (done with both directions)\n"); #endif - if (fc->start_task != GNUNET_SCHEDULER_NO_TASK) - GNUNET_SCHEDULER_cancel (fc->start_task); - if ( (NULL != fc->armClientSocket) && - (GNUNET_SYSERR == + fc->reference_count -= 1; + if (fc->reference_count <= 0) + { + if ( (NULL != fc->armClientSocket) && + (GNUNET_SYSERR == GNUNET_NETWORK_socket_close (fc->armClientSocket)) ) - GNUNET_log_strerror (GNUNET_ERROR_TYPE_ERROR, "close"); - if ( (NULL != fc->armServiceSocket) && - (GNUNET_SYSERR == + { + GNUNET_log_strerror (GNUNET_ERROR_TYPE_ERROR, "close"); + fc->armClientSocket = NULL; + } + if ( (NULL != fc->armServiceSocket) && + (GNUNET_SYSERR == GNUNET_NETWORK_socket_close (fc->armServiceSocket)) ) - GNUNET_log_strerror (GNUNET_ERROR_TYPE_ERROR, "close"); - GNUNET_free (fc->listen_info->serviceName); - GNUNET_free (fc->listen_info->service_addr); - GNUNET_free (fc->listen_info); - GNUNET_free (fc); + { + GNUNET_log_strerror (GNUNET_ERROR_TYPE_ERROR, "close"); + fc->armServiceSocket = NULL; + } + if (fc->listen_info != NULL) + { + if (fc->listen_info->serviceName != NULL) + { + GNUNET_free (fc->listen_info->serviceName); + fc->listen_info->serviceName = NULL; + } + if (fc->listen_info->service_addr != NULL) + { + GNUNET_free (fc->listen_info->service_addr); + fc->listen_info->service_addr = NULL; + } + GNUNET_free (fc->listen_info); + fc->listen_info = NULL; + } + GNUNET_free (fc); + } } @@ -703,6 +712,11 @@ receiveFromClient (void *cls, const struct GNUNET_SCHEDULER_TaskContext *tc) GNUNET_TIME_UNIT_FOREVER_REL, fc->armServiceSocket, &forwardToService, fc); + else + /* We have not added any task with fc as a closure, so we're + * dropping our reference to fc + */ + fc->reference_count -= 1; } @@ -728,6 +742,12 @@ fc_acceptConnection (void *cls, GNUNET_free (fc->listen_info->service_addr); fc->listen_info->service_addr = sli->service_addr; fc->listen_info->service_addr_len = sli->service_addr_len; + /* Drop fc reference count prematurely, it'll be incremented + * once or twice in the following conditional branches. + * This is, apparently, the place where reference count increases + * past 1. + */ + fc->reference_count -= 1; if (fc->client_to_service_task == GNUNET_SCHEDULER_NO_TASK) { if (fc->client_to_service_bufferDataLength == 0) @@ -740,6 +760,7 @@ fc_acceptConnection (void *cls, GNUNET_SCHEDULER_add_write_net (GNUNET_TIME_UNIT_FOREVER_REL, fc->armServiceSocket, &forwardToService, fc); + fc->reference_count += 1; } if (fc->service_to_client_task == GNUNET_SCHEDULER_NO_TASK) { @@ -753,6 +774,7 @@ fc_acceptConnection (void *cls, GNUNET_SCHEDULER_add_write_net (GNUNET_TIME_UNIT_FOREVER_REL, fc->armClientSocket, &forwardToClient, fc); + fc->reference_count += 1; } GNUNET_free (sli); } @@ -951,6 +973,7 @@ accept_and_forward (struct ServiceListeningInfo *serviceListeningInfo) struct ForwardedConnection *fc; fc = GNUNET_malloc (sizeof (struct ForwardedConnection)); + fc->reference_count = 1; fc->listen_info = serviceListeningInfo; fc->service_to_client_bufferPos = fc->service_to_client_buffer; fc->client_to_service_bufferPos = fc->client_to_service_buffer; @@ -988,6 +1011,10 @@ accept_and_forward (struct ServiceListeningInfo *serviceListeningInfo) fc->armClientSocket, &receiveFromClient, fc); GNUNET_assert (GNUNET_SCHEDULER_NO_TASK == fc->start_task); + /* We're creating another chain of tasks for this fc that + * will have its own reference to it. + */ + fc->reference_count += 1; fc->start_task = GNUNET_SCHEDULER_add_now (&start_forwarding, fc); -- cgit v1.2.3