Assure VirtualBranches based metadata can't be used in a racy fashion. (#9808)

Currently it's easy to create an instance the is used for writing before
obtaining a workspace lock, which leads to race-condition if the metadata
is changed after all.

The new implementation makes this impossible by enforcing a permission to be provided.

Author: Byron

crates/but-api/src/commands/diff.rs

@@ -100,7 +100,8 @@ fn changes_in_branch_inner(
     remote: Option<String>,
     branch_name: String,
 ) -> anyhow::Result<TreeChanges> {
-    let (repo, _meta, graph) = ctx.graph_and_meta(ctx.gix_repo()?)?;
+    let guard = ctx.project().shared_worktree_access();
+    let (repo, _meta, graph) = ctx.graph_and_meta(ctx.gix_repo()?, guard.read_permission())?;
     let name = if let Some(remote) = remote {
         Category::RemoteBranch.to_full_name(format!("{remote}/{branch_name}").as_str())
     } else {

crates/but-api/src/commands/stack.rs

@@ -88,13 +88,14 @@ pub fn create_reference(app: &App, params: create_reference::Params) -> Result<(
         })
         .transpose()?;
 
-    let (repo, mut meta, graph) = ctx.graph_and_meta_and_repo()?;
+    let mut guard = ctx.project().exclusive_worktree_access();
+    let (repo, mut meta, graph) = ctx.graph_and_meta_mut_and_repo(guard.write_permission())?;
     _ = but_workspace::branch::create_reference(
         new_ref,
         anchor,
         &repo,
         &graph.to_workspace()?,
-        &mut meta,
+        &mut *meta,
     )?;
     Ok(())
 }
@@ -104,7 +105,8 @@ pub fn create_branch(app: &App, params: CreateBranchParams) -> Result<(), Error>
     let ctx = CommandContext::open(&project, app.app_settings.get()?.clone())?;
     if app.app_settings.get()?.feature_flags.ws3 {
         use ReferencePosition::Above;
-        let (repo, mut meta, graph) = ctx.graph_and_meta_and_repo()?;
+        let mut guard = project.exclusive_worktree_access();
+        let (repo, mut meta, graph) = ctx.graph_and_meta_mut_and_repo(guard.write_permission())?;
         let ws = graph.to_workspace()?;
         let stack = ws.try_find_stack_by_id(params.stack_id)?;
         let new_ref = Category::LocalBranch
@@ -117,7 +119,6 @@ pub fn create_branch(app: &App, params: CreateBranchParams) -> Result<(), Error>
             .into());
         }
 
-        let mut guard = project.exclusive_worktree_access();
         ctx.snapshot_create_dependent_branch(&params.request.name, guard.write_permission())
             .ok();
         _ = but_workspace::branch::create_reference(
@@ -146,7 +147,7 @@ pub fn create_branch(app: &App, params: CreateBranchParams) -> Result<(), Error>
             },
             &repo,
             &ws,
