Don't allow '\0' characters in FilePath.

base/file_path.cc

 // Copyright (c) 2012 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 "base/file_path.h"
 
 #include <string.h>
 #include <algorithm>
 
 #include "base/basictypes.h"
@@ skipping 22 matching lines @@
 const FilePath::CharType FilePath::kSeparators[] = FILE_PATH_LITERAL("\\/");
 #else  // FILE_PATH_USES_WIN_SEPARATORS
 const FilePath::CharType FilePath::kSeparators[] = FILE_PATH_LITERAL("/");
 #endif  // FILE_PATH_USES_WIN_SEPARATORS
 
 const FilePath::CharType FilePath::kCurrentDirectory[] = FILE_PATH_LITERAL(".");
 const FilePath::CharType FilePath::kParentDirectory[] = FILE_PATH_LITERAL("..");
 
 const FilePath::CharType FilePath::kExtensionSeparator = FILE_PATH_LITERAL('.');
 
+const FilePath::CharType kStringTerminator = FILE_PATH_LITERAL('\0');
+

[Tom Sepez, 2012/12/20 21:14:34]

Should be in namespace { below.

 typedef FilePath::StringType StringType;
 
 namespace {
 
 const char* kCommonDoubleExtensionSuffixes[] = { "gz", "z", "bz2" };
 const char* kCommonDoubleExtensions[] = { "user.js" };
 
 // If this FilePath contains a drive letter specification, returns the
 // position of the last character of the drive letter specification,
 // otherwise returns npos.  This can only be true on Windows, when a pathname
@@ skipping 122 matching lines @@
 
 }  // namespace
 
 FilePath::FilePath() {
 }
 
 FilePath::FilePath(const FilePath& that) : path_(that.path_) {
 }
 
 FilePath::FilePath(const StringType& path) : path_(path) {
+  // Don't allow '\0' characters in path.

[Tom Sepez, 2012/12/20 21:14:34]

nit: not sure we need this comment. Maybe just add a note to file_path.h that
paths can't contain NULs as a precaution against OS usage.

+  StringType::size_type zero_pos = path_.find(kStringTerminator);
+  if (zero_pos != StringType::npos)
+    path_.erase(zero_pos);

[Tom Sepez, 2012/12/20 21:14:34]

nit: maybe write
   path_.erase(zero_pos, StringType::npos)
to make it clearer?

 }
 
 FilePath::~FilePath() {
 }
 
 FilePath& FilePath::operator=(const FilePath& that) {
   path_ = that.path_;
   return *this;
 }
 
@@ skipping 252 matching lines @@
 
   StringType current_extension = Extension();
 
   if (current_extension.length() != extension.length())
     return false;
 
   return FilePath::CompareEqualIgnoreCase(extension, current_extension);
 }
 
 FilePath FilePath::Append(const StringType& component) const {
-  DCHECK(!IsPathAbsolute(component));
+  StringType appended(component);

[Tom Sepez, 2012/12/20 21:14:34]

Would be nice to avoid the copy if the NULs not found.

+
+  // Don't allow '\0' characters to be appended.
+  StringType::size_type zero_pos = appended.find(kStringTerminator);
+  if (zero_pos != StringType::npos)
+    appended.erase(zero_pos);
+
+  DCHECK(!IsPathAbsolute(appended));
+
   if (path_.compare(kCurrentDirectory) == 0) {
     // Append normally doesn't do any normalization, but as a special case,
     // when appending to kCurrentDirectory, just return a new path for the
     // component argument.  Appending component to kCurrentDirectory would
     // serve no purpose other than needlessly lengthening the path, and
     // it's likely in practice to wind up with FilePath objects containing
     // only kCurrentDirectory when calling DirName on a single relative path
     // component.
-    return FilePath(component);
+    return FilePath(appended);
   }
 
   FilePath new_path(path_);
   new_path.StripTrailingSeparatorsInternal();
 
   // Don't append a separator if the path is empty (indicating the current
   // directory) or if the path component is empty (indicating nothing to
   // append).
-  if (component.length() > 0 && new_path.path_.length() > 0) {
+  if (appended.length() > 0 && new_path.path_.length() > 0) {
     // Don't append a separator if the path still ends with a trailing
     // separator after stripping (indicating the root directory).
     if (!IsSeparator(new_path.path_[new_path.path_.length() - 1])) {
       // Don't append a separator if the path is just a drive letter.
       if (FindDriveLetter(new_path.path_) + 1 != new_path.path_.length()) {
         new_path.path_.append(1, kSeparators[0]);
       }
     }
   }
 
-  new_path.path_.append(component);
+  new_path.path_.append(appended);
   return new_path;
 }
 
 FilePath FilePath::Append(const FilePath& component) const {
   return Append(component.value());
 }
 
 FilePath FilePath::AppendASCII(const base::StringPiece& component) const {
   DCHECK(IsStringASCII(component));
 #if defined(OS_WIN)
@@ skipping 96 matching lines @@
 #if defined(OS_WIN)
   pickle->WriteString16(path_);
 #else
   pickle->WriteString(path_);
 #endif
 }
 
 bool FilePath::ReadFromPickle(PickleIterator* iter) {
 #if defined(OS_WIN)
   if (!iter->ReadString16(&path_))
     return false;

[Tom Sepez, 2012/12/20 21:14:34]

Need the check here, otherwise IPCs bypass your constructors.

[Tom Sepez, 2012/12/20 21:19:39]

Or Not.  Looks like IPC reads the underlying string type and invokes your
modified constructor.  

This would be called to serialize other things then, check may not hurt.

 #else
   if (!iter->ReadString(&path_))
     return false;
 #endif
 
   return true;
 }
 
 #if defined(OS_WIN)
 // Windows specific implementation of file string comparisons
@@ skipping 615 matching lines @@
   }
   return FilePath(copy);
 #else
   return *this;
 #endif
 }
 
 void PrintTo(const FilePath& path, std::ostream* out) {
   *out << path.value();
 }