View Issue Details

IDProjectCategoryView StatusLast Update
0008786GNUnetutil librarypublic2024-06-08 12:03
Reporterulfvonbelow Assigned ToChristian Grothoff  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Platformx86-64OSGuix SystemOS Versiona1d711c92e
Product Version0.21.1 
Fixed in Version0.21.2 
Summary0008786: use-after-free in finish_client_drop when GNUNET_SERVICE_stop is used
DescriptionThe use-after-free is triggered when calling GNUNET_SERVICE_stop on a service
that has clients, which to my knowledge currently only occurs in test code,
thankfully. It occurs because GNUNET_SERVICE_stop calls
GNUNET_SERVICE_client_drop for each of those clients, which schedules a task
to run finish_client_drop, which attempts to read from the
GNUNET_SERVICE_Handle passed to GNUNET_SERVICE_stop. Immediately after
scheduling these tasks, it frees that same handle. This freeing needs to
happen only after finish_client_drop has been run for all the clients.
Steps To Reproduce1. ./configure --enable-sanitizer ; make ; make check
2. Observe use-after-free reported in a user of transport-testing-communicator.c
Additional InformationA proper fix without duplicating some code may be a bit involved, so for now I've attached a workaround that exploits the properties of the scheduler, which I've used in the process of getting the tests to pass.
TagsNo tags attached.
Attached Files
0001-util-temporary-workaround-for-use-after-free-in-fini.patch (2,867 bytes)   
From 6accb9703f2071af6f961eb8070ba4c873652ce0 Mon Sep 17 00:00:00 2001
From: ulfvonbelow <striness@tilde.club>
Date: Wed, 1 May 2024 22:32:13 -0500
Subject: [PATCH] util: temporary workaround for use-after-free in
 finish_client_drop.

The use-after-free is triggered when calling GNUNET_SERVICE_stop on a service
that has clients, which to my knowledge currently only occurs in test code,
thankfully. It occurs because GNUNET_SERVICE_stop calls
GNUNET_SERVICE_client_drop for each of those clients, which schedules a task
to run finish_client_drop, which attempts to read from the
GNUNET_SERVICE_Handle passed to GNUNET_SERVICE_stop. Immediately after
scheduling these tasks, it frees that same handle. This freeing needs to
happen only after finish_client_drop has been run for all the clients. The
workaround enqueues a task to free the handle, which due to the scheduler's
queueing behavior should cause it to run only after all the finish_client_drop
tasks for that handle have run. Just in case in the future the
finish_client_drop tasks are scheduled in such a way that this isn't the
case (for example, if they were scheduled with a lower priority), the freeing
task is also scheduled with GNUNET_SCHEDULER_PRIORITY_IDLE.

A proper fix would probably involve making the teardown code paths for
heap-allocated (created with GNUNET_SERVICE_start()) and
stack-allocated (created with GNUNET_SERVICE_run_()) more similar, or just
making it so that both use heap allocation. It's a bit strange to have both
GNUNET_SERVICE_stop and GNUNET_SERVICE_shutdown, which both do more or less
the same thing but in different contexts.
---
 src/lib/util/service.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/lib/util/service.c b/src/lib/util/service.c
index 7aeabf687..6354718de 100644
--- a/src/lib/util/service.c
+++ b/src/lib/util/service.c
@@ -1904,6 +1904,11 @@ GNUNET_SERVICE_start (const char *service_name,
 }
 
 
+static void free_thing (void *thing)
+{
+  GNUNET_free (thing);
+}
+
 /**
  * Stops a service that was started with #GNUNET_SERVICE_start().
  *
@@ -1916,10 +1921,17 @@ GNUNET_SERVICE_stop (struct GNUNET_SERVICE_Handle *srv)
 
   GNUNET_SERVICE_suspend (srv);
   while (NULL != (client = srv->clients_head))
-    GNUNET_SERVICE_client_drop (client);
+    GNUNET_SERVICE_client_drop (client); // schedules a task that needs srv
   teardown_service (srv);
   GNUNET_free (srv->handlers);
-  GNUNET_free (srv);
+  // ... frees srv
+  /* GNUNET_free (srv); */
+  /* XXX: this relies on either the queue behavior of the scheduler or the
+     priority level of the finish_client_drop task, but should work for
+     now. */
+  GNUNET_SCHEDULER_add_with_priority (GNUNET_SCHEDULER_PRIORITY_IDLE,
+                                      &free_thing,
+                                      srv);
 }
 
 
-- 
2.41.0

Activities

schanzen

2024-05-05 13:55

administrator   ~0022340

Assigning this to grothoff for review. The fix looks ok, but as the OP correctly identifies, the logic in the service seems off...

Christian Grothoff

2024-05-08 18:05

manager   ~0022375

Fixed in d65ae4ba7..db2eb78ec

schanzen

2024-06-08 12:03

administrator   ~0022546

0.21.2 released

Issue History

Date Modified Username Field Change
2024-05-03 02:57 ulfvonbelow New Issue
2024-05-03 02:57 ulfvonbelow File Added: 0001-util-temporary-workaround-for-use-after-free-in-fini.patch
2024-05-05 13:54 schanzen Assigned To => Christian Grothoff
2024-05-05 13:54 schanzen Status new => assigned
2024-05-05 13:55 schanzen Note Added: 0022340
2024-05-08 18:05 Christian Grothoff Status assigned => resolved
2024-05-08 18:05 Christian Grothoff Resolution open => fixed
2024-05-08 18:05 Christian Grothoff Fixed in Version => 0.21.2
2024-05-08 18:05 Christian Grothoff Note Added: 0022375
2024-06-08 12:03 schanzen Note Added: 0022546
2024-06-08 12:03 schanzen Status resolved => closed