Buffer reads of urandom, if you promise no forking.

Callers that lack hardware random may obtain a speed improvement by
calling |RAND_enable_fork_unsafe_buffering|, which enables a
thread-local buffer around reads from /dev/urandom.

Change-Id: I46e675d1679b20434dd520c58ece0f888f38a241
Reviewed-on: https://boringssl-review.googlesource.com/5792
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/internal.h b/crypto/internal.h
index a502d20..713659d 100644
--- a/crypto/internal.h
+++ b/crypto/internal.h
@@ -452,6 +452,7 @@
 typedef enum {
   OPENSSL_THREAD_LOCAL_ERR = 0,
   OPENSSL_THREAD_LOCAL_RAND,
+  OPENSSL_THREAD_LOCAL_URANDOM_BUF,
   OPENSSL_THREAD_LOCAL_TEST,
   NUM_OPENSSL_THREAD_LOCALS,
 } thread_local_data_t;
diff --git a/crypto/rand/CMakeLists.txt b/crypto/rand/CMakeLists.txt
index 35d5290..8e639c2 100644
--- a/crypto/rand/CMakeLists.txt
+++ b/crypto/rand/CMakeLists.txt
@@ -22,3 +22,12 @@
 )
 
 perlasm(rdrand-x86_64.${ASM_EXT} asm/rdrand-x86_64.pl)
+
+add_executable(
+  urandom_test
+
+  urandom_test.cc
+  $<TARGET_OBJECTS:test_support>
+)
+
+target_link_libraries(urandom_test crypto)
diff --git a/crypto/rand/urandom.c b/crypto/rand/urandom.c
index d7ed5c6..1cc5260 100644
--- a/crypto/rand/urandom.c
+++ b/crypto/rand/urandom.c
@@ -30,109 +30,126 @@
 
 
 /* This file implements a PRNG by reading from /dev/urandom, optionally with a
- * fork-safe buffer.
- *
- * If buffering is enabled then it maintains a global, linked list of buffers.
- * Threads which need random bytes grab a buffer from the list under a lock and
- * copy out the bytes that they need. In the rare case that the buffer is
- * empty, it's refilled from /dev/urandom outside of the lock.
- *
- * Large requests are always serviced from /dev/urandom directly.
- *
- * Each buffer contains the PID of the process that created it and it's tested
- * against the current PID each time. Thus processes that fork will discard all
- * the buffers filled by the parent process. There are two problems with this:
- *
- * 1) glibc maintains a cache of the current PID+PPID and, if this cache isn't
- *    correctly invalidated, the getpid() will continue to believe that
- *    it's the old process. Glibc depends on the glibc wrappers for fork,
- *    vfork and clone being used in order to invalidate the getpid() cache.
- *
- * 2) If a process forks, dies and then its child forks, it's possible that
- *    the third process will end up with the same PID as the original process.
- *    If the second process never used any random values then this will mean
- *    that the third process has stale, cached values and won't notice.
- */
+ * buffer, which is unsafe across |fork|. */
 
-/* BUF_SIZE is intended to be a 4K allocation with malloc overhead. struct
- * rand_buffer also fits in this space and the remainder is entropy. */
-#define BUF_SIZE (4096 - 16)
+#define BUF_SIZE 4096
 
-/* rand_buffer contains unused, random bytes. These structures form a linked
- * list via the |next| pointer, which is NULL in the final element. */
+/* rand_buffer contains unused, random bytes, some of which may have been
+ * consumed already. */
 struct rand_buffer {
-  size_t used; /* used contains the number of bytes of |rand| that have
-                  been consumed. */
-  struct rand_buffer *next;
-  pid_t pid; /* pid contains the pid at the time that the buffer was
-                created so that data is not duplicated after a fork. */
-  pid_t ppid; /* ppid contains the parent pid in order to try and reduce
-                 the possibility of duplicated PID confusing the
-                 detection of a fork. */
-  uint8_t rand[];
+  size_t used;
+  uint8_t rand[BUF_SIZE];
 };
 
-/* rand_bytes_per_buf is the number of actual entropy bytes in a buffer. */
-static const size_t rand_bytes_per_buf = BUF_SIZE - sizeof(struct rand_buffer);
+/* requested_lock is used to protect the |*_requested| variables. */
+static struct CRYPTO_STATIC_MUTEX requested_lock = CRYPTO_STATIC_MUTEX_INIT;
 
-static struct CRYPTO_STATIC_MUTEX global_lock = CRYPTO_STATIC_MUTEX_INIT;
+/* urandom_fd_requested is set by |RAND_set_urandom_fd|.  It's protected by
+ * |requested_lock|. */
+static int urandom_fd_requested = -2;
 
