attempt to make the libc urandom override work for non-glibc too

sandbox/linux/services/libc_urandom_override.cc

Index: sandbox/linux/services/libc_urandom_override.cc
diff --git a/sandbox/linux/services/libc_urandom_override.cc b/sandbox/linux/services/libc_urandom_override.cc
index 8af42ebbcf1f26e13a2e6a26bee2501da4ef00e7..c42498dbddcd2c716c07367f1a3b5e82bf47f593 100644
--- a/sandbox/linux/services/libc_urandom_override.cc
+++ b/sandbox/linux/services/libc_urandom_override.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2013 The Chromium Authors. All rights reserved.

[jln (very slow on Chromium), 2013/06/26 23:49:47]

You can, but don't need to update this anymore.

[Mostyn Bramley-Moore, 2013/06/27 00:26:10]

I will revert this if you prefer?

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -16,16 +16,19 @@
 
 // Note: this file is used by the zygote and nacl_helper.
 
+#if !defined(HAVE_XSTAT) && defined(LIBC_GLIBC)
+#define HAVE_XSTAT 1
+#endif
+
+#if HAVE_XSTAT

[jln (very slow on Chromium), 2013/06/26 23:49:47]

#if defined(HAVE_XSTAT)

[Mostyn Bramley-Moore, 2013/06/27 00:26:10]

I would prefer to use the value of HAVE_XSTAT instead of the fact that it's
defined, so it is possible to easily override this later.  eg you could add
-DHAVE_XSTAT=0 to the compiler flags if some version of glibc appears that does
not support xstat.

It's not very likely, so I won't mind changing back to check
defined(HAVE_XSTAT), but I think the flexibility would be nice to have.

+// This is used when mapping stat* calls to __xstat* calls.
+#define XSTAT_VERSION 3

[jln (very slow on Chromium), 2013/06/26 23:49:47]

We really shouldn't hardcode this.

This would lead to very subtle bugs, and I don't think anyone wants to go back
to this code... ever :)

[Mostyn Bramley-Moore, 2013/06/27 00:26:10]

This is only used when we are overriding stat/stat64, where we don't have an
xstat version specified by the caller.

The only other options I can see to hardcoding it are ifdefing out the stat
override (like in patchset 5), or removing this "call xstat instead" fallback.

I don't like the latter, because then we have done something possibly unsafe:
provided a stat function that has a chance of failing due to not being able to
find the real stat implementation (possibly because it doesn't exist, and
headers are supposed to convert calling code into __xstat).

