double connection_free() in dns_resolve()

[arma]

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."

[arma]

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.

[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.

[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.

[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.

[nickm]

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