From 6665bc92898ba139114278dbc20c72ef466ec4cd Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 24 Apr 2024 09:20:55 +0200 Subject: [PATCH] Use a temp-file to write new content atomically in storage layer (#2807) This will prevent half-written content on disk in case the write is interrupted. Lock files are *not* used as the assumption is that a lock is held centrally. --- crates/gitbutler-core/src/git/credentials.rs | 3 +- crates/gitbutler-core/src/keys/controller.rs | 3 +- crates/gitbutler-core/src/keys/storage.rs | 28 +++-- .../gitbutler-core/src/projects/controller.rs | 10 +- crates/gitbutler-core/src/projects/storage.rs | 21 ++-- crates/gitbutler-core/src/storage.rs | 108 ++++++++++-------- crates/gitbutler-core/src/users/controller.rs | 4 +- crates/gitbutler-core/src/users/storage.rs | 15 +-- .../gitbutler-core/tests/git/credentials.rs | 4 +- .../tests/suite/gb_repository.rs | 12 +- crates/gitbutler-core/tests/suite/projects.rs | 2 +- .../tests/suite/virtual_branches/init.rs | 8 +- .../tests/suite/virtual_branches/mod.rs | 8 +- crates/gitbutler-testsupport/src/suite.rs | 8 +- 14 files changed, 126 insertions(+), 108 deletions(-) diff --git a/crates/gitbutler-core/src/git/credentials.rs b/crates/gitbutler-core/src/git/credentials.rs index 519f56821..dd0544539 100644 --- a/crates/gitbutler-core/src/git/credentials.rs +++ b/crates/gitbutler-core/src/git/credentials.rs @@ -113,7 +113,8 @@ impl Helper { } } - pub fn from_path>(path: P) -> Self { + pub fn from_path(path: impl Into) -> Self { + let path = path.into(); let keys = keys::Controller::from_path(&path); let users = users::Controller::from_path(path); let home_dir = std::env::var_os("HOME").map(PathBuf::from); diff --git a/crates/gitbutler-core/src/keys/controller.rs b/crates/gitbutler-core/src/keys/controller.rs index eb8954411..f2816fbb5 100644 --- a/crates/gitbutler-core/src/keys/controller.rs +++ b/crates/gitbutler-core/src/keys/controller.rs @@ -1,4 +1,5 @@ use anyhow::Context; +use std::path::PathBuf; use super::{storage::Storage, PrivateKey}; @@ -12,7 +13,7 @@ impl Controller { Self { storage } } - pub fn from_path>(path: P) -> Self { + pub fn from_path(path: impl Into) -> Self { Self::new(Storage::from_path(path)) } diff --git a/crates/gitbutler-core/src/keys/storage.rs b/crates/gitbutler-core/src/keys/storage.rs index 1aeb504d9..fc44a0832 100644 --- a/crates/gitbutler-core/src/keys/storage.rs +++ b/crates/gitbutler-core/src/keys/storage.rs @@ -1,42 +1,40 @@ use super::PrivateKey; use crate::storage; +use std::path::PathBuf; +// TODO(ST): get rid of this type, it's more trouble than it's worth. #[derive(Clone)] pub struct Storage { - storage: storage::Storage, + inner: storage::Storage, } #[derive(Debug, thiserror::Error)] pub enum Error { - #[error("IO error: {0}")] - Storage(#[from] storage::Error), + #[error(transparent)] + Storage(#[from] std::io::Error), #[error("SSH key error: {0}")] SSHKey(#[from] ssh_key::Error), } impl Storage { pub fn new(storage: storage::Storage) -> Storage { - Storage { storage } + Storage { inner: storage } } - pub fn from_path>(path: P) -> Storage { + pub fn from_path(path: impl Into) -> Storage { Storage::new(storage::Storage::new(path)) } pub fn get(&self) -> Result, Error> { - self.storage - .read("keys/ed25519") - .map_err(Error::Storage) - .and_then(|s| s.map(|s| s.parse().map_err(Error::SSHKey)).transpose()) + let key = self.inner.read("keys/ed25519")?; + key.map(|s| s.parse().map_err(Into::into)).transpose() } + // TODO(ST): see if Key should rather deal with bytes instead for this kind of serialization. pub fn create(&self, key: &PrivateKey) -> Result<(), Error> { - self.storage - .write("keys/ed25519", &key.to_string()) - .map_err(Error::Storage)?; - self.storage - .write("keys/ed25519.pub", &key.public_key().to_string()) - .map_err(Error::Storage)?; + self.inner.write("keys/ed25519", &key.to_string())?; + self.inner + .write("keys/ed25519.pub", &key.public_key().to_string())?; Ok(()) } } diff --git a/crates/gitbutler-core/src/projects/controller.rs b/crates/gitbutler-core/src/projects/controller.rs index ecfcf3df8..6aef3ef16 100644 --- a/crates/gitbutler-core/src/projects/controller.rs +++ b/crates/gitbutler-core/src/projects/controller.rs @@ -46,12 +46,12 @@ impl Controller { } } - pub fn from_path>(path: P) -> Self { - let pathbuf = path.as_ref().to_path_buf(); + pub fn from_path(path: impl Into) -> Self { + let path = path.into(); Self { - local_data_dir: pathbuf.clone(), - projects_storage: storage::Storage::from_path(&pathbuf), - users: users::Controller::from_path(&pathbuf), + projects_storage: storage::Storage::from_path(&path), + users: users::Controller::from_path(&path), + local_data_dir: path, watchers: None, } } diff --git a/crates/gitbutler-core/src/projects/storage.rs b/crates/gitbutler-core/src/projects/storage.rs index 50a132cd8..5820ad687 100644 --- a/crates/gitbutler-core/src/projects/storage.rs +++ b/crates/gitbutler-core/src/projects/storage.rs @@ -1,4 +1,5 @@ use serde::{Deserialize, Serialize}; +use std::path::PathBuf; use crate::{ projects::{project, ProjectId}, @@ -9,7 +10,7 @@ const PROJECTS_FILE: &str = "projects.json"; #[derive(Debug, Clone)] pub struct Storage { - storage: storage::Storage, + inner: storage::Storage, } #[derive(Debug, Serialize, Deserialize, Default, Clone)] @@ -30,7 +31,7 @@ pub struct UpdateRequest { #[derive(Debug, thiserror::Error)] pub enum Error { #[error(transparent)] - Storage(#[from] storage::Error), + Storage(#[from] std::io::Error), #[error(transparent)] Json(#[from] serde_json::Error), #[error("project not found")] @@ -38,16 +39,16 @@ pub enum Error { } impl Storage { - pub fn new(storage: storage::Storage) -> Storage { - Storage { storage } + pub fn new(storage: storage::Storage) -> Self { + Self { inner: storage } } - pub fn from_path>(path: P) -> Storage { - Storage::new(storage::Storage::new(path)) + pub fn from_path(path: impl Into) -> Self { + Self::new(storage::Storage::new(path)) } pub fn list(&self) -> Result, Error> { - match self.storage.read(PROJECTS_FILE)? { + match self.inner.read(PROJECTS_FILE)? { Some(projects) => { let all_projects: Vec = serde_json::from_str(&projects)?; let all_projects: Vec = all_projects @@ -128,7 +129,7 @@ impl Storage { project.omit_certificate_check = Some(omit_certificate_check); } - self.storage + self.inner .write(PROJECTS_FILE, &serde_json::to_string_pretty(&projects)?)?; Ok(projects @@ -142,7 +143,7 @@ impl Storage { let mut projects = self.list()?; if let Some(index) = projects.iter().position(|p| p.id == *id) { projects.remove(index); - self.storage + self.inner .write(PROJECTS_FILE, &serde_json::to_string_pretty(&projects)?)?; } Ok(()) @@ -152,7 +153,7 @@ impl Storage { let mut projects = self.list()?; projects.push(project.clone()); let projects = serde_json::to_string_pretty(&projects)?; - self.storage.write(PROJECTS_FILE, &projects)?; + self.inner.write(PROJECTS_FILE, &projects)?; Ok(()) } } diff --git a/crates/gitbutler-core/src/storage.rs b/crates/gitbutler-core/src/storage.rs index 1b474eda7..19274e62d 100644 --- a/crates/gitbutler-core/src/storage.rs +++ b/crates/gitbutler-core/src/storage.rs @@ -1,71 +1,85 @@ -#[cfg(target_family = "unix")] -use std::os::unix::prelude::*; +use gix::tempfile::create_dir::Retries; +use gix::tempfile::{AutoRemove, ContainingDirectory}; +use std::io::Write; use std::{ fs, path::{Path, PathBuf}, - sync::{Arc, RwLock}, }; -#[derive(Debug, Default, Clone)] +/// A facility to read, write and delete files. +#[derive(Debug, Clone)] pub struct Storage { - local_data_dir: Arc>, -} - -#[derive(Debug, thiserror::Error)] -pub enum Error { - #[error(transparent)] - IO(#[from] std::io::Error), + /// The directory into which all of or files will be written or read-from. + local_data_dir: PathBuf, } impl Storage { - pub fn new>(local_data_dir: P) -> Storage { + pub fn new(local_data_dir: impl Into) -> Storage { Storage { - local_data_dir: Arc::new(RwLock::new(local_data_dir.as_ref().to_path_buf())), + local_data_dir: local_data_dir.into(), } } - pub fn read>(&self, path: P) -> Result, Error> { - let local_data_dir = self.local_data_dir.read().unwrap(); - let file_path = local_data_dir.join(path); - if !file_path.exists() { - return Ok(None); + /// Read the content of the file at `rela_path` which is a path relative to our root directory. + /// Return `Ok(None)` if the file doesn't exist. + // TODO(ST): make all these operations write bytes. + pub fn read(&self, rela_path: impl AsRef) -> std::io::Result> { + match fs::read_to_string(self.local_data_dir.join(rela_path)) { + Ok(content) => Ok(Some(content)), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(None), + Err(err) => Err(err), } - let contents = fs::read_to_string(&file_path).map_err(Error::IO)?; - Ok(Some(contents)) } - pub fn write>(&self, path: P, content: &str) -> Result<(), Error> { - let local_data_dir = self.local_data_dir.write().unwrap(); - let file_path = local_data_dir.join(path); + /// Write `content` to `rela_path` atomically, so it's either written completely, or not at all. + /// Creates the file and intermediate directories. + /// + /// ### On Synchronization + /// + /// Mutating operations are assumed to be synchronized by the caller, + /// even though all writes will be atomic. + /// + /// If these operations are not synchronized, they will be racy as it's undefined + /// which *whole* write will win. Thus, operations which touch multiple files and + /// need them to be consistent *need* to synchronize by some mean. + /// + /// Generally, the filesystem is used for synchronization, not in-memory primitives. + pub fn write(&self, rela_path: impl AsRef, content: &str) -> std::io::Result<()> { + let file_path = self.local_data_dir.join(rela_path); let dir = file_path.parent().unwrap(); - if !dir.exists() { - fs::create_dir_all(dir).map_err(Error::IO)?; + // NOTE: This creates a 0o600 files on Unix by default. + let mut tempfile = gix::tempfile::new( + dir, + ContainingDirectory::CreateAllRaceProof(Retries::default()), + AutoRemove::Tempfile, + )?; + tempfile.write_all(content.as_bytes())?; + match tempfile.persist(file_path) { + Ok(Some(_opened_file)) => Ok(()), + Ok(None) => unreachable!("BUG: a signal has caused the tempfile to be removed, but we didn't install a handler"), + Err(err) => Err(err.error) } - fs::write(&file_path, content).map_err(Error::IO)?; - - // Set the permissions to be user-only. We can't actually - // do this on Windows, so we ignore that platform. - #[cfg(target_family = "unix")] - { - let metadata = fs::metadata(file_path.clone())?; - let mut permissions = metadata.permissions(); - permissions.set_mode(0o600); // User read/write - fs::set_permissions(file_path.clone(), permissions)?; - } - - Ok(()) } - pub fn delete>(&self, path: P) -> Result<(), Error> { - let local_data_dir = self.local_data_dir.write().unwrap(); - let file_path = local_data_dir.join(path); - if !file_path.exists() { - return Ok(()); - } - if file_path.is_dir() { - fs::remove_dir_all(file_path.clone()).map_err(Error::IO)?; + /// Delete the file or directory at `rela_path`. + /// + /// ### Panics + /// + /// If a symlink is encountered. + pub fn delete(&self, rela_path: impl AsRef) -> std::io::Result<()> { + let file_path = self.local_data_dir.join(rela_path); + let md = match file_path.symlink_metadata() { + Ok(md) => md, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(err) => return Err(err), + }; + + if md.is_dir() { + fs::remove_dir_all(file_path)?; + } else if md.is_file() { + fs::remove_file(file_path)?; } else { - fs::remove_file(file_path.clone()).map_err(Error::IO)?; + unreachable!("BUG: we do not create or work with symlinks") } Ok(()) } diff --git a/crates/gitbutler-core/src/users/controller.rs b/crates/gitbutler-core/src/users/controller.rs index 499cffb0a..d1fa8eada 100644 --- a/crates/gitbutler-core/src/users/controller.rs +++ b/crates/gitbutler-core/src/users/controller.rs @@ -1,7 +1,9 @@ use anyhow::Context; +use std::path::PathBuf; use super::{storage::Storage, User}; +/// TODO(ST): useless intermediary - remove #[derive(Clone)] pub struct Controller { storage: Storage, @@ -12,7 +14,7 @@ impl Controller { Controller { storage } } - pub fn from_path>(path: P) -> Controller { + pub fn from_path(path: impl Into) -> Controller { Controller::new(Storage::from_path(path)) } diff --git a/crates/gitbutler-core/src/users/storage.rs b/crates/gitbutler-core/src/users/storage.rs index 8c77323c3..8e6b51feb 100644 --- a/crates/gitbutler-core/src/users/storage.rs +++ b/crates/gitbutler-core/src/users/storage.rs @@ -1,4 +1,5 @@ use anyhow::Result; +use std::path::PathBuf; use crate::{storage, users::user}; @@ -6,28 +7,28 @@ const USER_FILE: &str = "user.json"; #[derive(Debug, Clone)] pub struct Storage { - storage: storage::Storage, + inner: storage::Storage, } #[derive(Debug, thiserror::Error)] pub enum Error { #[error(transparent)] - Storage(#[from] storage::Error), + Storage(#[from] std::io::Error), #[error(transparent)] Json(#[from] serde_json::Error), } impl Storage { pub fn new(storage: storage::Storage) -> Storage { - Storage { storage } + Storage { inner: storage } } - pub fn from_path>(path: P) -> Storage { + pub fn from_path(path: impl Into) -> Storage { Storage::new(storage::Storage::new(path)) } pub fn get(&self) -> Result, Error> { - match self.storage.read(USER_FILE)? { + match self.inner.read(USER_FILE)? { Some(data) => Ok(Some(serde_json::from_str(&data)?)), None => Ok(None), } @@ -35,12 +36,12 @@ impl Storage { pub fn set(&self, user: &user::User) -> Result<(), Error> { let data = serde_json::to_string(user)?; - self.storage.write(USER_FILE, &data)?; + self.inner.write(USER_FILE, &data)?; Ok(()) } pub fn delete(&self) -> Result<(), Error> { - self.storage.delete(USER_FILE)?; + self.inner.delete(USER_FILE)?; Ok(()) } } diff --git a/crates/gitbutler-core/tests/git/credentials.rs b/crates/gitbutler-core/tests/git/credentials.rs index 786a7cc36..5fe116099 100644 --- a/crates/gitbutler-core/tests/git/credentials.rs +++ b/crates/gitbutler-core/tests/git/credentials.rs @@ -19,14 +19,14 @@ impl TestCase<'_> { fn run(&self) -> Vec<(String, Vec)> { let local_app_data = temp_dir(); - let users = users::Controller::from_path(&local_app_data); + let users = users::Controller::from_path(local_app_data.path()); let user = users::User { github_access_token: self.github_access_token.map(ToString::to_string), ..Default::default() }; users.set_user(&user).unwrap(); - let keys = keys::Controller::from_path(&local_app_data); + let keys = keys::Controller::from_path(local_app_data.path()); let helper = Helper::new(keys, users, self.home_dir.clone()); let (repo, _tmp) = test_repository(); diff --git a/crates/gitbutler-core/tests/suite/gb_repository.rs b/crates/gitbutler-core/tests/suite/gb_repository.rs index 0079954a6..e56804113 100644 --- a/crates/gitbutler-core/tests/suite/gb_repository.rs +++ b/crates/gitbutler-core/tests/suite/gb_repository.rs @@ -12,7 +12,7 @@ mod init { let test_project = TestProject::default(); let data_dir = paths::data_dir(); - let projects = projects::Controller::from_path(&data_dir); + let projects = projects::Controller::from_path(data_dir.path()); let project = projects .add(test_project.path()) @@ -32,7 +32,7 @@ mod init { let test_project = TestProject::default(); let data_dir = paths::data_dir(); - let projects = projects::Controller::from_path(&data_dir); + let projects = projects::Controller::from_path(data_dir.path()); let project = projects .add(test_project.path()) @@ -54,7 +54,7 @@ mod init { let test_project = TestProject::default(); let data_dir = paths::data_dir(); - let projects = projects::Controller::from_path(&data_dir); + let projects = projects::Controller::from_path(data_dir.path()); let project = projects .add(test_project.path()) @@ -84,7 +84,7 @@ mod flush { let test_project = TestProject::default(); let data_dir = paths::data_dir(); - let projects = projects::Controller::from_path(&data_dir); + let projects = projects::Controller::from_path(data_dir.path()); let project = projects .add(test_project.path()) @@ -107,7 +107,7 @@ mod flush { let test_project = TestProject::default(); let data_dir = paths::data_dir(); - let projects = projects::Controller::from_path(&data_dir); + let projects = projects::Controller::from_path(data_dir.path()); let project = projects .add(test_project.path()) @@ -131,7 +131,7 @@ mod flush { let test_project = TestProject::default(); let data_dir = paths::data_dir(); - let projects = projects::Controller::from_path(&data_dir); + let projects = projects::Controller::from_path(data_dir.path()); let project = projects .add(test_project.path()) diff --git a/crates/gitbutler-core/tests/suite/projects.rs b/crates/gitbutler-core/tests/suite/projects.rs index 337edecca..ba4f0c2b2 100644 --- a/crates/gitbutler-core/tests/suite/projects.rs +++ b/crates/gitbutler-core/tests/suite/projects.rs @@ -5,7 +5,7 @@ use gitbutler_testsupport::{self, paths}; pub fn new() -> (Controller, TempDir) { let data_dir = paths::data_dir(); - let controller = Controller::from_path(&data_dir); + let controller = Controller::from_path(data_dir.path()); (controller, data_dir) } diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/init.rs b/crates/gitbutler-core/tests/suite/virtual_branches/init.rs index b5770f90a..ba39156fc 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/init.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/init.rs @@ -3,10 +3,10 @@ use super::*; #[tokio::test] async fn twice() { let data_dir = paths::data_dir(); - let keys = keys::Controller::from_path(&data_dir); - let projects = projects::Controller::from_path(&data_dir); - let users = users::Controller::from_path(&data_dir); - let helper = git::credentials::Helper::from_path(&data_dir); + let keys = keys::Controller::from_path(data_dir.path()); + let projects = projects::Controller::from_path(data_dir.path()); + let users = users::Controller::from_path(data_dir.path()); + let helper = git::credentials::Helper::from_path(data_dir.path()); let test_project = TestProject::default(); diff --git a/crates/gitbutler-core/tests/suite/virtual_branches/mod.rs b/crates/gitbutler-core/tests/suite/virtual_branches/mod.rs index a53c2bc36..464e8c9de 100644 --- a/crates/gitbutler-core/tests/suite/virtual_branches/mod.rs +++ b/crates/gitbutler-core/tests/suite/virtual_branches/mod.rs @@ -29,10 +29,10 @@ impl Drop for Test { impl Default for Test { fn default() -> Self { let data_dir = paths::data_dir(); - let keys = keys::Controller::from_path(&data_dir); - let projects = projects::Controller::from_path(&data_dir); - let users = users::Controller::from_path(&data_dir); - let helper = git::credentials::Helper::from_path(&data_dir); + let keys = keys::Controller::from_path(data_dir.path()); + let projects = projects::Controller::from_path(data_dir.path()); + let users = users::Controller::from_path(data_dir.path()); + let helper = git::credentials::Helper::from_path(data_dir.path()); let test_project = TestProject::default(); let project = projects diff --git a/crates/gitbutler-testsupport/src/suite.rs b/crates/gitbutler-testsupport/src/suite.rs index cd64b44f9..4667c6786 100644 --- a/crates/gitbutler-testsupport/src/suite.rs +++ b/crates/gitbutler-testsupport/src/suite.rs @@ -27,10 +27,10 @@ impl Drop for Suite { impl Default for Suite { fn default() -> Self { let local_app_data = temp_dir(); - let storage = gitbutler_core::storage::Storage::new(&local_app_data); - let users = gitbutler_core::users::Controller::from_path(&local_app_data); - let projects = gitbutler_core::projects::Controller::from_path(&local_app_data); - let keys = gitbutler_core::keys::Controller::from_path(&local_app_data); + let storage = gitbutler_core::storage::Storage::new(local_app_data.path()); + let users = gitbutler_core::users::Controller::from_path(local_app_data.path()); + let projects = gitbutler_core::projects::Controller::from_path(local_app_data.path()); + let keys = gitbutler_core::keys::Controller::from_path(local_app_data.path()); Self { storage, local_app_data: Some(local_app_data),