[tor-commits] [tor/release-0.2.3] Avoid hard (impossible?)-to-trigger double-free in dns_resolve()

[nickm at torproject.org]

commit 62637fa22405278758febb1743da9af562524d4c
Author: Nick Mathewson <nickm at torproject.org>
Date:   Tue Jul 31 12:58:19 2012 -0400

    Avoid hard (impossible?)-to-trigger double-free in dns_resolve()
    
    Fixes 6480; fix on 0.2.0.1-alpha; based on pseudonymous patch.
---
 changes/bug6480 |    5 +++++
 src/or/dns.c    |   30 ++++++++++++++++++------------
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/changes/bug6480 b/changes/bug6480
new file mode 100644
index 0000000..83ae00b
--- /dev/null
+++ b/changes/bug6480
@@ -0,0 +1,5 @@
+  o Major bugfixes:
+    - Avoid read-from-freed-RAM bug and related double-free bug that
+      could occur when a DNS request fails while launching it.  Fixes
+      bug 6480; bugfix on 0.2.0.1-alpha.
+
diff --git a/src/or/dns.c b/src/or/dns.c
index da11668..3e88fad 100644
--- a/src/or/dns.c
+++ b/src/or/dns.c
@@ -168,7 +168,8 @@ static void add_wildcarded_test_address(const char *address);
 static int configure_nameservers(int force);
 static int answer_is_wildcarded(const char *ip);
 static int dns_resolve_impl(edge_connection_t *exitconn, int is_resolve,
-                            or_circuit_t *oncirc, char **resolved_to_hostname);
+                            or_circuit_t *oncirc, char **resolved_to_hostname,
+                            int *made_connection_pending_out);
 #ifdef DEBUG_DNS_CACHE
 static void _assert_cache_ok(void);
 #define assert_cache_ok() _assert_cache_ok()
@@ -596,10 +597,12 @@ dns_resolve(edge_connection_t *exitconn)
 {
   or_circuit_t *oncirc = TO_OR_CIRCUIT(exitconn->on_circuit);
   int is_resolve, r;
+  int made_connection_pending = 0;
   char *hostname = NULL;
   is_resolve = exitconn->_base.purpose == EXIT_PURPOSE_RESOLVE;
 
-  r = dns_resolve_impl(exitconn, is_resolve, oncirc, &hostname);
+  r = dns_resolve_impl(exitconn, is_resolve, oncirc, &hostname,
+                       &made_connection_pending);
 
   switch (r) {
     case 1:
@@ -639,16 +642,11 @@ dns_resolve(edge_connection_t *exitconn)
 
       dns_cancel_pending_resolve(exitconn->_base.address);
 
-      if (!exitconn->_base.marked_for_close) {
+      if (!made_connection_pending && !exitconn->_base.marked_for_close) {
+        /* If we made the connection pending, then we freed it already in
+         * dns_cancel_pending_resolve().  If we marked it for close, it'll
+         * get freed from the main loop.  Otherwise, can free it now. */
         connection_free(TO_CONN(exitconn));
-        // XXX ... and we just leak exitconn otherwise? -RD
-        // If it's marked for close, it's on closeable_connection_lst in
-        // main.c.  If it's on the closeable list, it will get freed from
-        // main.c. -NM
-        // "<armadev> If that's true, there are other bugs around, where we
-        //  don't check if it's marked, and will end up double-freeing."
-        // On the other hand, I don't know of any actual bugs here, so this
-        // shouldn't be holding up the rc. -RD
       }
       break;
     default:
@@ -667,10 +665,15 @@ dns_resolve(edge_connection_t *exitconn)
  * Return -2 on a transient error. If it's a reverse resolve and it's
  * successful, sets *<b>hostname_out</b> to a newly allocated string
  * holding the cached reverse DNS value.
+ *
+ * Set *<b>made_connection_pending_out</b> to true if we have placed
+ * <b>exitconn</b> on the list of pending connections for some resolve; set it
+ * to false otherwise.
  */
 static int
 dns_resolve_impl(edge_connection_t *exitconn, int is_resolve,
-                 or_circuit_t *oncirc, char **hostname_out)
+                 or_circuit_t *oncirc, char **hostname_out,
+                 int *made_connection_pending_out)
 {
   cached_resolve_t *resolve;
   cached_resolve_t search;
@@ -684,6 +687,7 @@ dns_resolve_impl(edge_connection_t *exitconn, int is_resolve,
   tor_assert(!SOCKET_OK(exitconn->_base.s));
   assert_cache_ok();
   tor_assert(oncirc);
+  *made_connection_pending_out = 0;
 
   /* first check if exitconn->_base.address is an IP. If so, we already
    * know the answer. */
@@ -757,6 +761,7 @@ dns_resolve_impl(edge_connection_t *exitconn, int is_resolve,
         pending_connection->conn = exitconn;
         pending_connection->next = resolve->pending_connections;
         resolve->pending_connections = pending_connection;
+        *made_connection_pending_out = 1;
         log_debug(LD_EXIT,"Connection (fd %d) waiting for pending DNS "
                   "resolve of %s", exitconn->_base.s,
                   escaped_safe_str(exitconn->_base.address));
@@ -797,6 +802,7 @@ dns_resolve_impl(edge_connection_t *exitconn, int is_resolve,
   pending_connection = tor_malloc_zero(sizeof(pending_connection_t));
   pending_connection->conn = exitconn;
   resolve->pending_connections = pending_connection;
+  *made_connection_pending_out = 1;
 
   /* Add this resolve to the cache and priority queue. */
   HT_INSERT(cache_map, &cache_root, resolve);