-            &mut meta,
+            &mut *meta,
         )?;
     } else {
         // NOTE: locking is built-in here.
@@ -166,20 +167,20 @@ pub struct RemoveBranchParams {
 pub fn remove_branch(app: &App, params: RemoveBranchParams) -> Result<(), Error> {
     let project = gitbutler_project::get(params.project_id)?;
     let ctx = CommandContext::open(&project, app.app_settings.get()?.clone())?;
+    let mut guard = project.exclusive_worktree_access();
     if app.app_settings.get()?.feature_flags.ws3 {
-        let (repo, mut meta, graph) = ctx.graph_and_meta_and_repo()?;
+        let (repo, mut meta, graph) = ctx.graph_and_meta_mut_and_repo(guard.write_permission())?;
         let ws = graph.to_workspace()?;
         let ref_name = Category::LocalBranch
             .to_full_name(params.branch_name.as_str())
             .map_err(anyhow::Error::from)?;
-        let mut guard = project.exclusive_worktree_access();
         ctx.snapshot_remove_dependent_branch(&params.branch_name, guard.write_permission())
             .ok();
         but_workspace::branch::remove_reference(
             ref_name.as_ref(),
             &repo,
             &ws,
-            &mut meta,
+            &mut *meta,
             but_workspace::branch::remove_reference::Options {
                 avoid_anonymous_stacks: true,
                 // The UI kind of keeps it, but we can't do that somehow

crates/but-api/src/commands/virtual_branches.rs

@@ -43,7 +43,8 @@ pub fn create_virtual_branch(
     let ws3_enabled = app.app_settings.get()?.feature_flags.ws3;
     let ctx = CommandContext::open(&project, app.app_settings.get()?.clone())?;
     let stack_entry = if ws3_enabled {
-        let (repo, mut meta, graph) = ctx.graph_and_meta_and_repo()?;
+        let mut guard = project.exclusive_worktree_access();
+        let (repo, mut meta, graph) = ctx.graph_and_meta_mut_and_repo(guard.write_permission())?;
         let ws = graph.to_workspace()?;
         let new_ref = Category::LocalBranch
             .to_full_name(
@@ -63,9 +64,13 @@ pub fn create_virtual_branch(
             )
             .map_err(anyhow::Error::from)?;
 
-        let _guard = project.exclusive_worktree_access();
-        let graph =
-            but_workspace::branch::create_reference(new_ref.as_ref(), None, &repo, &ws, &mut meta)?;
+        let graph = but_workspace::branch::create_reference(
+            new_ref.as_ref(),
+            None,
+            &repo,
+            &ws,
+            &mut *meta,
+        )?;
 
         let ws = graph.to_workspace()?;
         let (stack_idx, segment_idx) = ws

crates/gitbutler-command-context/src/lib.rs

@@ -1,7 +1,8 @@
 use anyhow::Result;
-use but_graph::VirtualBranchesTomlMetadata;
 use but_settings::AppSettings;
+use gitbutler_project::access::{WorktreeReadPermission, WorktreeWritePermission};
 use gitbutler_project::Project;
+use std::ops::{Deref, DerefMut};
 use std::path::Path;
 
 pub struct CommandContext {
@@ -14,6 +15,42 @@ pub struct CommandContext {
     db_handle: Option<but_db::DbHandle>,
 }
 
+/// A [`but_graph::VirtualBranchesTomlMetadata`] instance that is only accessible if it sees a read
+/// permission upon instantiation.
+///
+/// This is necessary only while the `virtual_branches.toml` file is used as it's read eagerly on
+/// instantiation, and must thus not interleave with other writers.
+pub struct VirtualBranchesTomlMetadata(but_graph::VirtualBranchesTomlMetadata);
+
+impl Deref for VirtualBranchesTomlMetadata {
+    type Target = but_graph::VirtualBranchesTomlMetadata;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+/// A [`but_graph::VirtualBranchesTomlMetadata`] instance that is only accessible if it sees a write
+/// permission upon instantiation.
+///
+/// This is necessary only while the `virtual_branches.toml` file is used as it's read eagerly on
+/// instantiation, and must thus not interleave with other writers.
+pub struct VirtualBranchesTomlMetadataMut(but_graph::VirtualBranchesTomlMetadata);
+
+impl Deref for VirtualBranchesTomlMetadataMut {
+    type Target = but_graph::VirtualBranchesTomlMetadata;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl DerefMut for VirtualBranchesTomlMetadataMut {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.0
+    }
+}
+
 impl CommandContext {
     /// Open the repository identified by `project` and perform some checks.
     pub fn open(project: &Project, app_settings: AppSettings) -> Result<Self> {
@@ -29,10 +66,6 @@ impl CommandContext {
     pub fn project(&self) -> &Project {
         &self.project
     }
-    //
-    // pub fn meta(&self) -> Result<VirtualBranchesTomlMetadata> {
-    //     VirtualBranchesTomlMetadata::from_path(project.gb_dir().join("virtual_branches.toml"))
-    // }
 
     /// Return the [`project`](Self::project) repository.
     pub fn repo(&self) -> &git2::Repository {
@@ -65,38 +98,42 @@ impl CommandContext {
 
     /// Create a new Graph traversal from the current HEAD, using (and returning) the given `repo` (configured by the caller),
     /// along with a new metadata instance, and the graph itself.
+    ///
+    /// The read-permission is required to obtain a shared metadata instance. Note that it must be held
+    /// for until the end of the operation for the protection to be effective.
     pub fn graph_and_meta(
         &self,
         repo: gix::Repository,
+        _read_only: &WorktreeReadPermission,
     ) -> Result<(
         gix::Repository,
         VirtualBranchesTomlMetadata,
         but_graph::Graph,
     )> {
         let meta = self.meta()?;
         let graph = but_graph::Graph::from_head(&repo, &meta, meta.graph_options())?;
-        Ok((repo, meta, graph))
+        Ok((repo, VirtualBranchesTomlMetadata(meta), graph))
     }
 
     /// Open the repository with standard options and create a new Graph traversal from the current HEAD,
     /// along with a new metadata instance, and the graph itself.
     ///
+    /// The write-permission is required to obtain a mutable metadata instance. Note that it must be held
+    /// for until the end of the operation for the protection to be effective.
+    ///
     /// Use [`Self::graph_and_meta()`] if control over the repository configuration is needed.
-    pub fn graph_and_meta_and_repo(
+    pub fn graph_and_meta_mut_and_repo(
         &self,
+        _write: &mut WorktreeWritePermission,
     ) -> Result<(
         gix::Repository,
-        VirtualBranchesTomlMetadata,
+        VirtualBranchesTomlMetadataMut,
         but_graph::Graph,
     )> {
         let repo = self.gix_repo()?;
-        self.graph_and_meta(repo)
-    }
-
-    /// Return the `RefMetadata` implementation based on the `virtual_branches.toml` file.
-    /// This can one day be changed to auto-migrate away from the toml and to the database.
-    pub fn meta(&self) -> Result<VirtualBranchesTomlMetadata> {
-        VirtualBranchesTomlMetadata::from_path(self.project.gb_dir().join("virtual_branches.toml"))
+        let meta = self.meta()?;
+        let graph = but_graph::Graph::from_head(&repo, &meta, meta.graph_options())?;
+        Ok((repo, VirtualBranchesTomlMetadataMut(meta), graph))
     }
 
     /// Return a newly opened `gitoxide` repository, with all configuration available
@@ -133,6 +170,17 @@ impl CommandContext {
     }
 }
 
+/// Keep these private
+impl CommandContext {
+    /// Return the `RefMetadata` implementation based on the `virtual_branches.toml` file.
+    /// This can one day be changed to auto-migrate away from the toml and to the database.
+    fn meta(&self) -> Result<but_graph::VirtualBranchesTomlMetadata> {
+        but_graph::VirtualBranchesTomlMetadata::from_path(
+            self.project.gb_dir().join("virtual_branches.toml"),
+        )
+    }
+}
+
 /// Return a newly opened `gitoxide` repository, with all configuration available
 /// to correctly figure out author and committer names (i.e. with most global configuration loaded),
 /// *and* which will perform diffs quickly thanks to an adequate object cache.