Automated rollback of commit 2ba4460.

PiperOrigin-RevId: 681393675
Change-Id: If591c8f4813c4630f75785fb36bc1c33f62bff1d

Author: cblichmann

sandboxed_api/sandbox2/BUILD

@@ -728,7 +728,6 @@ cc_test(
     copts = sapi_platform_copts(),
     data = ["//sandboxed_api/sandbox2/testcases:minimal_dynamic"],
     deps = [
-        ":mount_tree_cc_proto",
         ":mounts",
         "//sandboxed_api:testing",
         "//sandboxed_api/util:file_base",

sandboxed_api/sandbox2/mount_tree.proto

@@ -56,14 +56,7 @@ message MountTree {
   }
 
   // The entries are mappings from the next path component to the subtree.
-  message MountMapEntry {
-    MountTree sub_tree = 1;
-    // Helps to keep a stable and deterministic order of mounts. Protobuf makes
-    // no guarantees about the order of map entries and may even randomize in
-    // certain build configurations.
-    int64 index = 2;
-  }
-  map<string, MountMapEntry> entries = 1;
+  map<string, MountTree> entries = 1;
 
   // The node of the current path. If not set, we'll just create a directory at
   // this position.

sandboxed_api/sandbox2/mounts.cc

@@ -20,7 +20,6 @@
 #include <sys/statvfs.h>
 #include <unistd.h>
 
-#include <algorithm>
 #include <cerrno>
 #include <cstddef>
 #include <cstdint>
@@ -231,7 +230,7 @@ absl::Status Mounts::Remove(absl::string_view path) {
       return absl::NotFoundError(
           absl::StrCat("Path does not exist in mounts: ", path));
     }
-    curtree = it->second.mutable_sub_tree();
+    curtree = &it->second;
   }
   curtree->clear_node();
   curtree->clear_entries();
@@ -281,29 +280,25 @@ absl::Status Mounts::Insert(absl::string_view path,
 
   std::vector<absl::string_view> parts =
       absl::StrSplit(absl::StripPrefix(fixed_path, "/"), '/');
+  std::string final_part(parts.back());
+  parts.pop_back();
 
   MountTree* curtree = &mount_tree_;
-  int i = 0;
-  while (true) {
-    std::string part(parts[i]);
-    auto [it, did_insert] = curtree->mutable_entries()->insert(
-        {std::string(part), MountTree::MountMapEntry()});
-    auto& entry = it->second;
-    if (did_insert) {
-      entry.set_index(++mount_index_);
-    }
-    curtree = entry.mutable_sub_tree();
-    if (i == parts.size() - 1) {  // Final part
-      break;
-    }
-    ++i;
+  for (absl::string_view part : parts) {
+    curtree = &(curtree->mutable_entries()
+                    ->insert({std::string(part), MountTree()})
+                    .first->second);
     if (curtree->has_node() && curtree->node().has_file_node()) {
       return absl::FailedPreconditionError(
           absl::StrCat("Cannot insert ", path,
                        " since a file is mounted as a parent directory"));
     }
   }
 
+  curtree = &(curtree->mutable_entries()
+                  ->insert({final_part, MountTree()})
+                  .first->second);
+
   if (curtree->has_node()) {
     if (internal::IsEquivalentNode(curtree->node(), new_node)) {
       SAPI_RAW_LOG(INFO, "Inserting %s with the same value twice",
@@ -379,7 +374,7 @@ absl::StatusOr<std::string> Mounts::ResolvePath(absl::string_view path) const {
       }
       return absl::NotFoundError("Path could not be resolved in the mounts");
     }
-    curtree = &it->second.sub_tree();
+    curtree = &it->second;
     tail = parts.second;
   }
   switch (curtree->node().node_case()) {
@@ -692,21 +687,6 @@ void MountWithDefaults(const std::string& source, const std::string& target,
   }
 }
 
-using MapEntry = std::pair<const std::string, MountTree::MountMapEntry>;
-
-std::vector<const MapEntry*> GetSortedEntries(const MountTree& tree) {
-  std::vector<const MapEntry*> ordered_entries;
-  ordered_entries.reserve(tree.entries_size());
-  for (const auto& entry : tree.entries()) {
-    ordered_entries.push_back(&entry);
-  }
-  std::sort(ordered_entries.begin(), ordered_entries.end(),
-            [](const MapEntry* a, const MapEntry* b) {
-              return a->second.index() < b->second.index();
-            });
-  return ordered_entries;
-}
-
 // Traverses the MountTree to create all required files and perform the mounts.
 void CreateMounts(const MountTree& tree, const std::string& path,
                   bool create_backing_files) {
@@ -768,10 +748,9 @@ void CreateMounts(const MountTree& tree, const std::string& path,
   }
 
   // Traverse the subtrees.
-  for (const MapEntry* entry : GetSortedEntries(tree)) {
-    auto& [key, value] = *entry;
-    std::string new_path = sapi::file::JoinPath(path, key);
-    CreateMounts(value.sub_tree(), new_path, create_backing_files);
+  for (const auto& kv : tree.entries()) {
+    std::string new_path = sapi::file::JoinPath(path, kv.first);
+    CreateMounts(kv.second, new_path, create_backing_files);
   }
 }
 
@@ -802,10 +781,9 @@ void RecursivelyListMountsImpl(const MountTree& tree,
         absl::StrCat("tmpfs: ", node.tmpfs_node().tmpfs_options()));
   }
 
-  for (const MapEntry* entry : GetSortedEntries(tree)) {
-    auto& [key, value] = *entry;
-    RecursivelyListMountsImpl(value.sub_tree(),
-                              absl::StrCat(tree_path, "/", key),
+  for (const auto& subentry : tree.entries()) {
+    RecursivelyListMountsImpl(subentry.second,
+                              absl::StrCat(tree_path, "/", subentry.first),
                               outside_entries, inside_entries);
   }
 }

sandboxed_api/sandbox2/mounts.h

@@ -16,7 +16,6 @@
 #define SANDBOXED_API_SANDBOX2_MOUNTTREE_H_
 
 #include <cstddef>
-#include <cstdint>
 #include <string>
 #include <utility>
 #include <vector>
@@ -34,7 +33,6 @@ bool IsSameFile(const std::string& path1, const std::string& path2);
 bool IsWritable(const MountTree::Node& node);
 bool HasSameTarget(const MountTree::Node& n1, const MountTree::Node& n2);
 bool IsEquivalentNode(const MountTree::Node& n1, const MountTree::Node& n2);
-
 }  // namespace internal
 
 class Mounts {
@@ -99,10 +97,11 @@ class Mounts {
   absl::StatusOr<std::string> ResolvePath(absl::string_view path) const;
 
  private:
+  friend class MountTreeTest;
+
   absl::Status Insert(absl::string_view path, const MountTree::Node& node);
 
   MountTree mount_tree_;
-  int64_t mount_index_ = 0;  // Used to keep track of the mount insertion order
 };
 
 }  // namespace sandbox2

sandboxed_api/sandbox2/mounts_test.cc

@@ -25,7 +25,6 @@
 #include "absl/status/status.h"
 #include "absl/strings/match.h"
 #include "absl/strings/str_cat.h"
-#include "sandboxed_api/sandbox2/mount_tree.pb.h"
 #include "sandboxed_api/testing.h"
 #include "sandboxed_api/util/path.h"
 #include "sandboxed_api/util/status_matchers.h"
@@ -41,7 +40,6 @@ using ::sapi::GetTestSourcePath;
 using ::sapi::GetTestTempPath;
 using ::sapi::IsOk;
 using ::sapi::StatusIs;
-using ::testing::ElementsAre;
 using ::testing::Eq;
 using ::testing::StrEq;
 using ::testing::UnorderedElementsAreArray;
@@ -427,24 +425,6 @@ TEST(MountTreeTest, TestNodeEquivalence) {
   EXPECT_FALSE(internal::IsEquivalentNode(nodes[6], nodes[0]));
 }
 
-TEST(MountTreeTest, Order) {
-  Mounts mounts;
-  ASSERT_THAT(mounts.AddDirectoryAt("/A", "/a"), IsOk());
-  ASSERT_THAT(mounts.AddDirectoryAt("/B", "/d/b"), IsOk());
-  ASSERT_THAT(mounts.AddDirectoryAt("/C/D/E", "/d/c/e/f/h"), IsOk());
-  ASSERT_THAT(mounts.AddFileAt("/J/G/H", "/d/c/e/f/h/j"), IsOk());
-  ASSERT_THAT(mounts.AddDirectoryAt("/K/L/M", "/d/c/e/f/h/k"), IsOk());
-
-  std::vector<std::string> inside;
-  std::vector<std::string> outside;
-  mounts.RecursivelyListMounts(&inside, &outside);
-
-  EXPECT_THAT(inside,
-              ElementsAre("/A/", "/B/", "/C/D/E/", "/J/G/H", "/K/L/M/"));
-  EXPECT_THAT(outside, ElementsAre("R /a/", "R /d/b/", "R /d/c/e/f/h/",
-                                   "R /d/c/e/f/h/j", "R /d/c/e/f/h/k/"));
-}
-
 TEST(MountsResolvePathTest, Files) {
   Mounts mounts;
   ASSERT_THAT(mounts.AddFileAt("/A", "/a"), IsOk());