+#endif
+
 namespace sandbox {
 
 static bool g_override_urandom = false;
 
-void InitLibcUrandomOverrides() {
-  // Make sure /dev/urandom is open.
-  base::GetUrandomFD();
-  g_override_urandom = true;
-}
-
 // TODO(sergeyu): Currently this code doesn't work properly under ASAN
 // - it crashes content_unittests. Make sure it works properly and
 // enable it here. http://crbug.com/123263
@@ -34,16 +37,35 @@ void InitLibcUrandomOverrides() {
 static const char kUrandomDevPath[] = "/dev/urandom";
 
 typedef FILE* (*FopenFunction)(const char* path, const char* mode);
+typedef int (*StatFunction)(const char *path, struct stat *buf);
+typedef int (*Stat64Function)(const char *path, struct stat64 *buf);
+
+static pthread_once_t g_libc_file_io_funcs_guard = PTHREAD_ONCE_INIT;
+static FopenFunction g_libc_fopen = NULL;
+static FopenFunction g_libc_fopen64 = NULL;
+static StatFunction g_libc_stat = NULL;
+static Stat64Function g_libc_stat64 = NULL;
+
+#if HAVE_XSTAT

[jln (very slow on Chromium), 2013/06/26 23:49:47]

#if defined(HAVE_XSTAT)

 typedef int (*XstatFunction)(int version, const char *path, struct stat *buf);
 typedef int (*Xstat64Function)(int version, const char *path,
                                struct stat64 *buf);
 
-static pthread_once_t g_libc_file_io_funcs_guard = PTHREAD_ONCE_INIT;
-static FopenFunction g_libc_fopen;
-static FopenFunction g_libc_fopen64;
-static XstatFunction g_libc_xstat;
-static Xstat64Function g_libc_xstat64;
+static XstatFunction g_libc_xstat = NULL;
+static Xstat64Function g_libc_xstat64 = NULL;
+#endif  // HAVE_XSTAT
 
+void InitLibcUrandomOverrides() {
+  // Make sure /dev/urandom is open.
+  base::GetUrandomFD();
+  g_override_urandom = true;
+
+  CHECK_EQ(0, pthread_once(&g_libc_file_io_funcs_guard,
+              InitLibcFileIOFunctions());
+}
+
+// Find the libc's real fopen* and *stat* functions. This should only be
+// called once, and should be guarded by g_libc_file_io_funcs_guard.
 static void InitLibcFileIOFunctions() {
   g_libc_fopen = reinterpret_cast<FopenFunction>(
       dlsym(RTLD_NEXT, "fopen"));
@@ -59,9 +81,12 @@ static void InitLibcFileIOFunctions() {
     g_libc_fopen64 = g_libc_fopen;
   }
 
-#if defined(LIBC_GLIBC)
-  // TODO(sergeyu): This works only on systems with glibc. Fix it to
-  // work properly on other systems if necessary.
+  g_libc_stat = reinterpret_cast<StatFunction>(
+      dlsym(RTLD_NEXT, "stat"));
+  g_libc_stat64 = reinterpret_cast<Stat64Function>(
+      dlsym(RTLD_NEXT, "stat64"));
+
+#if HAVE_XSTAT

[jln (very slow on Chromium), 2013/06/26 23:49:47]

#if defined(HAVE_XSTAT)

   g_libc_xstat = reinterpret_cast<XstatFunction>(
       dlsym(RTLD_NEXT, "__xstat"));
   g_libc_xstat64 = reinterpret_cast<Xstat64Function>(
@@ -71,8 +96,16 @@ static void InitLibcFileIOFunctions() {
     LOG(FATAL) << "Failed to get __xstat() from libc.";
   }
   if (!g_libc_xstat64) {
-    LOG(WARNING) << "Failed to get __xstat64() from libc.";
+    LOG(FATAL) << "Failed to get __xstat64() from libc.";
+  }
+#else
+  if (!g_libc_stat) {
+    LOG(FATAL) << "Failed to get stat() from libc.";
+  }
+  if (!g_libc_stat64) {
+    LOG(FATAL) << "Failed to get stat64() from libc.";
   }
+#endif  // HAVE_XSTAT
 }
 
 // fopen() and fopen64() are intercepted here so that NSS can open
@@ -120,8 +153,11 @@ FILE* fopen64(const char* path, const char* mode) {
   }
 }
 
-// stat() is subject to the same problem as fopen(), so we have to use
-// the same trick to override it.
+// The stat() family of functions are subject to the same problem as
+// fopen(), so we have to use the same trick to override them.
+
+#if HAVE_XSTAT
+
 __attribute__ ((__visibility__("default")))
 int xstat_override(int version,
                    const char *path,
@@ -152,11 +188,57 @@ int xstat64_override(int version, const char *path, struct stat64 *buf) {
   } else {
     CHECK_EQ(0, pthread_once(&g_libc_file_io_funcs_guard,
                              InitLibcFileIOFunctions));
-    CHECK(g_libc_xstat64);
     return g_libc_xstat64(version, path, buf);
   }
 }
-#endif  // defined(LIBC_GLIBC)
+
+#endif  // HAVE_XSTAT
+
+__attribute__ ((__visibility__("default")))
+int stat_override(const char *path,
+                  struct stat *buf)  __asm__ ("stat");
+
+__attribute__ ((__visibility__("default")))
+int stat_override(const char *path, struct stat *buf) {
+  if (g_override_urandom && strcmp(path, kUrandomDevPath) == 0) {
+    int result = fstat(base::GetUrandomFD(), buf);
+    return result;
+  }

[jln (very slow on Chromium), 2013/06/26 23:49:47]

do you want to use the } else { construct as above, just for consistency ?

+
+  CHECK_EQ(0, pthread_once(&g_libc_file_io_funcs_guard,
+                           InitLibcFileIOFunctions));
+
+#if HAVE_XSTAT

[jln (very slow on Chromium), 2013/06/26 23:49:47]

#if defined(HAVE_XSTAT)

+  if (!g_libc_stat) {

[jln (very slow on Chromium), 2013/06/26 23:49:47]

So, if I'm not mistaken, since this stat is inlined on glibc with xstat, this
should never be called in practice, right?

Let's just RAW_CHECK(false, "stat called on xstat libc") with the proper comment
?

The comment should explain that we don't expect this to ever run on glibc
because of xstat, but that it's not commented out to prevent the code from
rotting.

[Mostyn Bramley-Moore, 2013/06/27 00:26:10]

Correct- this whole function should never be called on glibc, since the header
files convert stat* calls to __xstat*.  The only way I think it can be called if
code is built and linked against two different versions of glibc (unlikely but
not impossible).

But you asked for this code to be compiled even on glibc to avoid ifdefs, and
therefore we assume a responsibility to handle this call as best we can in all
situations, to avoid obscure failures.
RAW_CHECK doesn't take a message argument, is it OK if I leave a source code
comment instead?

+    return g_libc_xstat(XSTAT_VERSION, path, buf);
+  }
+#endif  // HAVE_XSTAT
+
+  return g_libc_stat(path, buf);
+}
+
+__attribute__ ((__visibility__("default")))
+int stat64_override(const char *path,
+                    struct stat64 *buf)  __asm__ ("stat64");
+
+__attribute__ ((__visibility__("default")))
+int stat64_override(const char *path, struct stat64 *buf) {
+  if (g_override_urandom && strcmp(path, kUrandomDevPath) == 0) {
+    int result = fstat64(base::GetUrandomFD(), buf);
+    return result;
+  }
+
+  CHECK_EQ(0, pthread_once(&g_libc_file_io_funcs_guard,
+                           InitLibcFileIOFunctions));
+
+#if HAVE_XSTAT
+  if (!g_libc_stat64) {
+    return g_libc_xstat64(XSTAT_VERSION, path, buf);

[jln (very slow on Chromium), 2013/06/26 23:49:47]

Same remark here of course.

+  }
+#endif  // HAVE_XSTAT
+
+  return g_libc_stat64(path, buf);
+}
 
 #endif  // !defined(ADDRESS_SANITIZER)