[or-cvs] r17672: {tor} Fix bug 889: share deep-copied keys between threads to avoid (in tor/trunk: . src/common src/or)

[nickm at seul.org]

Author: nickm
Date: 2008-12-18 00:28:27 -0500 (Thu, 18 Dec 2008)
New Revision: 17672

Modified:
   tor/trunk/ChangeLog
   tor/trunk/src/common/crypto.c
   tor/trunk/src/common/crypto.h
   tor/trunk/src/or/router.c
   tor/trunk/src/or/test.c
Log:
Fix bug 889: share deep-copied keys between threads to avoid races in reference counts.  Bugfix on 0.1.0.1-rc.

Modified: tor/trunk/ChangeLog
===================================================================
--- tor/trunk/ChangeLog	2008-12-18 05:15:11 UTC (rev 17671)
+++ tor/trunk/ChangeLog	2008-12-18 05:28:27 UTC (rev 17672)
@@ -66,6 +66,9 @@
     - Clip the CircuitBuildTimeout to a minimum of 30 seconds. Warn the
       user if lower values are given in the configuration. Bugfix on
       0.1.1.17-rc. Patch by Sebastian.
+    - Fix a race condition when freeing keys shared between main thread
+      and CPU workers that could result in a memory leak.  Bugfix on
+      0.1.0.1-rc.  Fixes bug 889.
 
   o Minor bugfixes (hidden services):
     - Do not throw away existing introduction points on SIGHUP; bugfix on

Modified: tor/trunk/src/common/crypto.c
===================================================================
--- tor/trunk/src/common/crypto.c	2008-12-18 05:15:11 UTC (rev 17671)
+++ tor/trunk/src/common/crypto.c	2008-12-18 05:28:27 UTC (rev 17672)
@@ -672,6 +672,23 @@
   return env;
 }
 
+/** Make a real honest-to-goodness copy of <b>env</b>, and return it. */
+crypto_pk_env_t *
+crypto_pk_copy_full(crypto_pk_env_t *env)
+{
+  RSA *new_key;
+  tor_assert(env);
+  tor_assert(env->key);
+
+  if (PRIVATE_KEY_OK(env)) {
+    new_key = RSAPrivateKey_dup(env->key);
+  } else {
+    new_key = RSAPublicKey_dup(env->key);
+  }
+
+  return _crypto_new_pk_env_rsa(new_key);
+}
+
 /** Encrypt <b>fromlen</b> bytes from <b>from</b> with the public key
  * in <b>env</b>, using the padding method <b>padding</b>.  On success,
  * write the result to <b>to</b>, and return the number of bytes

Modified: tor/trunk/src/common/crypto.h
===================================================================
--- tor/trunk/src/common/crypto.h	2008-12-18 05:15:11 UTC (rev 17671)
+++ tor/trunk/src/common/crypto.h	2008-12-18 05:28:27 UTC (rev 17672)
@@ -92,6 +92,7 @@
 int crypto_pk_cmp_keys(crypto_pk_env_t *a, crypto_pk_env_t *b);
 size_t crypto_pk_keysize(crypto_pk_env_t *env);
 crypto_pk_env_t *crypto_pk_dup_key(crypto_pk_env_t *orig);
+crypto_pk_env_t *crypto_pk_copy_full(crypto_pk_env_t *orig);
 int crypto_pk_key_is_private(const crypto_pk_env_t *key);
 
 int crypto_pk_public_encrypt(crypto_pk_env_t *env, char *to,

Modified: tor/trunk/src/or/router.c
===================================================================
--- tor/trunk/src/or/router.c	2008-12-18 05:15:11 UTC (rev 17671)
+++ tor/trunk/src/or/router.c	2008-12-18 05:28:27 UTC (rev 17672)
@@ -75,8 +75,8 @@
   return onionkey;
 }
 
-/** Store a copy of the current onion key into *<b>key</b>, and a copy
- * of the most recent onion key into *<b>last</b>.
+/** Store a full copy of the current onion key into *<b>key</b>, and a full
+ * copy of the most recent onion key into *<b>last</b>.
  */
 void
 dup_onion_keys(crypto_pk_env_t **key, crypto_pk_env_t **last)
@@ -85,9 +85,9 @@
   tor_assert(last);
   tor_mutex_acquire(key_lock);
   tor_assert(onionkey);
-  *key = crypto_pk_dup_key(onionkey);
+  *key = crypto_pk_copy_full(onionkey);
   if (lastonionkey)
-    *last = crypto_pk_dup_key(lastonionkey);
+    *last = crypto_pk_copy_full(lastonionkey);
   else
     *last = NULL;
   tor_mutex_release(key_lock);

Modified: tor/trunk/src/or/test.c
===================================================================
--- tor/trunk/src/or/test.c	2008-12-18 05:15:11 UTC (rev 17671)
+++ tor/trunk/src/or/test.c	2008-12-18 05:28:27 UTC (rev 17672)
@@ -740,6 +740,14 @@
       test_memeq(data1,data3,j);
     }
   }
+
+  /* Try copy_full */
+  crypto_free_pk_env(pk2);
+  pk2 = crypto_pk_copy_full(pk1);
+  test_assert(pk2 != NULL);
+  test_neq_ptr(pk1, pk2);
+  test_assert(crypto_pk_cmp_keys(pk1,pk2) == 0);
+
  done:
   if (pk1)
     crypto_free_pk_env(pk1);