Avoid std::string copying in GetFileNameInWhitelist.

sandbox/linux/services/broker_process_utils.cc

Index: sandbox/linux/services/broker_process_utils.cc
diff --git a/sandbox/linux/services/broker_process_utils.cc b/sandbox/linux/services/broker_process_utils.cc
new file mode 100644
index 0000000000000000000000000000000000000000..fed02622334578e31f3b2c3925fd3cf33e38dc47
--- /dev/null
+++ b/sandbox/linux/services/broker_process_utils.cc
@@ -0,0 +1,47 @@
+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "broker_process_utils.h"
+
+#include "base/logging.h"
+
+namespace sandbox {
+
+const char* BrokerProcessUtils::Find(

[jln (very slow on Chromium), 2013/07/08 22:36:51]

If this is not really guaranteed async signal safe. More importantly, it means
that in the broker process, the paranoid part where we always return a string
from the whitelist, and never the one provided (which is a security in depth
measure in case strcmp is broken) is lost).

I would not make it part of this class.

Just put it back inline where it was?

[Jorge Lucangeli Obes, 2013/07/09 03:52:12]

Done, but I don't understand why this would not return a string from the
whitelist, since the only two return statements are "return it->c_str()" and
"return NULL".

+    const char* needle,
+    const std::vector<std::string>& haystack) {
+  std::vector<std::string>::const_iterator it;
+
+  for (it = haystack.begin(); it != haystack.end(); it++) {
+    if (StrCmp(needle, it->c_str()) == 0)
+      return it->c_str();
+  }
+
+  return NULL;
+}
+
+int BrokerProcessUtils::StrCmp(const char* s1, const char* s2) {

[jln (very slow on Chromium), 2013/07/08 22:36:51]

As mentioned, this really needs unit tests.

Please, have at least one where one of the strings is infinite: fill a page with
'A' and have the next page PROT_NONE to trigger a segfault. (Or if you don't
want to spend time doing this, it's also ok to just rely on ASAN checking this
for you.

+  while (*s1 != '\0' && *s2 != '\0') {
+    if (*s1 < *s2)
+      return -1;
+    else if (*s1 > *s2)
+      return 1;
+
+    s1++;
+    s2++;
+  }
+
+  if (*s1 == '\0' && *s2 == '\0')

[jln (very slow on Chromium), 2013/07/08 22:36:51]

Style: use brackets everywhere if you use them in the last else {}

[Jorge Lucangeli Obes, 2013/07/09 03:52:12]

Done.

+    return 0;
+  else if (*s1 == '\0')
+    return -1;
+  else if (*s2 == '\0')
+    return 1;
+  else {
+    NOTREACHED();

[jln (very slow on Chromium), 2013/07/08 22:36:51]

This is not async signal safe. Use RAW_CHECK (in !NDEBUG).

[Jorge Lucangeli Obes, 2013/07/09 03:52:12]

Done.

+    return 0;

[jln (very slow on Chromium), 2013/07/08 22:36:51]

GCC is still dumb enough to not see this dead code and require you to put a
return here (pretty sure Clang is ok)? If yes, put a comment maybe ?

[Jorge Lucangeli Obes, 2013/07/09 03:52:12]

Done.

+  }
+}
+
+}  // namespace sandbox