Allow replacing a read-only node with writable for same target

PiperOrigin-RevId: 548942347
Change-Id: I4b22740ca27772831afcddb69d515c84aca04c51

sandboxed_api/sandbox2/mounts.cc

@@ -145,24 +145,54 @@ bool IsSameFile(const std::string& path1, const std::string& path2) {
   return stat1.st_dev == stat2.st_dev && stat1.st_ino == stat2.st_ino;
 }
 
-bool IsEquivalentNode(const MountTree::Node& n1, const MountTree::Node& n2) {
+bool IsWritable(const MountTree::Node& node) {
+  switch (node.node_case()) {
+    case MountTree::Node::kFileNode:
+      return node.file_node().writable();
+    case MountTree::Node::kDirNode:
+      return node.dir_node().writable();
+    case MountTree::Node::kRootNode:
+      return node.root_node().writable();
+    default:
+      return false;
+  }
+}
+
+bool HasSameTarget(const MountTree::Node& n1, const MountTree::Node& n2) {
   // Return early when node types are different
   if (n1.node_case() != n2.node_case()) {
     return false;
   }
+  // Compare proto fileds
+  switch (n1.node_case()) {
+    case MountTree::Node::kFileNode:
+      // Check whether files are the same (e.g. symlinks / hardlinks)
+      return IsSameFile(n1.file_node().outside(), n2.file_node().outside());
+    case MountTree::Node::kDirNode:
+      // Check whether dirs are the same (e.g. symlinks / hardlinks)
+      return IsSameFile(n1.dir_node().outside(), n2.dir_node().outside());
+    case MountTree::Node::kTmpfsNode:
+      return n1.tmpfs_node().tmpfs_options() == n2.tmpfs_node().tmpfs_options();
+    case MountTree::Node::kRootNode:
+      return true;
+    default:
+      return false;
+  }
+}
+
+bool IsEquivalentNode(const MountTree::Node& n1, const MountTree::Node& n2) {
+  if (!HasSameTarget(n1, n2)) {
+    return false;
+  }
 
   // Compare proto fileds
   switch (n1.node_case()) {
     case MountTree::Node::kFileNode:
-      // Check whether files are the same (e.g. symlinks / hardlinks)
+      return n1.file_node().writable() == n2.file_node().writable();
-      return n1.file_node().writable() == n2.file_node().writable() &&
-             IsSameFile(n1.file_node().outside(), n2.file_node().outside());
     case MountTree::Node::kDirNode:
-      // Check whether dirs are the same (e.g. symlinks / hardlinks)
+      return n1.dir_node().writable() == n2.dir_node().writable();
-      return n1.dir_node().writable() == n2.dir_node().writable() &&
-             IsSameFile(n1.dir_node().outside(), n2.dir_node().outside());
     case MountTree::Node::kTmpfsNode:
-      return n1.tmpfs_node().tmpfs_options() == n2.tmpfs_node().tmpfs_options();
+      return true;
     case MountTree::Node::kRootNode:
       return n1.root_node().writable() == n2.root_node().writable();
     default:
@@ -275,6 +305,24 @@ absl::Status Mounts::Insert(absl::string_view path,
                    std::string(path).c_str());
       return absl::OkStatus();
     }
+    if (internal::HasSameTarget(curtree->node(), new_node)) {
+      if (!internal::IsWritable(curtree->node()) &&
+          internal::IsWritable(new_node)) {
+        SAPI_RAW_LOG(INFO,
+                     "Chaning %s to writable, was insterted read-only before",
+                     std::string(path).c_str());
+        *curtree->mutable_node() = new_node;
+        return absl::OkStatus();
+      }
+      if (internal::IsWritable(curtree->node()) &&
+          !internal::IsWritable(new_node)) {
+        SAPI_RAW_LOG(INFO,
+                     "Inserting %s read-only is a nop, as it was insterted "
+                     "writable before",
+                     std::string(path).c_str());
+        return absl::OkStatus();
+      }
+    }
     return absl::FailedPreconditionError(absl::StrCat(
         "Inserting ", path, " twice with conflicting values ",
         curtree->node().DebugString(), " vs. ", new_node.DebugString()));
@@ -289,10 +337,6 @@ absl::Status Mounts::Insert(absl::string_view path,
   return absl::OkStatus();
 }
 
-absl::Status Mounts::AddFile(absl::string_view path, bool is_ro) {
-  return AddFileAt(path, path, is_ro);
-}
-
 absl::Status Mounts::AddFileAt(absl::string_view outside,
                                absl::string_view inside, bool is_ro) {
   MountTree::Node node;

sandboxed_api/sandbox2/mounts.h

@@ -29,6 +29,8 @@ namespace sandbox2 {
 namespace internal {
 
 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
 
@@ -47,11 +49,17 @@ class Mounts {
   Mounts& operator=(const Mounts&) = default;
   Mounts& operator=(Mounts&&) = default;
 
-  absl::Status AddFile(absl::string_view path, bool is_ro = true);
+  absl::Status AddFile(absl::string_view path, bool is_ro = true) {
+    return AddFileAt(path, path, is_ro);
+  }
 
   absl::Status AddFileAt(absl::string_view outside, absl::string_view inside,
                          bool is_ro = true);
 
+  absl::Status AddDirectory(absl::string_view path, bool is_ro = true) {
+    return AddDirectoryAt(path, path, is_ro);
+  }
+
   absl::Status AddDirectoryAt(absl::string_view outside,
                               absl::string_view inside, bool is_ro = true);
 

sandboxed_api/sandbox2/mounts_test.cc

@@ -128,6 +128,18 @@ TEST(MountTreeTest, TestMultipleInsertionFileSymlink) {
   EXPECT_THAT(mounts.AddFileAt(symlink_path, "/a"), IsOk());
 }
 
+TEST(MountTreeTest, TestMultipleInsertionUpgradeToWritable) {
+  Mounts mounts;
+  EXPECT_THAT(mounts.AddFile("/a"), IsOk());
+  EXPECT_THAT(mounts.AddFile("/a", /*is_ro=*/false), IsOk());
+  EXPECT_THAT(mounts.AddDirectory("/b"), IsOk());
+  EXPECT_THAT(mounts.AddDirectory("/b", /*is_ro=*/false), IsOk());
+  EXPECT_THAT(mounts.AddFile("/c", /*is_ro=*/false), IsOk());
+  EXPECT_THAT(mounts.AddFile("/c"), IsOk());
+  EXPECT_THAT(mounts.AddDirectory("/d", /*is_ro=*/false), IsOk());
+  EXPECT_THAT(mounts.AddDirectory("/d"), IsOk());
+}
+
 TEST(MountTreeTest, TestMultipleInsertionDirSymlink) {
   Mounts mounts;
 
@@ -265,6 +277,96 @@ TEST(MountTreeTest, TestList) {
   // clang-format on
 }
 
+TEST(MountTreeTest, TestIsWritable) {
+  MountTree::Node nodes[7];
+  MountTree::FileNode* fn0 = nodes[0].mutable_file_node();
+  fn0->set_writable(false);
+  fn0->set_outside("foo");
+  MountTree::FileNode* fn1 = nodes[1].mutable_file_node();
+  fn1->set_writable(true);
+  fn1->set_outside("bar");
+  MountTree::DirNode* dn0 = nodes[2].mutable_dir_node();
+  dn0->set_writable(false);
+  dn0->set_outside("foo");
+  MountTree::DirNode* dn1 = nodes[3].mutable_dir_node();
+  dn1->set_writable(true);
+  dn1->set_outside("bar");
+  MountTree::RootNode* rn0 = nodes[4].mutable_root_node();
+  rn0->set_writable(false);
+  MountTree::RootNode* rn1 = nodes[5].mutable_root_node();
+  rn1->set_writable(true);
+  MountTree::TmpfsNode* tn0 = nodes[6].mutable_tmpfs_node();
+  tn0->set_tmpfs_options("option1");
+
+  EXPECT_FALSE(internal::IsWritable(nodes[0]));
+  EXPECT_TRUE(internal::IsWritable(nodes[1]));
+  EXPECT_FALSE(internal::IsWritable(nodes[2]));
+  EXPECT_TRUE(internal::IsWritable(nodes[3]));
+  EXPECT_FALSE(internal::IsWritable(nodes[4]));
+  EXPECT_TRUE(internal::IsWritable(nodes[5]));
+  EXPECT_FALSE(internal::IsWritable(nodes[6]));
+}
+
+TEST(MountTreeTest, TestHasSameTarget) {
+  MountTree::Node nodes[10];
+  MountTree::FileNode* fn0 = nodes[0].mutable_file_node();
+  fn0->set_writable(false);
+  fn0->set_outside("foo");
+  MountTree::FileNode* fn1 = nodes[1].mutable_file_node();
+  fn1->set_writable(true);
+  fn1->set_outside("foo");
+  MountTree::FileNode* fn2 = nodes[2].mutable_file_node();
+  fn2->set_writable(false);
+  fn2->set_outside("bar");
+  MountTree::DirNode* dn0 = nodes[3].mutable_dir_node();
+  dn0->set_writable(false);
+  dn0->set_outside("foo");
+  MountTree::DirNode* dn1 = nodes[4].mutable_dir_node();
+  dn1->set_writable(true);
+  dn1->set_outside("foo");
+  MountTree::DirNode* dn2 = nodes[5].mutable_dir_node();
+  dn2->set_writable(false);
+  dn2->set_outside("bar");
+  MountTree::TmpfsNode* tn0 = nodes[6].mutable_tmpfs_node();
+  tn0->set_tmpfs_options("option1");
+  MountTree::TmpfsNode* tn1 = nodes[7].mutable_tmpfs_node();
+  tn1->set_tmpfs_options("option2");
+  MountTree::RootNode* rn0 = nodes[8].mutable_root_node();
+  rn0->set_writable(false);
+  MountTree::RootNode* rn1 = nodes[9].mutable_root_node();
+  rn1->set_writable(true);
+
+  // Compare same file nodes
+  EXPECT_TRUE(internal::HasSameTarget(nodes[0], nodes[0]));
+  // Compare almost same file nodes (ro vs rw)
+  EXPECT_TRUE(internal::HasSameTarget(nodes[0], nodes[1]));
+  // Compare different file nodes
+  EXPECT_FALSE(internal::HasSameTarget(nodes[0], nodes[2]));
+  // Compare file node with dir node
+  EXPECT_FALSE(internal::HasSameTarget(nodes[0], nodes[3]));
+
+  // Compare same dir nodes
+  EXPECT_TRUE(internal::HasSameTarget(nodes[3], nodes[3]));
+  // Compare almost same dir nodes (ro vs rw)
+  EXPECT_TRUE(internal::HasSameTarget(nodes[3], nodes[4]));
+  // Compare different dir nodes
+  EXPECT_FALSE(internal::HasSameTarget(nodes[3], nodes[5]));
+  // Compare dir node with tmpfs node
+  EXPECT_FALSE(internal::HasSameTarget(nodes[3], nodes[6]));
+
+  // Compare same tmpfs nodes
+  EXPECT_TRUE(internal::HasSameTarget(nodes[6], nodes[6]));
+  // Compare different tmpfs nodes
+  EXPECT_FALSE(internal::HasSameTarget(nodes[6], nodes[7]));
+  // Compare dir node with root node
+  EXPECT_FALSE(internal::HasSameTarget(nodes[6], nodes[8]));
+
+  // Compare same root nodes
+  EXPECT_TRUE(internal::HasSameTarget(nodes[8], nodes[8]));
+  // Compare almost same root nodes (ro vs rw)
+  EXPECT_TRUE(internal::HasSameTarget(nodes[8], nodes[9]));
+}
+
 TEST(MountTreeTest, TestNodeEquivalence) {
   MountTree::Node nodes[8];
   MountTree::FileNode* fn0 = nodes[0].mutable_file_node();