Validate MessageHeader before using

mojo/common/test/test_support_impl.cc

Index: mojo/common/test/test_support_impl.cc
diff --git a/mojo/common/test/test_support_impl.cc b/mojo/common/test/test_support_impl.cc
index 54c3376ef7dc0b9ea12bddfa77a49c9fd66a74fa..c88f917fbf0bfcb840258c86700553d29767aba1 100644
--- a/mojo/common/test/test_support_impl.cc
+++ b/mojo/common/test/test_support_impl.cc
@@ -4,10 +4,33 @@
 
 #include "mojo/common/test/test_support_impl.h"
 

[viettrungluu, 2014/04/25 21:55:59]

#include "base/files/file_path.h"

[viettrungluu, 2014/04/25 21:55:59]

#include <stdlib.h>
#include <string.h>

(for calloc and strdup)

[darin (slow to review), 2014/04/29 06:31:45]

Hmm, I generally follow an approach of only including what is needed, rather
than including everything that is used. The former helps avoid including too
much, but I can see how it might be annoying to not include what you use if
someone ends up removing an include from say file_util.h, etc.

[viettrungluu, 2014/04/29 20:55:01]

In this case, file_util.h really should only forward declare FilePath....

(In general, I try to include the header for a type T if either nothing I
use/include requires that T be defined[*] or if I have a use of T that's
independent of uses that imply that T be already be defined.)

[*] Many includes in headers are incidental to the implementation (e.g., so that
you can put a |Foo foo_;| member) and not implied by the interface. I'd argue
that it's bad to rely on this.

+#include "base/file_util.h"
+#include "base/files/file_enumerator.h"
+#include "base/path_service.h"
+#include "base/strings/string_split.h"
 #include "base/test/perf_log.h"
 
 namespace mojo {
 namespace test {
+namespace {
+
+base::FilePath ResolveSourceRootRelativePath(const char* relative_path) {
+  base::FilePath path;
+  if (!PathService::Get(base::DIR_SOURCE_ROOT, &path))
+    return base::FilePath();
+
+  std::vector<std::string> components;
+  base::SplitString(relative_path, '/', &components);
+
+  for (size_t i = 0; i < components.size(); ++i) {
+    if (!components[i].empty())
+      path = path.AppendASCII(components[i]);
+  }
+
+  return path;
+}
+
+}  // namespace
 
 TestSupportImpl::TestSupportImpl() {
 }
@@ -21,5 +44,24 @@ void TestSupportImpl::LogPerfResult(const char* test_name,
   base::LogPerfResult(test_name, value, units);
 }
 
+FILE* TestSupportImpl::OpenSourceRootRelativeFile(const char* relative_path) {
+  return base::OpenFile(ResolveSourceRootRelativePath(relative_path), "rt");

[viettrungluu, 2014/04/25 21:55:59]

Do we really want "rt"?

[darin (slow to review), 2014/04/29 06:31:45]

Oh, hmm... we can leave off the "t". That is just force of habit when dealing
with text files on Windows. I don't think it is relevant in this case though.

[viettrungluu, 2014/04/29 20:55:01]

(Makes me think that we should do "rb", actually!)

+}
+
+char** TestSupportImpl::EnumerateSourceRootRelativeDirectory(
+    const char* relative_path) {
+  std::vector<std::string> names;
+  base::FileEnumerator e(ResolveSourceRootRelativePath(relative_path), false,
+                         base::FileEnumerator::FILES);
+  for (base::FilePath name = e.Next(); !name.empty(); name = e.Next())
+    names.push_back(name.BaseName().AsUTF8Unsafe());
+
+  char** rv = static_cast<char**>(calloc(names.size() + 1, sizeof(char*)));
+  for (size_t i = 0; i < names.size(); ++i)
+    rv[i] = strdup(names[i].c_str());
+  rv[names.size()] = NULL;

[viettrungluu, 2014/04/25 21:55:59]

calloc actually zeros memory.

[darin (slow to review), 2014/04/29 06:31:45]

Yeah. Perhaps I'll draw attention to the extra NULL entry with a comment.

+  return rv;
+}
+
 }  // namespace test
 }  // namespace mojo