Add better error checking in ConvertFileSystemURLToPathInsideCrostini

Change method API to take resulting FilePath as input pointer, and
return bool to indicate success or otherwise since it is not
guaranteed that input will always convert.

Bug: 878324
Change-Id: I17f99201c2c4b5105cceb48d6a3c0d57b80c5f7b
Reviewed-on: https://chromium-review.googlesource.com/c/1295634
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601903}

chrome/browser/chromeos/extensions/file_manager/private_api_misc.cc

@@ -759,13 +759,16 @@ FileManagerPrivateInternalGetLinuxPackageInfoFunction::Run() {
       file_manager::util::GetFileSystemContextForRenderFrameHost(
           profile, render_frame_host());
 
-  std::string url =
-      file_manager::util::ConvertFileSystemURLToPathInsideCrostini(
-          profile, file_system_context->CrackURL(GURL(params->url)));
+  base::FilePath path;
+  if (!file_manager::util::ConvertFileSystemURLToPathInsideCrostini(
+          profile, file_system_context->CrackURL(GURL(params->url)), &path)) {
+    return RespondNow(Error("Invalid url: " + params->url));
+  }
+
   crostini::CrostiniPackageInstallerService::GetForProfile(profile)
       ->GetLinuxPackageInfo(
           crostini::kCrostiniDefaultVmName,
-          crostini::kCrostiniDefaultContainerName, url,
+          crostini::kCrostiniDefaultContainerName, path.value(),
           base::BindOnce(
               &FileManagerPrivateInternalGetLinuxPackageInfoFunction::
                   OnGetLinuxPackageInfo,
@@ -804,13 +807,16 @@ FileManagerPrivateInternalInstallLinuxPackageFunction::Run() {
       file_manager::util::GetFileSystemContextForRenderFrameHost(
           profile, render_frame_host());
 
-  std::string url =
-      file_manager::util::ConvertFileSystemURLToPathInsideCrostini(
-          profile, file_system_context->CrackURL(GURL(params->url)));
+  base::FilePath path;
+  if (!file_manager::util::ConvertFileSystemURLToPathInsideCrostini(
+          profile, file_system_context->CrackURL(GURL(params->url)), &path)) {
+    return RespondNow(Error("Invalid url: " + params->url));
+  }
+
   crostini::CrostiniPackageInstallerService::GetForProfile(profile)
       ->InstallLinuxPackage(
           crostini::kCrostiniDefaultVmName,
-          crostini::kCrostiniDefaultContainerName, url,
+          crostini::kCrostiniDefaultContainerName, path.value(),
           base::BindOnce(
               &FileManagerPrivateInternalInstallLinuxPackageFunction::
                   OnInstallLinuxPackage,

chrome/browser/chromeos/file_manager/crostini_file_tasks.cc

@@ -153,8 +153,13 @@ void ExecuteCrostiniTask(
 
   std::vector<std::string> files;
   for (const storage::FileSystemURL& file_system_url : file_system_urls) {
-    files.emplace_back(util::ConvertFileSystemURLToPathInsideCrostini(
-        profile, file_system_url));
+    base::FilePath file;
+    if (!util::ConvertFileSystemURLToPathInsideCrostini(
+            profile, file_system_url, &file)) {
+      LOG(ERROR) << "Invalid file: " << file_system_url.DebugString();
+      return;
+    }
+    files.emplace_back(file.value());
   }
 
   crostini::LaunchCrostiniApp(profile, task.app_id, display::kInvalidDisplayId,

chrome/browser/chromeos/file_manager/path_util.cc

@@ -181,37 +181,31 @@ std::vector<std::string> GetCrostiniMountOptions(
   return options;
 }
 
-std::string ConvertFileSystemURLToPathInsideCrostini(
+bool ConvertFileSystemURLToPathInsideCrostini(
     Profile* profile,
-    const storage::FileSystemURL& file_system_url) {
-  std::string id(file_system_url.mount_filesystem_id());
+    const storage::FileSystemURL& file_system_url,
+    base::FilePath* inside) {
+  const std::string& id(file_system_url.mount_filesystem_id());
   std::string mount_point_name_crostini = GetCrostiniMountPointName(profile);
   std::string mount_point_name_downloads = GetDownloadsMountPointName(profile);
-  DCHECK(file_system_url.mount_type() == storage::kFileSystemTypeExternal);
-  DCHECK(file_system_url.type() == storage::kFileSystemTypeNativeLocal);
-  DCHECK(id == mount_point_name_crostini || id == mount_point_name_downloads);
 
   // Reformat virtual_path() from:
   //   <mount_label>/path/to/file
   // To either:
   //   /<home-directory>/path/to/file     (path is already in crostini volume)
   //   /ChromeOS/<volume_id>/path/to/file (path is shared with crostini)
-  base::FilePath result;
   base::FilePath folder;
   if (id == mount_point_name_crostini) {
     folder = base::FilePath(mount_point_name_crostini);
-    result = crostini::ContainerHomeDirectoryForProfile(profile);
+    *inside = crostini::ContainerHomeDirectoryForProfile(profile);
   } else if (id == mount_point_name_downloads) {
     folder = base::FilePath(mount_point_name_downloads);
-    result =
+    *inside =
         crostini::ContainerChromeOSBaseDirectory().Append(kDownloadsFolderName);
   } else {
-    NOTREACHED();
+    return false;
   }
-  bool success =
-      folder.AppendRelativePath(file_system_url.virtual_path(), &result);
-  DCHECK(success);
-  return result.AsUTF8Unsafe();
+  return folder.AppendRelativePath(file_system_url.virtual_path(), inside);
 }
 
 bool ConvertPathToArcUrl(const base::FilePath& path, GURL* arc_url_out) {

chrome/browser/chromeos/file_manager/path_util.h

@@ -63,9 +63,10 @@ std::vector<std::string> GetCrostiniMountOptions(
     const std::string& container_public_key);
 
 // Convert a cracked url to a path inside the Crostini VM.
-std::string ConvertFileSystemURLToPathInsideCrostini(
+bool ConvertFileSystemURLToPathInsideCrostini(
     Profile* profile,
-    const storage::FileSystemURL& file_system_url);
+    const storage::FileSystemURL& file_system_url,
+    base::FilePath* inside);
 
 // DEPRECATED. Use |ConvertToContentUrls| instead.
 // While this function can convert paths under Downloads, /media/removable

chrome/browser/chromeos/file_manager/path_util_unittest.cc

@@ -175,16 +175,27 @@ TEST(FileManagerPathUtilTest, ConvertFileSystemURLToPathInsideCrostini) {
       GetDownloadsMountPointName(&profile), storage::kFileSystemTypeNativeLocal,
       storage::FileSystemMountOption(), GetDownloadsFolderForProfile(&profile));
 
-  EXPECT_EQ("/home/testing_profile/path/in/crostini",
-            ConvertFileSystemURLToPathInsideCrostini(
-                &profile, mount_points->CreateExternalFileSystemURL(
-                              GURL(), "crostini_test_termina_penguin",
-                              base::FilePath("path/in/crostini"))));
-  EXPECT_EQ("/ChromeOS/Downloads/path/in/downloads",
-            ConvertFileSystemURLToPathInsideCrostini(
-                &profile,
-                mount_points->CreateExternalFileSystemURL(
-                    GURL(), "Downloads", base::FilePath("path/in/downloads"))));
+  base::FilePath inside;
+  EXPECT_TRUE(ConvertFileSystemURLToPathInsideCrostini(
+      &profile,
+      mount_points->CreateExternalFileSystemURL(
+          GURL(), "crostini_test_termina_penguin",
+          base::FilePath("path/in/crostini")),
+      &inside));
+  EXPECT_EQ("/home/testing_profile/path/in/crostini", inside.value());
+
+  EXPECT_TRUE(ConvertFileSystemURLToPathInsideCrostini(
+      &profile,
+      mount_points->CreateExternalFileSystemURL(
+          GURL(), "Downloads", base::FilePath("path/in/downloads")),
+      &inside));
+  EXPECT_EQ("/ChromeOS/Downloads/path/in/downloads", inside.value());
+
+  EXPECT_FALSE(ConvertFileSystemURLToPathInsideCrostini(
+      &profile,
+      mount_points->CreateExternalFileSystemURL(
+          GURL(), "unknown", base::FilePath("path/in/unknown")),
+      &inside));
 }
 
 std::unique_ptr<KeyedService> CreateFileSystemOperationRunnerForTesting(