code for encrypting sensitive tcmalloc metadata

third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h

+// Copyright (c) 2011, Google Inc.

[jschuh, 2011/09/13 16:37:12]

I'd rename this metadata_encrypt_posix.h. Also, I don't think /dev/urandom is
posix. So, maybe you should test for /dev/urandom first and fall back to
/dev/random (which I think is posix). Or, you might be able to just switch based
on a compile-time macro.

[bxx, 2011/09/13 19:36:09]

Done.

+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+//     * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+// ---
+// Author: Rebecca Shapiro
+//
+// Generic pointer encryption implementation.
+
+#ifndef TCMALLOC_METADATA_ENCRYPT_GENERIC_H_
+#define TCMALLOC_METADATA_ENCRYPT_GENERIC_H_
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdint.h>
+#include "base/logging.h"
+
+namespace {
+static uintptr_t key_;
+static bool initialized_ = false;

[jschuh, 2011/09/13 16:37:12]

You don't need a separate bool for initialized. Just check that key_ isn't zero.

[bxx, 2011/09/13 19:36:09]

Done.

+}
+
+namespace tcmalloc {
+
+void InitEncryption() {
+  // Ensure this function only executes once.

[jar (doing other things), 2011/09/13 18:15:18]

You also want assurances that this is only executed on one thread... presumably
during initialization before multi-threading has begun.

[bxx, 2011/09/13 20:06:58]

Initialization is done when TCMallocGuard() is constructed, this is something
that is called before multi-threading begins.  This should fail if called by
multiple threads (see next set of patches)

[bxx, 2011/09/13 20:06:58]

It gets initialized when TCMallocGuard() is constructed.  This is done before
multi-threading begins.  Wouldn't the static variables defined here be global
across all threads?

+  CHECK(initialized_ == false);
+  initialized_ == true;

[jschuh, 2011/09/13 16:37:12]

Never mark something as "true" until you've actually done it. ;)

[jar (doing other things), 2011/09/13 18:15:18]

+1  Very good coding style.

On 2011/09/13 16:37:12, Justin Schuh wrote:

[bxx, 2011/09/13 19:36:09]

Done.

+
+  // Read random bytes from urandom to initialize key.
+  int fd = open("/dev/urandom", O_RDONLY);
+  CHECK(fd > 0);
+  size_t uintp_size = sizeof(uintptr_t);
+  size_t num_read_bytes = 0;
+  while (num_read_bytes < uintp_size) {
+    size_t num_bytes = read(fd, reinterpret_cast<void *>(&key_)+num_read_bytes,

[jschuh, 2011/09/13 16:37:12]

Don't read directly into key. Better to store it in a local stack variable until
the operation is complete. then you can verify that your seed is non-zero (and
retry if needed). A related question: do we have a guarantee (or expectation)
that we're single threaded at this point, or should we CAS the key in?

[bxx, 2011/09/13 19:36:09]

The initialization is done when TCMallocGuard() is constructed.  Chrome creates
this before main() and any treads are created, according to the comments.  Given
my experience of trying to find a good spot to perform initialization, I am
willing to believe that it is not multithreaded at this point.
http://www.google.com/codesearch#OAMlx_jo-ck/src/third_party/tcmalloc/chromiu...

+                            uintp_size-num_read_bytes);
+    if (num_bytes < 0) {

[jar (doing other things), 2011/09/13 18:15:18]

Since num_bytes is unsigned, this is a very strange test (always false).

In addition, if num_bytes were signed, I doubt that we would want to continue
with the loop.

[bxx, 2011/09/13 20:06:58]

The man pages tell me read() returns -1 when there is an error.  I see chromium
also checks for negative values when using read().  But now I see that it
returns ssize_t, not size_t.
http://www.google.com/codesearch#hfE6470xZHk/base/file_util_posix.cc&q=readfr...

+      CHECK( errno == EINTR);
+    } else if (num_bytes == 0) {
+      break;
+    }
+    num_read_bytes += num_bytes;
+  }
+  CHECK(num_read_bytes == uintp_size);
+  close(fd);
+}
+
+uintptr_t EncryptUintptr(uintptr_t ptr) { return ptr ^ key_; }
+
+uintptr_t DecryptUintptr(uintptr_t ptr) { return EncryptUintptr(ptr); }
+
+}
+
+#endif // TCMALLOC_METADATA_ENCRYPT_GENERIC_H_