-/* list_head is the start of a global, linked-list of rand_buffer objects. It's
- * protected by |global_lock|. */
-static struct rand_buffer *list_head;
-
-/* urandom_fd is a file descriptor to /dev/urandom. It's protected by
- * |global_lock|. */
+/* urandom_fd is a file descriptor to /dev/urandom. It's protected by |once|. */
 static int urandom_fd = -2;
 
+/* urandom_buffering_requested is set by |RAND_enable_fork_unsafe_buffering|.
+ * It's protected by |requested_lock|. */
+static int urandom_buffering_requested = 0;
+
 /* urandom_buffering controls whether buffering is enabled (1) or not (0). This
- * is protected by |global_lock|. */
+ * is protected by |once|. */
 static int urandom_buffering = 0;
 
-/* urandom_get_fd_locked returns a file descriptor to /dev/urandom. The caller
- * of this function must hold |global_lock|. */
-static int urandom_get_fd_locked(void) {
-  if (urandom_fd != -2) {
-    return urandom_fd;
+static CRYPTO_once_t once = CRYPTO_ONCE_INIT;
+
+/* init_once initializes the state of this module to values previously
+ * requested. This is the only function that modifies |urandom_fd| and
+ * |urandom_buffering|, whose values may be read safely after calling the
+ * once. */
+static void init_once(void) {
+  CRYPTO_STATIC_MUTEX_lock_read(&requested_lock);
+  urandom_buffering = urandom_buffering_requested;
+  int fd = urandom_fd_requested;
+  CRYPTO_STATIC_MUTEX_unlock(&requested_lock);
+
+  if (fd == -2) {
+    do {
+      fd = open("/dev/urandom", O_RDONLY);
+    } while (fd == -1 && errno == EINTR);
   }
 
-  do {
-    urandom_fd = open("/dev/urandom", O_RDONLY);
-  } while (urandom_fd == -1 && errno == EINTR);
-  return urandom_fd;
+  if (fd < 0) {
+    abort();
+  }
+
+  int flags = fcntl(fd, F_GETFD);
+  if (flags == -1) {
+    abort();
+  }
+  flags |= FD_CLOEXEC;
+  if (fcntl(fd, F_SETFD, flags) == -1) {
+    abort();
+  }
+  urandom_fd = fd;
 }
 
-/* RAND_cleanup frees all buffers, closes any cached file descriptor
- * and resets the global state. */
-void RAND_cleanup(void) {
-  struct rand_buffer *cur;
-
-  CRYPTO_STATIC_MUTEX_lock_write(&global_lock);
-  while ((cur = list_head)) {
-    list_head = cur->next;
-    OPENSSL_free(cur);
-  }
-  if (urandom_fd >= 0) {
-    close(urandom_fd);
-  }
-  urandom_fd = -2;
-  list_head = NULL;
-  CRYPTO_STATIC_MUTEX_unlock(&global_lock);
-}
+void RAND_cleanup(void) {}
 
 void RAND_set_urandom_fd(int fd) {
-  CRYPTO_STATIC_MUTEX_lock_write(&global_lock);
-  if (urandom_fd != -2) {
-    /* |RAND_set_urandom_fd| may not be called after the RNG is used. */
+  fd = dup(fd);
+  if (fd < 0) {
     abort();
   }
-  do {
-    urandom_fd = dup(fd);
-  } while (urandom_fd == -1 && errno == EINTR);
-  if (urandom_fd < 0) {
-    abort();
+
+  CRYPTO_STATIC_MUTEX_lock_write(&requested_lock);
+  urandom_fd_requested = fd;
+  CRYPTO_STATIC_MUTEX_unlock(&requested_lock);
+
+  CRYPTO_once(&once, init_once);
+  if (urandom_fd != fd) {
+    abort();  // Already initialized.
   }
-  CRYPTO_STATIC_MUTEX_unlock(&global_lock);
+}
+
+void RAND_enable_fork_unsafe_buffering(int fd) {
+  if (fd >= 0) {
+    fd = dup(fd);
+    if (fd < 0) {
+      abort();
+    }
+  } else {
+    fd = -2;
+  }
+
+  CRYPTO_STATIC_MUTEX_lock_write(&requested_lock);
+  urandom_buffering_requested = 1;
+  urandom_fd_requested = fd;
+  CRYPTO_STATIC_MUTEX_unlock(&requested_lock);
+
+  CRYPTO_once(&once, init_once);
+  if (urandom_buffering != 1 || (fd >= 0 && urandom_fd != fd)) {
+    abort();  // Already initialized.
+  }
+}
+
+static struct rand_buffer *get_thread_local_buffer(void) {
+  struct rand_buffer *buf =
+      CRYPTO_get_thread_local(OPENSSL_THREAD_LOCAL_URANDOM_BUF);
+  if (buf != NULL) {
+    return buf;
+  }
+
+  buf = OPENSSL_malloc(sizeof(struct rand_buffer));
+  if (buf == NULL) {
+    return NULL;
+  }
+  buf->used = BUF_SIZE;  /* To trigger a |read_full| on first use. */
+  if (!CRYPTO_set_thread_local(OPENSSL_THREAD_LOCAL_URANDOM_BUF, buf,
+                               OPENSSL_free)) {
+    OPENSSL_free(buf);
+    return NULL;
+  }
+
+  return buf;
 }
 
 /* read_full reads exactly |len| bytes from |fd| into |out| and returns 1. In
@@ -155,110 +172,48 @@
   return 1;
 }
 
-/* CRYPTO_sysrand puts |num| random bytes into |out|. */
-void CRYPTO_sysrand(uint8_t *out, size_t requested) {
-  int fd;
-  struct rand_buffer *buf;
-  size_t todo;
-  pid_t pid, ppid;
+/* read_from_buffer reads |requested| random bytes from the buffer into |out|,
+ * refilling it if necessary to satisfy the request. */
+static void read_from_buffer(struct rand_buffer *buf,
+                             uint8_t *out, size_t requested) {
+  size_t remaining = BUF_SIZE - buf->used;
 
+  while (requested > remaining) {
+    memcpy(out, &buf->rand[buf->used], remaining);
+    buf->used += remaining;
+    out += remaining;
+    requested -= remaining;
+
+    if (!read_full(urandom_fd, buf->rand, BUF_SIZE)) {
+      abort();
+      return;
+    }
+    buf->used = 0;
+    remaining = BUF_SIZE;
+  }
+
+  memcpy(out, &buf->rand[buf->used], requested);
+  buf->used += requested;
+}
+
+/* CRYPTO_sysrand puts |requested| random bytes into |out|. */
+void CRYPTO_sysrand(uint8_t *out, size_t requested) {
   if (requested == 0) {
     return;
   }
 
-  CRYPTO_STATIC_MUTEX_lock_write(&global_lock);
-  fd = urandom_get_fd_locked();
+  CRYPTO_once(&once, init_once);
+  if (urandom_buffering && requested < BUF_SIZE) {
+    struct rand_buffer *buf = get_thread_local_buffer();
+    if (buf != NULL) {
+      read_from_buffer(buf, out, requested);
+      return;
+    }
+  }
 
-  if (fd < 0) {
-    CRYPTO_STATIC_MUTEX_unlock(&global_lock);
+  if (!read_full(urandom_fd, out, requested)) {
     abort();
-    return;
   }
-
-  /* If buffering is not enabled, or if the request is large, then the
-   * result comes directly from urandom. */
-  if (!urandom_buffering || requested > BUF_SIZE / 2) {
-    CRYPTO_STATIC_MUTEX_unlock(&global_lock);
-    if (!read_full(fd, out, requested)) {
-      abort();
-    }
-    return;
-  }
-
-  pid = getpid();
-  ppid = getppid();
-
-  for (;;) {
-    buf = list_head;
-    if (buf && buf->pid == pid && buf->ppid == ppid &&
-        rand_bytes_per_buf - buf->used >= requested) {
-      memcpy(out, &buf->rand[buf->used], requested);
-      buf->used += requested;
-      CRYPTO_STATIC_MUTEX_unlock(&global_lock);
-      return;
-    }
-
-    /* If we don't immediately have enough entropy with the correct
-     * PID, remove the buffer from the list in order to gain
-     * exclusive access and unlock. */
-    if (buf) {
-      list_head = buf->next;
-    }
-    CRYPTO_STATIC_MUTEX_unlock(&global_lock);
-
-    if (!buf) {
-      buf = (struct rand_buffer *)OPENSSL_malloc(BUF_SIZE);
-      if (!buf) {
-        abort();
-        return;
-      }
-      /* The buffer doesn't contain any random bytes yet
-       * so we mark it as fully used so that it will be
-       * filled below. */
-      buf->used = rand_bytes_per_buf;
-      buf->next = NULL;
-      buf->pid = pid;
-      buf->ppid = ppid;
-    }
-
-    if (buf->pid == pid && buf->ppid == ppid) {
-      break;
-    }
-
-    /* We have forked and so cannot use these bytes as they
-     * may have been used in another process. */
-    OPENSSL_free(buf);
-    CRYPTO_STATIC_MUTEX_lock_write(&global_lock);
-  }
-
-  while (requested > 0) {
-    todo = rand_bytes_per_buf - buf->used;
-    if (todo > requested) {
-      todo = requested;
-    }
-    memcpy(out, &buf->rand[buf->used], todo);
-    requested -= todo;
-    out += todo;
-    buf->used += todo;
-
-    if (buf->used < rand_bytes_per_buf) {
-      break;
-    }
-
-    if (!read_full(fd, buf->rand, rand_bytes_per_buf)) {
-      OPENSSL_free(buf);
-      abort();
-      return;
-    }
-
-    buf->used = 0;
-  }
-
-  CRYPTO_STATIC_MUTEX_lock_write(&global_lock);
-  assert(list_head != buf);
-  buf->next = list_head;
-  list_head = buf;
-  CRYPTO_STATIC_MUTEX_unlock(&global_lock);
 }
 
 #endif  /* !OPENSSL_WINDOWS */
diff --git a/crypto/rand/urandom_test.cc b/crypto/rand/urandom_test.cc
new file mode 100644
index 0000000..956efdf
--- /dev/null
+++ b/crypto/rand/urandom_test.cc
@@ -0,0 +1,52 @@
+/* Copyright (c) 2015, Google Inc.
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
+ * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
+ * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
+ * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */
+
+#include <stdio.h>
+#include <string.h>
+
+#include <openssl/rand.h>
+#include <openssl/crypto.h>
+
+
+extern "C" {
+extern void CRYPTO_sysrand(uint8_t *out, size_t requested);
+}
+
+static bool TestBuffering() {
+  RAND_enable_fork_unsafe_buffering(-1);
+  uint8_t buf[4096];
+
+  memset(buf, 0, sizeof(buf));
+  static const size_t kBufSize = 4096;
+  CRYPTO_sysrand(buf, 0);
+  CRYPTO_sysrand(buf, 2048);                      /* fills the buffer */
+  CRYPTO_sysrand(buf, kBufSize - 1);              /* triggers a second fill */
+  CRYPTO_sysrand(buf, kBufSize - 2048 + 1);       /* consumes the remainder */
+  CRYPTO_sysrand(buf, 4096);                      /* bypasses the buffer */
+
+  /* Lame, but might as well sanity check that something happened. */
+  uint8_t cmp[4096];
+  memset(cmp, 0, sizeof(cmp));
+  return memcmp(buf, cmp, 4096) != 0;
+}
+
+int main() {
+  CRYPTO_library_init();
+  if (!TestBuffering()) {
+    return false;
+  }
+
+  printf("PASS\n");
+  return 0;
+}
diff --git a/include/openssl/rand.h b/include/openssl/rand.h
index 335c76e..de1bd8d 100644
--- a/include/openssl/rand.h
+++ b/include/openssl/rand.h
@@ -40,11 +40,26 @@
  * randomness rather opening /dev/urandom internally. The caller retains
  * ownership of |fd| and is at liberty to close it at any time. This is useful
  * if, due to a sandbox, /dev/urandom isn't available. If used, it must be
- * called before |RAND_bytes| is called in the current address space.
+ * called before the first call to |RAND_bytes|, and it is mutually exclusive
+ * with |RAND_enable_fork_unsafe_buffering|.
  *
  * |RAND_set_urandom_fd| does not buffer any entropy, so it is safe to call
- * |fork| between |RAND_set_urandom_fd| and the first call to |RAND_bytes|. */
+ * |fork| at any time after calling |RAND_set_urandom_fd|. */
 OPENSSL_EXPORT void RAND_set_urandom_fd(int fd);
+
+/* RAND_enable_fork_unsafe_buffering enables efficient buffered reading of
+ * /dev/urandom. It adds an overhead of a few KB of memory per thread. It must
+ * be called before the first call to |RAND_bytes| and it is mutually exclusive
+ * with calls to |RAND_set_urandom_fd|.
+ *
+ * If |fd| is non-negative then a copy of |fd| will be used rather than opening
+ * /dev/urandom internally. Like |RAND_set_urandom_fd|, the caller retains
+ * ownership of |fd|. If |fd| is negative then /dev/urandom will be opened and
+ * any error from open(2) crashes the address space.
+ *
+ * It has an unusual name because the buffer is unsafe across calls to |fork|.
+ * Hence, this function should never be called by libraries. */
+OPENSSL_EXPORT void RAND_enable_fork_unsafe_buffering(int fd);
 #endif
 
 
diff --git a/util/all_tests.json b/util/all_tests.json
index a6daa2f..41924c6 100644
--- a/util/all_tests.json
+++ b/util/all_tests.json
@@ -47,6 +47,7 @@
 	["crypto/pkcs8/pkcs8_test"],
 	["crypto/pkcs8/pkcs12_test"],
 	["crypto/poly1305/poly1305_test", "crypto/poly1305/poly1305_test.txt"],
+	["crypto/rand/urandom_test"],
 	["crypto/refcount_test"],
 	["crypto/rsa/rsa_test"],
 	["crypto/thread_test"],