Bug 1173320 - patch 5/8 - Cleanup manual string path management, r=smaug

dom/devicestorage/DeviceStorage.h

@@ -101,8 +101,9 @@ public:
 
   // we want to make sure that the names of file can't reach
   // outside of the type of storage the user asked for.
-  bool IsSafePath();
-  bool IsSafePath(const nsAString& aPath);
+  bool IsSafePath() const;
+  bool ValidateAndSplitPath(const nsAString& aPath,
+                            nsTArray<nsString>* aParts = nullptr) const;
 
   void Dump(const char* label);
 
@@ -137,7 +138,6 @@ public:
 private:
   ~DeviceStorageFile() {}
   void Init();
-  void NormalizeFilePath();
   void AppendRelativePath(const nsAString& aPath);
   void AccumDirectoryUsage(nsIFile* aFile,
                            uint64_t* aPicturesSoFar,

dom/devicestorage/nsDeviceStorage.cpp

@@ -30,6 +30,7 @@
 
 #include "nsArrayUtils.h"
 #include "nsAutoPtr.h"
+#include "nsCharSeparatedTokenizer.h"
 #include "nsGlobalWindow.h"
 #include "nsServiceManagerUtils.h"
 #include "nsIFile.h"
@@ -76,10 +77,35 @@ using namespace mozilla::dom;
 using namespace mozilla::dom::devicestorage;
 using namespace mozilla::ipc;
 
-namespace mozilla {
+namespace mozilla
+{
   MOZ_TYPE_SPECIFIC_SCOPED_POINTER_TEMPLATE(ScopedPRFileDesc, PRFileDesc, PR_Close);
 } // namespace mozilla
 
+namespace {
+
+void
+NormalizeFilePath(nsAString& aPath)
+{
+#if defined(XP_WIN)
+  char16_t* cur = aPath.BeginWriting();
+  char16_t* end = aPath.EndWriting();
+  for (; cur < end; ++cur) {
+    if (char16_t('\\') == *cur) {
+      *cur = FILESYSTEM_DOM_PATH_SEPARATOR_CHAR;
+    }
+  }
+#endif
+}
+
+bool
+TokenizerIgnoreNothing(char16_t /* aChar */)
+{
+  return false;
+}
+
+} // anonymous namespace
+
 StaticAutoPtr<DeviceStorageUsedSpaceCache>
   DeviceStorageUsedSpaceCache::sDeviceStorageUsedSpaceCache;
 
@@ -510,7 +536,8 @@ DeviceStorageFile::DeviceStorageFile(const nsAString& aStorageType,
   if (!mPath.EqualsLiteral("")) {
     AppendRelativePath(mPath);
   }
-  NormalizeFilePath();
+
+  NormalizeFilePath(mPath);
 }
 
 DeviceStorageFile::DeviceStorageFile(const nsAString& aStorageType,
@@ -525,7 +552,7 @@ DeviceStorageFile::DeviceStorageFile(const nsAString& aStorageType,
 {
   Init();
   AppendRelativePath(aPath);
-  NormalizeFilePath();
+  NormalizeFilePath(mPath);
 }
 
 DeviceStorageFile::DeviceStorageFile(const nsAString& aStorageType,
@@ -734,7 +761,7 @@ DeviceStorageFile::CreateUnique(const nsAString& aStorageType,
 void
 DeviceStorageFile::SetPath(const nsAString& aPath) {
   mPath.Assign(aPath);
-  NormalizeFilePath();
+  NormalizeFilePath(mPath);
 }
 
 void
@@ -745,13 +772,14 @@ DeviceStorageFile::SetEditable(bool aEditable) {
 // we want to make sure that the names of file can't reach
 // outside of the type of storage the user asked for.
 bool
-DeviceStorageFile::IsSafePath()
+DeviceStorageFile::IsSafePath() const
 {
-  return IsSafePath(mRootDir) && IsSafePath(mPath);
+  return ValidateAndSplitPath(mRootDir) && ValidateAndSplitPath(mPath);
 }
 
 bool
-DeviceStorageFile::IsSafePath(const nsAString& aPath)
+DeviceStorageFile::ValidateAndSplitPath(const nsAString& aPath,
+                                        nsTArray<nsString>* aParts) const
 {
   nsAString::const_iterator start, end;
   aPath.BeginReading(start);
@@ -764,33 +792,43 @@ DeviceStorageFile::IsSafePath(const nsAString& aPath)
       StringBeginsWith(aPath, tildeSlash)) {
     NS_WARNING("Path name starts with tilde!");
     return false;
-   }
-  // split on /.  if any token is "", ., or .., return false.
-  NS_ConvertUTF16toUTF8 cname(aPath);
-  char* buffer = cname.BeginWriting();
-  const char* token;
+  }
 
-  while ((token = nsCRT::strtok(buffer, "/", &buffer))) {
-    if (PL_strcmp(token, "") == 0 ||
-        PL_strcmp(token, ".") == 0 ||
-        PL_strcmp(token, "..") == 0 ) {
+  NS_NAMED_LITERAL_STRING(kCurrentDir, ".");
+  NS_NAMED_LITERAL_STRING(kParentDir, "..");
+
+  // Split path and check each path component.
+  nsCharSeparatedTokenizerTemplate<TokenizerIgnoreNothing>
+    tokenizer(aPath, FILESYSTEM_DOM_PATH_SEPARATOR_CHAR);
+
+  while (tokenizer.hasMoreTokens()) {
+    nsDependentSubstring pathComponent = tokenizer.nextToken();
+    // The path containing empty components, such as "foo//bar", is invalid.
+    // We don't allow paths, such as "../foo", "foo/./bar" and "foo/../bar",
+    // to walk up the directory.
+    if (pathComponent.IsEmpty() ||
+        pathComponent.Equals(kCurrentDir) ||
+        pathComponent.Equals(kParentDir)) {
       return false;
     }
+
+    if (aParts) {
+      aParts->AppendElement(pathComponent);
+    }
   }
   return true;
 }
 
 void
-DeviceStorageFile::NormalizeFilePath() {
-  FileSystemUtils::LocalPathToNormalizedPath(mPath, mPath);
-}
-
-void
-DeviceStorageFile::AppendRelativePath(const nsAString& aPath) {
+DeviceStorageFile::AppendRelativePath(const nsAString& aPath)
+{
   if (!mFile) {
     return;
   }
-  if (!IsSafePath(aPath)) {
+
+  nsTArray<nsString> parts;
+
+  if (!ValidateAndSplitPath(aPath, &parts)) {
     // All of the APIs (in the child) do checks to verify that the path is
     // valid and return PERMISSION_DENIED if a non-safe path is entered.
     // This check is done in the parent and prevents a compromised
@@ -800,9 +838,13 @@ DeviceStorageFile::AppendRelativePath(const nsAString& aPath) {
     NS_WARNING(NS_LossyConvertUTF16toASCII(aPath).get());
     return;
   }
-  nsString localPath;
-  FileSystemUtils::NormalizedPathToLocalPath(aPath, localPath);
-  mFile->AppendRelativePath(localPath);
+
+  for (uint32_t i = 0; i < parts.Length(); ++i) {
+    nsresult rv = mFile->AppendRelativePath(parts[i]);
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      return;
+    }
+  }
 }
 
 nsresult

dom/filesystem/CreateFileTask.cpp

@@ -168,8 +168,8 @@ CreateFileTask::GetRequestParams(const nsString& aSerializedDOMPath,
 
   param.replace() = mReplace;
   if (mBlobData) {
-    BlobChild* actor
-      = ContentChild::GetSingleton()->GetOrCreateActorForBlob(mBlobData);
+    BlobChild* actor =
+      ContentChild::GetSingleton()->GetOrCreateActorForBlob(mBlobData);
     if (actor) {
       param.data() = actor;
     }
@@ -205,7 +205,7 @@ CreateFileTask::SetSuccessRequestResult(const FileSystemResponseValue& aValue,
 nsresult
 CreateFileTask::Work()
 {
-  class AutoClose
+  class MOZ_RAII AutoClose final
   {
   public:
     explicit AutoClose(nsIOutputStream* aStream)
@@ -218,6 +218,7 @@ CreateFileTask::Work()
     {
       mStream->Close();
     }
+
   private:
     nsCOMPtr<nsIOutputStream> mStream;
   };

dom/filesystem/DeviceStorageFileSystem.cpp

@@ -21,9 +21,8 @@
 namespace mozilla {
 namespace dom {
 
-DeviceStorageFileSystem::DeviceStorageFileSystem(
-  const nsAString& aStorageType,
-  const nsAString& aStorageName)
+DeviceStorageFileSystem::DeviceStorageFileSystem(const nsAString& aStorageType,
+                                                 const nsAString& aStorageName)
   : mWindowId(0)
 {
   MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
@@ -51,9 +50,6 @@ DeviceStorageFileSystem::DeviceStorageFileSystem(
     return;
   }
 
-  FileSystemUtils::LocalPathToNormalizedPath(mLocalRootPath,
-    mNormalizedLocalRootPath);
-
   // DeviceStorageTypeChecker is a singleton object and must be initialized on
   // the main thread. We initialize it here so that we can use it on the worker
   // thread.
@@ -105,12 +101,15 @@ DeviceStorageFileSystem::IsSafeFile(nsIFile* aFile) const
              "Should be on parent process!");
   MOZ_ASSERT(aFile);
 
-  // Check if this file belongs to this storage.
-  nsAutoString path;
-  if (NS_FAILED(aFile->GetPath(path))) {
+  nsCOMPtr<nsIFile> rootPath;
+  nsresult rv = NS_NewLocalFile(GetLocalRootPath(), false,
+                                getter_AddRefs(rootPath));
+  if (NS_WARN_IF(NS_FAILED(rv))) {
     return false;
   }
-  if (!LocalPathToRealPath(path, path)) {
+
+  // Check if this file belongs to this storage.
+  if (NS_WARN_IF(!FileSystemUtils::IsDescendantPath(rootPath, aFile))) {
     return false;
   }
 

dom/filesystem/Directory.cpp

@@ -37,6 +37,12 @@ namespace dom {
 
 namespace {
 
+bool
+TokenizerIgnoreNothing(char16_t /* aChar */)
+{
+  return false;
+}
+
 bool
 IsValidRelativeDOMPath(const nsString& aPath, nsTArray<nsString>& aParts)
 {
@@ -55,7 +61,9 @@ IsValidRelativeDOMPath(const nsString& aPath, nsTArray<nsString>& aParts)
   NS_NAMED_LITERAL_STRING(kParentDir, "..");
 
   // Split path and check each path component.
-  nsCharSeparatedTokenizer tokenizer(aPath, FILESYSTEM_DOM_PATH_SEPARATOR_CHAR);
+  nsCharSeparatedTokenizerTemplate<TokenizerIgnoreNothing>
+    tokenizer(aPath, FILESYSTEM_DOM_PATH_SEPARATOR_CHAR);
+
   while (tokenizer.hasMoreTokens()) {
     nsDependentSubstring pathComponent = tokenizer.nextToken();
     // The path containing empty components, such as "foo//bar", is invalid.

dom/filesystem/FileSystemBase.cpp

@@ -99,20 +99,5 @@ FileSystemBase::IsSafeDirectory(Directory* aDir) const
   return false;
 }
 
-bool
-FileSystemBase::LocalPathToRealPath(const nsAString& aLocalPath,
-                                    nsAString& aRealPath) const
-{
-  nsAutoString path;
-  FileSystemUtils::LocalPathToNormalizedPath(aLocalPath, path);
-  if (!FileSystemUtils::IsDescendantPath(mNormalizedLocalRootPath, path)) {
-    aRealPath.Truncate();
-    return false;
-  }
-
-  aRealPath = Substring(path, mNormalizedLocalRootPath.Length());
-  return true;
-}
-
 } // namespace dom
 } // namespace mozilla

dom/filesystem/FileSystemBase.h

@@ -87,18 +87,12 @@ public:
 protected:
   virtual ~FileSystemBase();
 
-  bool
-  LocalPathToRealPath(const nsAString& aLocalPath, nsAString& aRealPath) const;
-
   // The local path of the root (i.e. the OS path, with OS path separators, of
   // the OS directory that acts as the root of this OSFileSystem).
   // Only available in the parent process.
   // In the child process, we don't use it and its value should be empty.
   nsString mLocalRootPath;
 
-  // The same, but with path separators normalized to "/".
-  nsString mNormalizedLocalRootPath;
-
   bool mShutdown;
 
   // The permission name required to access the file system.

dom/filesystem/FileSystemPermissionRequest.cpp

@@ -15,7 +15,8 @@
 namespace mozilla {
 namespace dom {
 
-NS_IMPL_ISUPPORTS(FileSystemPermissionRequest, nsIRunnable, nsIContentPermissionRequest)
+NS_IMPL_ISUPPORTS(FileSystemPermissionRequest, nsIRunnable,
+                  nsIContentPermissionRequest)
 
 // static
 void
@@ -28,8 +29,7 @@ FileSystemPermissionRequest::RequestForTask(FileSystemTaskBase* aTask)
   NS_DispatchToCurrentThread(request);
 }
 
-FileSystemPermissionRequest::FileSystemPermissionRequest(
-  FileSystemTaskBase* aTask)
+FileSystemPermissionRequest::FileSystemPermissionRequest(FileSystemTaskBase* aTask)
   : mTask(aTask)
 {
   MOZ_ASSERT(mTask, "aTask should not be null!");

dom/filesystem/FileSystemTaskBase.cpp

@@ -23,7 +23,7 @@ namespace dom {
 
 namespace {
 
-class FileSystemReleaseRunnable : public nsRunnable
+class FileSystemReleaseRunnable final : public nsRunnable
 {
 public:
   explicit FileSystemReleaseRunnable(RefPtr<FileSystemBase>& aDoomed)

dom/filesystem/FileSystemUtils.cpp

@@ -6,67 +6,10 @@
 
 #include "mozilla/dom/FileSystemUtils.h"
 
-#include "nsXULAppAPI.h"
-
 namespace mozilla {
 namespace dom {
 
-// static
-void
-FileSystemUtils::LocalPathToNormalizedPath(const nsAString& aLocal,
-                                           nsAString& aNorm)
-{
-  nsString result;
-  result = aLocal;
-#if defined(XP_WIN)
-  char16_t* cur = result.BeginWriting();
-  char16_t* end = result.EndWriting();
-  for (; cur < end; ++cur) {
-    if (char16_t('\\') == *cur)
-      *cur = char16_t('/');
-  }
-#endif
-  aNorm = result;
-}
-
-// static
-void
-FileSystemUtils::NormalizedPathToLocalPath(const nsAString& aNorm,
-                                           nsAString& aLocal)
-{
-  nsString result;
-  result = aNorm;
-#if defined(XP_WIN)
-  char16_t* cur = result.BeginWriting();
-  char16_t* end = result.EndWriting();
-  for (; cur < end; ++cur) {
-    if (char16_t('/') == *cur)
-      *cur = char16_t('\\');
-  }
-#endif
-  aLocal = result;
-}
-
-// static
-bool
-FileSystemUtils::IsDescendantPath(const nsAString& aPath,
-                                  const nsAString& aDescendantPath)
-{
-  // The descendant path should begin with its ancestor path.
-  nsAutoString prefix;
-  prefix = aPath + NS_LITERAL_STRING(FILESYSTEM_DOM_PATH_SEPARATOR_LITERAL);
-
-  // Check the sub-directory path to see if it has the parent path as prefix.
-  if (aDescendantPath.Length() < prefix.Length() ||
-      !StringBeginsWith(aDescendantPath, prefix)) {
-    return false;
-  }
-
-  return true;
-}
-
-// static
-bool
+/* static */ bool
 FileSystemUtils::IsDescendantPath(nsIFile* aFile,
                                   nsIFile* aDescendantFile)
 {

dom/filesystem/FileSystemUtils.h

@@ -7,7 +7,7 @@
 #ifndef mozilla_dom_FileSystemUtils_h
 #define mozilla_dom_FileSystemUtils_h
 
-#include "nsString.h"
+class nsIFile;
 
 namespace mozilla {
 namespace dom {
@@ -22,31 +22,11 @@ namespace dom {
 class FileSystemUtils
 {
 public:
-  /*
-   * Convert the path separator to "/".
-   */
-  static void
-  LocalPathToNormalizedPath(const nsAString& aLocal, nsAString& aNorm);
-
-  /*
-   * Convert the normalized path separator "/" to the system dependent path
-   * separator, which is "/" on Mac and Linux, and "\" on Windows.
-   */
-  static void
-  NormalizedPathToLocalPath(const nsAString& aNorm, nsAString& aLocal);
-
   /*
    * Return true if aDescendantPath is a descendant of aPath.
    */
   static bool
   IsDescendantPath(nsIFile* aPath, nsIFile* aDescendantPath);
-
-  /*
-   * Return true if aDescendantPath is a descendant of aPath. Both aPath and
-   * aDescendantPath are absolute DOM path.
-   */
-  static bool
-  IsDescendantPath(const nsAString& aPath, const nsAString& aDescendantPath);
 };
 
 } // namespace dom

dom/filesystem/GetDirectoryListingTask.cpp

@@ -128,15 +128,6 @@ GetDirectoryListingTask::GetRequestParams(const nsString& aSerializedDOMPath,
                                              mFilters);
 }
 
-void
-GetDirectoryListingTask::CreateNormalizedRelativePath(const nsAString& aPath,
-                                                      nsAString& aRelativePath) const
-{
-  uint32_t rootPathLen = mFileSystem->GetLocalRootPath().Length();
-  FileSystemUtils::LocalPathToNormalizedPath(
-    Substring(aPath, rootPathLen, aPath.Length() - rootPathLen), aRelativePath);
-}
-
 FileSystemResponseValue
 GetDirectoryListingTask::GetSuccessRequestResult(ErrorResult& aRv) const
 {
@@ -366,10 +357,11 @@ GetDirectoryListingTask::HandlerCallback()
       MOZ_ASSERT(FileSystemUtils::IsDescendantPath(rootPath, directoryPath));
 #endif
 
-      RefPtr<Directory> directory = Directory::Create(mFileSystem->GetParentObject(),
-                                                      directoryPath,
-                                                      Directory::eNotDOMRootDirectory,
-                                                      mFileSystem);
+      RefPtr<Directory> directory =
+        Directory::Create(mFileSystem->GetParentObject(),
+                          directoryPath,
+                          Directory::eNotDOMRootDirectory,
+                          mFileSystem);
       MOZ_ASSERT(directory);
 
       // Propogate mFilter onto sub-Directory object:

dom/filesystem/GetDirectoryListingTask.h

@@ -70,10 +70,6 @@ private:
   virtual void
   HandlerCallback() override;
 
-  void
-  CreateNormalizedRelativePath(const nsAString& aPath,
-                               nsAString& aRelativePath) const;
-
   RefPtr<Promise> mPromise;
   nsCOMPtr<nsIFile> mTargetPath;
   nsString mFilters;

dom/filesystem/OSFileSystem.cpp

@@ -20,8 +20,6 @@ namespace dom {
 OSFileSystem::OSFileSystem(const nsAString& aRootDir)
 {
   mLocalRootPath = aRootDir;
-  FileSystemUtils::LocalPathToNormalizedPath(mLocalRootPath,
-                                             mNormalizedLocalRootPath);
 
   // Non-mobile devices don't have the concept of separate permissions to
   // access different parts of devices storage like Pictures, or Videos, etc.