Keep stable iteration order for mounts

- Roll-forward with fixes
- Add test

PiperOrigin-RevId: 680927684
Change-Id: I6d7a4a6ce6769216abcb8d678577c3bd50bf4079

sandboxed_api/sandbox2/BUILD

@@ -728,6 +728,7 @@ 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,7 +56,14 @@ message MountTree {
   }
 
   // The entries are mappings from the next path component to the subtree.
-  map<string, MountTree> entries = 1;
+  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;
 
   // The node of the current path. If not set, we'll just create a directory at
   // this position.

sandboxed_api/sandbox2/mounts.cc

@@ -20,6 +20,7 @@
 #include <sys/statvfs.h>
 #include <unistd.h>
 
+#include <algorithm>
 #include <cerrno>
 #include <cstddef>
 #include <cstdint>
@@ -230,7 +231,7 @@ absl::Status Mounts::Remove(absl::string_view path) {
       return absl::NotFoundError(
           absl::StrCat("Path does not exist in mounts: ", path));
     }
-    curtree = &it->second;
+    curtree = it->second.mutable_sub_tree();
   }
   curtree->clear_node();
   curtree->clear_entries();
@@ -280,25 +281,29 @@ 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_;
-  for (absl::string_view part : parts) {
-    curtree = &(curtree->mutable_entries()
-                    ->insert({std::string(part), MountTree()})
-                    .first->second);
+  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;
     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",
@@ -374,7 +379,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;
+    curtree = &it->second.sub_tree();
     tail = parts.second;
   }
   switch (curtree->node().node_case()) {
@@ -687,6 +692,21 @@ 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) {
@@ -748,9 +768,10 @@ void CreateMounts(const MountTree& tree, const std::string& path,
   }
 
   // Traverse the subtrees.
-  for (const auto& kv : tree.entries()) {
-    std::string new_path = sapi::file::JoinPath(path, kv.first);
-    CreateMounts(kv.second, new_path, create_backing_files);
+  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);
   }
 }
 
@@ -781,9 +802,10 @@ void RecursivelyListMountsImpl(const MountTree& tree,
         absl::StrCat("tmpfs: ", node.tmpfs_node().tmpfs_options()));
   }
 
-  for (const auto& subentry : tree.entries()) {
-    RecursivelyListMountsImpl(subentry.second,
-                              absl::StrCat(tree_path, "/", subentry.first),
+  for (const MapEntry* entry : GetSortedEntries(tree)) {
+    auto& [key, value] = *entry;
+    RecursivelyListMountsImpl(value.sub_tree(),
+                              absl::StrCat(tree_path, "/", key),
                               outside_entries, inside_entries);
   }
 }

sandboxed_api/sandbox2/mounts.h

@@ -16,6 +16,7 @@
 #define SANDBOXED_API_SANDBOX2_MOUNTTREE_H_
 
 #include <cstddef>
+#include <cstdint>
 #include <string>
 #include <utility>
 #include <vector>
@@ -33,6 +34,7 @@ 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 {
@@ -97,11 +99,10 @@ 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,6 +25,7 @@
 #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"
@@ -40,6 +41,7 @@ using ::sapi::GetTestSourcePath;
 using ::sapi::GetTestTempPath;
 using ::sapi::IsOk;
 using ::sapi::StatusIs;
+using ::testing::ElementsAre;
 using ::testing::Eq;
 using ::testing::StrEq;
 using ::testing::UnorderedElementsAreArray;
@@ -425,6 +427,24 @@ 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());