Sandbox: Small cleanup for the path handling code.

No functionality change.

BUG=473878

Review URL: https://codereview.chromium.org/1243723005

Cr-Commit-Position: refs/heads/master@{#340605}

7 files changed, 51 insertions, 66 deletions

diff --git a/sandbox/win/src/file_policy_test.cc b/sandbox/win/src/file_policy_test.cc
index 1990e2f..f7509bd 100644
--- a/sandbox/win/src/file_policy_test.cc
+++ b/sandbox/win/src/file_policy_test.cc
@@ -346,8 +346,8 @@ TEST(FilePolicyTest, AllowImplicitDeviceName) {
   ASSERT_NE(::GetTempPath(MAX_PATH, temp_directory), 0u);
   ASSERT_NE(::GetTempFileName(temp_directory, L"test", 0, temp_file_name), 0u);
 
-  std::wstring path;
-  EXPECT_TRUE(ConvertToLongPath(temp_file_name, &path));
+  std::wstring path(temp_file_name);
+  EXPECT_TRUE(ConvertToLongPath(&path));
   EXPECT_TRUE(GetNtPathFromWin32Path(path, &path));
   path = path.substr(sandbox::kNTDevicePrefixLen);
 
diff --git a/sandbox/win/src/filesystem_dispatcher.cc b/sandbox/win/src/filesystem_dispatcher.cc
index b6da10d..d4ef796 100644
--- a/sandbox/win/src/filesystem_dispatcher.cc
+++ b/sandbox/win/src/filesystem_dispatcher.cc
@@ -92,7 +92,7 @@ bool FilesystemDispatcher::NtCreateFile(IPCInfo* ipc,
                                         uint32 share_access,
                                         uint32 create_disposition,
                                         uint32 create_options) {
-  if (!PreProcessName(*name, name)) {
+  if (!PreProcessName(name)) {
     // The path requested might contain a reparse point.
     ipc->return_info.nt_status = STATUS_ACCESS_DENIED;
     return true;
@@ -138,7 +138,7 @@ bool FilesystemDispatcher::NtOpenFile(IPCInfo* ipc,
                                       uint32 desired_access,
                                       uint32 share_access,
                                       uint32 open_options) {
-  if (!PreProcessName(*name, name)) {
+  if (!PreProcessName(name)) {
     // The path requested might contain a reparse point.
     ipc->return_info.nt_status = STATUS_ACCESS_DENIED;
     return true;
@@ -184,7 +184,7 @@ bool FilesystemDispatcher::NtQueryAttributesFile(IPCInfo* ipc,
   if (sizeof(FILE_BASIC_INFORMATION) != info->Size())
     return false;
 
-  if (!PreProcessName(*name, name)) {
+  if (!PreProcessName(name)) {
     // The path requested might contain a reparse point.
     ipc->return_info.nt_status = STATUS_ACCESS_DENIED;
     return true;
@@ -224,7 +224,7 @@ bool FilesystemDispatcher::NtQueryFullAttributesFile(IPCInfo* ipc,
   if (sizeof(FILE_NETWORK_OPEN_INFORMATION) != info->Size())
     return false;
 
-  if (!PreProcessName(*name, name)) {
+  if (!PreProcessName(name)) {
     // The path requested might contain a reparse point.
     ipc->return_info.nt_status = STATUS_ACCESS_DENIED;
     return true;
@@ -279,7 +279,7 @@ bool FilesystemDispatcher::NtSetInformationFile(IPCInfo* ipc,
   base::string16 name;
   name.assign(rename_info->FileName, rename_info->FileNameLength /
                                      sizeof(rename_info->FileName[0]));
-  if (!PreProcessName(name, &name)) {
+  if (!PreProcessName(&name)) {
     // The path requested might contain a reparse point.
     ipc->return_info.nt_status = STATUS_ACCESS_DENIED;
     return true;
diff --git a/sandbox/win/src/filesystem_policy.cc b/sandbox/win/src/filesystem_policy.cc
index 2c3f703..eb6b197 100644
--- a/sandbox/win/src/filesystem_policy.cc
+++ b/sandbox/win/src/filesystem_policy.cc
@@ -79,7 +79,7 @@ bool FileSystemPolicy::GenerateRules(const wchar_t* name,
     return false;
   }
 
-  if (!PreProcessName(mod_name, &mod_name)) {
+  if (!PreProcessName(&mod_name)) {
     // The path to be added might contain a reparse point.
     NOTREACHED();
     return false;
@@ -394,15 +394,14 @@ bool FileSystemPolicy::SetInformationFileAction(
   return true;
 }
 
-bool PreProcessName(const base::string16& path, base::string16* new_path) {
-  ConvertToLongPath(path, new_path);
+bool PreProcessName(base::string16* path) {
+  ConvertToLongPath(path);
 
-  bool reparsed = false;
-  if (ERROR_SUCCESS != IsReparsePoint(*new_path, &reparsed))
-    return false;
+  if (ERROR_NOT_A_REPARSE_POINT == IsReparsePoint(*path))
+    return true;
 
-  // We can't process reparsed file.
-  return !reparsed;
+  // We can't process a reparsed file.
+  return false;
 }
 
 base::string16 FixNTPrefixForMatch(const base::string16& name) {
diff --git a/sandbox/win/src/filesystem_policy.h b/sandbox/win/src/filesystem_policy.h
index ce28344..4103ad6 100644
--- a/sandbox/win/src/filesystem_policy.h
+++ b/sandbox/win/src/filesystem_policy.h
@@ -98,10 +98,9 @@ class FileSystemPolicy {
                                        NTSTATUS* nt_status);
 };
 
-// Expands the path and check if it's a reparse point. Returns false if
-// we cannot determine or if there is an unexpected error. In that case
-// the path cannot be trusted.
-bool PreProcessName(const base::string16& path, base::string16* new_path);
+// Expands the path and check if it's a reparse point. Returns false if the path
+// cannot be trusted.
+bool PreProcessName(base::string16* path);
 
 // Corrects global paths to have a correctly escaped NT prefix at the
 // beginning. If the name has no NT prefix (either normal or escaped)
diff --git a/sandbox/win/src/win_utils.cc b/sandbox/win/src/win_utils.cc
index 2ff1b73..e09c680 100644
--- a/sandbox/win/src/win_utils.cc
+++ b/sandbox/win/src/win_utils.cc
@@ -156,12 +156,10 @@ bool ResolveRegistryName(base::string16 name, base::string16* resolved_name) {
 //    \??\c:\some\foo\bar
 //    \Device\HarddiskVolume0\some\foo\bar
 //    \??\HarddiskVolume0\some\foo\bar
-DWORD IsReparsePoint(const base::string16& full_path, bool* result) {
+DWORD IsReparsePoint(const base::string16& full_path) {
   // Check if it's a pipe. We can't query the attributes of a pipe.
-  if (IsPipe(full_path)) {
-    *result = FALSE;
-    return ERROR_SUCCESS;
-  }
+  if (IsPipe(full_path))
+    return ERROR_NOT_A_REPARSE_POINT;
 
   base::string16 path;
   bool nt_path = IsNTPath(full_path, &path);
@@ -199,7 +197,6 @@ DWORD IsReparsePoint(const base::string16& full_path, bool* result) {
       }
     } else if (FILE_ATTRIBUTE_REPARSE_POINT & attributes) {
       // This is a reparse point.
-      *result = true;
       return ERROR_SUCCESS;
     }
 
@@ -207,8 +204,7 @@ DWORD IsReparsePoint(const base::string16& full_path, bool* result) {
     last_pos = path.rfind(L'\\');
   } while (last_pos > 2);  // Skip root dir.
 
-  *result = false;
-  return ERROR_SUCCESS;
+  return ERROR_NOT_A_REPARSE_POINT;
 }
 
 // We get a |full_path| of the forms accepted by IsReparsePoint(), and the name
@@ -279,34 +275,31 @@ bool SameObject(HANDLE handle, const wchar_t* full_path) {
 
 // Paths like \Device\HarddiskVolume0\some\foo\bar are assumed to be already
 // expanded.
-bool ConvertToLongPath(const base::string16& short_path,
-                       base::string16* long_path) {
-  if (IsPipe(short_path)) {
-    // TODO(rvargas): Change the signature to use a single argument.
-    long_path->assign(short_path);
+bool ConvertToLongPath(base::string16* path) {
+  if (IsPipe(*path))
     return true;
-  }
 
-  base::string16 path;
-  if (IsDevicePath(short_path, &path))
+  base::string16 temp_path;
+  if (IsDevicePath(*path, &temp_path))
     return false;
 
-  bool is_nt_path = IsNTPath(path, &path);
+  bool is_nt_path = IsNTPath(temp_path, &temp_path);
   bool added_implied_device = false;
-  if (!StartsWithDriveLetter(path) && is_nt_path) {
-    path = base::string16(kNTDotPrefix) + path;
+  if (!StartsWithDriveLetter(temp_path) && is_nt_path) {
+    temp_path = base::string16(kNTDotPrefix) + temp_path;
     added_implied_device = true;
   }
 
   DWORD size = MAX_PATH;
   scoped_ptr<wchar_t[]> long_path_buf(new wchar_t[size]);
 
-  DWORD return_value = ::GetLongPathName(path.c_str(), long_path_buf.get(),
+  DWORD return_value = ::GetLongPathName(temp_path.c_str(), long_path_buf.get(),
                                          size);
   while (return_value >= size) {
     size *= 2;
     long_path_buf.reset(new wchar_t[size]);
-    return_value = ::GetLongPathName(path.c_str(), long_path_buf.get(), size);
+    return_value = ::GetLongPathName(temp_path.c_str(), long_path_buf.get(),
+                                     size);
   }
 
   DWORD last_error = ::GetLastError();
@@ -314,31 +307,31 @@ bool ConvertToLongPath(const base::string16& short_path,
                             ERROR_PATH_NOT_FOUND == last_error ||
                             ERROR_INVALID_NAME == last_error)) {
     // The file does not exist, but maybe a sub path needs to be expanded.
-    base::string16::size_type last_slash = path.rfind(L'\\');
+    base::string16::size_type last_slash = temp_path.rfind(L'\\');
     if (base::string16::npos == last_slash)
       return false;
 
-    base::string16 begin = path.substr(0, last_slash);
-    base::string16 end = path.substr(last_slash);
-    if (!ConvertToLongPath(begin, &begin))
+    base::string16 begin = temp_path.substr(0, last_slash);
+    base::string16 end = temp_path.substr(last_slash);
+    if (!ConvertToLongPath(&begin))
       return false;
 
     // Ok, it worked. Let's reset the return value.
-    path = begin + end;
+    temp_path = begin + end;
     return_value = 1;
   } else if (0 != return_value) {
-    path = long_path_buf.get();
+    temp_path = long_path_buf.get();
   }
 
   if (return_value != 0) {
     if (added_implied_device)
-      RemoveImpliedDevice(&path);
+      RemoveImpliedDevice(&temp_path);
 
     if (is_nt_path) {
-      *long_path = kNTPrefix;
-      *long_path += path;
+      *path = kNTPrefix;
+      *path += temp_path;
     } else {
-      *long_path = path;
+      *path = temp_path;
     }
 
     return true;
diff --git a/sandbox/win/src/win_utils.h b/sandbox/win/src/win_utils.h
index 3e4565f..bf3ed84 100644
--- a/sandbox/win/src/win_utils.h
+++ b/sandbox/win/src/win_utils.h
@@ -66,16 +66,15 @@ class SingletonBase {
 
 // Convert a short path (C:\path~1 or \\??\\c:\path~1) to the long version of
 // the path. If the path is not a valid filesystem path, the function returns
-// false and the output parameter is not modified.
-bool ConvertToLongPath(const base::string16& short_path,
-                       base::string16* long_path);
+// false and argument is not modified.
+bool ConvertToLongPath(base::string16* path);
 
-// Sets result to true if the path contains a reparse point. The return value
-// is ERROR_SUCCESS when the function succeeds or the appropriate error code
-// when the function fails.
+// Returns ERROR_SUCCESS if the path contains a reparse point,
+// ERROR_NOT_A_REPARSE_POINT if there's no reparse point in this path, or an
+// error code when the function fails.
 // This function is not smart. It looks for each element in the path and
 // returns true if any of them is a reparse point.
-DWORD IsReparsePoint(const base::string16& full_path, bool* result);
+DWORD IsReparsePoint(const base::string16& full_path);
 
 // Returns true if the handle corresponds to the object pointed by this path.
 bool SameObject(HANDLE handle, const wchar_t* full_path);
diff --git a/sandbox/win/src/win_utils_unittest.cc b/sandbox/win/src/win_utils_unittest.cc
index 4cf59b8..15da51b 100644
--- a/sandbox/win/src/win_utils_unittest.cc
+++ b/sandbox/win/src/win_utils_unittest.cc
@@ -22,17 +22,13 @@ TEST(WinUtils, IsReparsePoint) {
   ASSERT_TRUE(::DeleteFile(my_folder));
   ASSERT_TRUE(::CreateDirectory(my_folder, NULL));
 
-  bool result = true;
-  EXPECT_EQ(ERROR_SUCCESS, IsReparsePoint(my_folder, &result));
-  EXPECT_FALSE(result);
+  EXPECT_EQ(ERROR_NOT_A_REPARSE_POINT, IsReparsePoint(my_folder));
 
-  // We have to fix Bug 32224 to pass this test.
   base::string16 not_found = base::string16(my_folder) + L"\\foo\\bar";
-  // EXPECT_EQ(ERROR_PATH_NOT_FOUND, IsReparsePoint(not_found, &result));
+  EXPECT_EQ(ERROR_NOT_A_REPARSE_POINT, IsReparsePoint(not_found));
 
   base::string16 new_file = base::string16(my_folder) + L"\\foo";
-  EXPECT_EQ(ERROR_SUCCESS, IsReparsePoint(new_file, &result));
-  EXPECT_FALSE(result);
+  EXPECT_EQ(ERROR_NOT_A_REPARSE_POINT, IsReparsePoint(new_file));
 
   // Replace the directory with a reparse point to %temp%.
   HANDLE dir = ::CreateFile(my_folder, FILE_ALL_ACCESS,
@@ -43,8 +39,7 @@ TEST(WinUtils, IsReparsePoint) {
   base::string16 temp_dir_nt = base::string16(L"\\??\\") + temp_directory;
   EXPECT_TRUE(SetReparsePoint(dir, temp_dir_nt.c_str()));
 
-  EXPECT_EQ(ERROR_SUCCESS, IsReparsePoint(new_file, &result));
-  EXPECT_TRUE(result);
+  EXPECT_EQ(ERROR_SUCCESS, IsReparsePoint(new_file));
 
   EXPECT_TRUE(DeleteReparsePoint(dir));
   EXPECT_TRUE(::CloseHandle(dir));