Opened 21 months ago

Closed 4 weeks ago

#6480 closed defect (fixed)

double connection_free() in dns_resolve()

Reported by: arma Owned by:
Priority: normal Milestone: Tor: 0.2.3.x-final
Component: Tor Version:
Keywords: tor-relay 022-backport Cc:
Actual Points: Parent ID:
Points:

Description

If dns_resolve()'s call to dns_resolve_impl() returns -1, it ends up running

      dns_cancel_pending_resolve(exitconn->_base.address);

      if (!exitconn->_base.marked_for_close) {
        connection_free(TO_CONN(exitconn));

But dns_cancel_pending_resolve() runs

  while (resolve->pending_connections) {
...
    if (!pendconn->_base.marked_for_close)
      connection_free(TO_CONN(pendconn));

So we would end up calling connection_free() on it twice. But we don't in practice, since the first connection_free() scribbles 0xCC on it, which sets marked_for_close to true, so we don't free it the second time! Cue Nick's circus music.

Our friendly irc person says "fix not so easy btw, connection_free() still need to call if no it was attached to pending resolve list."

Child Tickets

Change History (10)

comment:1 Changed 21 months ago by arma

  • Status changed from new to needs_review

Our friendly irc person suggests this patch:

--- src/or/dns.c
+++ src/or/dns.mod.c
@@ -168,7 +168,8 @@
 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 *pended_connection);
 #ifdef DEBUG_DNS_CACHE
 static void _assert_cache_ok(void);
 #define assert_cache_ok() _assert_cache_ok()
@@ -597,9 +598,11 @@
   or_circuit_t *oncirc = TO_OR_CIRCUIT(exitconn->on_circuit);
   int is_resolve, r;
   char *hostname = NULL;
+  int pended_connection = 0;
   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,
+                       &pended_connection);
 
   switch (r) {
     case 1:
@@ -639,7 +642,7 @@
 
       dns_cancel_pending_resolve(exitconn->_base.address);
 
-      if (!exitconn->_base.marked_for_close) {
+      if (!pended_connection && !exitconn->_base.marked_for_close) {
         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
@@ -670,7 +673,8 @@
  */
 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 *pended_connection)
 {
   cached_resolve_t *resolve;
   cached_resolve_t search;
@@ -797,6 +801,7 @@
   pending_connection = tor_malloc_zero(sizeof(pending_connection_t));
   pending_connection->conn = exitconn;
   resolve->pending_connections = pending_connection;
+  *pended_connection = 1;
 
   /* Add this resolve to the cache and priority queue. */
   HT_INSERT(cache_map, &cache_root, resolve);

which looks pretty straightforward.

I assigned the ticket to 0.2.4 originally since it isn't occurring in practice. We might want to move that to 0.2.3 if we become confident of the diagnosis and fix.

comment:2 Changed 21 months ago by nickm

Adapted that patch, and added some more comments, in branch "bug6480" in my public repo. Personally I'd like to stick it in 0.2.3, but if you think it's risky, we can try it out in 0.2.4.x for a bit first.

comment:3 Changed 21 months ago by nickm

Based on a conversation on IRC, I now think that maybe this should be "major" and in 0.2.3 with a possible backport to 0.2.2. It *is* a remote freed-memory read. If somebody's running Tor under valgrind, or with a particularly aggressive/paranoid malloc implementation, this could turn into a remote crash for them.

comment:4 Changed 21 months ago by nickm

  • Milestone changed from Tor: 0.2.4.x-final to Tor: 0.2.3.x-final

comment:5 Changed 21 months ago by arma

I think putting it in 0.2.3 is fine.

I don't think backporting to 0.2.2 will be worth our while. I want 0.2.2 to be solidly frozen except for super critical things.

comment:6 Changed 21 months ago by nickm

  • Milestone changed from Tor: 0.2.3.x-final to Tor: 0.2.2.x-final

Okay. Merged "bug6480_squashed" to maint-0.2.3. Moving this ticket to 0.2.2, just in case.

comment:7 Changed 19 months ago by nickm

  • Keywords tor-relay added

comment:8 Changed 19 months ago by nickm

  • Component changed from Tor Relay to Tor

comment:9 Changed 13 months ago by nickm

  • Keywords 022-backport added

comment:10 Changed 4 weeks ago by nickm

  • Milestone changed from Tor: 0.2.2.x-final to Tor: 0.2.3.x-final
  • Resolution set to fixed
  • Status changed from needs_review to closed

We're dropping 0.2.2 servers from the Tor network. Nothing gets backported to 0.2.2 for server-side at this point.

Note: See TracTickets for help on using tickets.