From e0e5b4426ad3cb21c7b4d2332b96c876ce0f5da3 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Fabian=20Gr=C3=BCnbichler?= Date: Fri, 11 Sep 2020 14:34:38 +0200 Subject: [PATCH] BackupDir: make constructor fallible MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit since converting from i64 epoch timestamp to DateTime is not always possible. previously, passing invalid backup-time from client to server (or vice-versa) panicked the corresponding tokio task. now we get proper error messages including the invalid timestamp. Signed-off-by: Fabian Grünbichler --- src/api2/admin/datastore.rs | 20 +++++++++--------- src/api2/backup.rs | 2 +- src/api2/reader.rs | 2 +- src/backup/backup_info.rs | 36 +++++++++++++++++++------------- src/bin/proxmox-backup-client.rs | 13 ++++++------ src/client/pull.rs | 2 +- 6 files changed, 41 insertions(+), 34 deletions(-) diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs index 85c84df4..be2796db 100644 --- a/src/api2/admin/datastore.rs +++ b/src/api2/admin/datastore.rs @@ -230,7 +230,7 @@ pub fn list_snapshot_files( let datastore = DataStore::lookup_datastore(&store)?; - let snapshot = BackupDir::new(backup_type, backup_id, backup_time); + let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?; let allowed = (user_privs & (PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_READ)) != 0; if !allowed { check_backup_owner(&datastore, snapshot.group(), &userid)?; } @@ -280,7 +280,7 @@ fn delete_snapshot( let user_info = CachedUserInfo::new()?; let user_privs = user_info.lookup_privs(&userid, &["datastore", &store]); - let snapshot = BackupDir::new(backup_type, backup_id, backup_time); + let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?; let datastore = DataStore::lookup_datastore(&store)?; @@ -490,7 +490,7 @@ pub fn verify( match (backup_type, backup_id, backup_time) { (Some(backup_type), Some(backup_id), Some(backup_time)) => { worker_id = format!("{}_{}_{}_{:08X}", store, backup_type, backup_id, backup_time); - let dir = BackupDir::new(backup_type, backup_id, backup_time); + let dir = BackupDir::new(backup_type, backup_id, backup_time)?; backup_dir = Some(dir); } (Some(backup_type), Some(backup_id), None) => { @@ -897,7 +897,7 @@ fn download_file( let backup_id = tools::required_string_param(¶m, "backup-id")?; let backup_time = tools::required_integer_param(¶m, "backup-time")?; - let backup_dir = BackupDir::new(backup_type, backup_id, backup_time); + let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?; let allowed = (user_privs & PRIV_DATASTORE_READ) != 0; if !allowed { check_backup_owner(&datastore, backup_dir.group(), &userid)?; } @@ -970,7 +970,7 @@ fn download_file_decoded( let backup_id = tools::required_string_param(¶m, "backup-id")?; let backup_time = tools::required_integer_param(¶m, "backup-time")?; - let backup_dir = BackupDir::new(backup_type, backup_id, backup_time); + let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?; let allowed = (user_privs & PRIV_DATASTORE_READ) != 0; if !allowed { check_backup_owner(&datastore, backup_dir.group(), &userid)?; } @@ -1083,7 +1083,7 @@ fn upload_backup_log( let backup_id = tools::required_string_param(¶m, "backup-id")?; let backup_time = tools::required_integer_param(¶m, "backup-time")?; - let backup_dir = BackupDir::new(backup_type, backup_id, backup_time); + let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?; let userid: Userid = rpcenv.get_user().unwrap().parse()?; check_backup_owner(&datastore, backup_dir.group(), &userid)?; @@ -1159,7 +1159,7 @@ fn catalog( let user_info = CachedUserInfo::new()?; let user_privs = user_info.lookup_privs(&userid, &["datastore", &store]); - let backup_dir = BackupDir::new(backup_type, backup_id, backup_time); + let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?; let allowed = (user_privs & PRIV_DATASTORE_READ) != 0; if !allowed { check_backup_owner(&datastore, backup_dir.group(), &userid)?; } @@ -1276,7 +1276,7 @@ fn pxar_file_download( let backup_id = tools::required_string_param(¶m, "backup-id")?; let backup_time = tools::required_integer_param(¶m, "backup-time")?; - let backup_dir = BackupDir::new(backup_type, backup_id, backup_time); + let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?; let allowed = (user_privs & PRIV_DATASTORE_READ) != 0; if !allowed { check_backup_owner(&datastore, backup_dir.group(), &userid)?; } @@ -1417,7 +1417,7 @@ fn get_notes( let user_info = CachedUserInfo::new()?; let user_privs = user_info.lookup_privs(&userid, &["datastore", &store]); - let backup_dir = BackupDir::new(backup_type, backup_id, backup_time); + let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?; let allowed = (user_privs & PRIV_DATASTORE_READ) != 0; if !allowed { check_backup_owner(&datastore, backup_dir.group(), &userid)?; } @@ -1470,7 +1470,7 @@ fn set_notes( let user_info = CachedUserInfo::new()?; let user_privs = user_info.lookup_privs(&userid, &["datastore", &store]); - let backup_dir = BackupDir::new(backup_type, backup_id, backup_time); + let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?; let allowed = (user_privs & PRIV_DATASTORE_READ) != 0; if !allowed { check_backup_owner(&datastore, backup_dir.group(), &userid)?; } diff --git a/src/api2/backup.rs b/src/api2/backup.rs index c00f9be8..9420b146 100644 --- a/src/api2/backup.rs +++ b/src/api2/backup.rs @@ -114,7 +114,7 @@ async move { } let last_backup = BackupInfo::last_backup(&datastore.base_path(), &backup_group, true).unwrap_or(None); - let backup_dir = BackupDir::new_with_group(backup_group.clone(), backup_time); + let backup_dir = BackupDir::new_with_group(backup_group.clone(), backup_time)?; let _last_guard = if let Some(last) = &last_backup { if backup_dir.backup_time() <= last.backup_dir.backup_time() { diff --git a/src/api2/reader.rs b/src/api2/reader.rs index cf82af06..5252d2e9 100644 --- a/src/api2/reader.rs +++ b/src/api2/reader.rs @@ -83,7 +83,7 @@ fn upgrade_to_backup_reader_protocol( let env_type = rpcenv.env_type(); - let backup_dir = BackupDir::new(backup_type, backup_id, backup_time); + let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?; let path = datastore.base_path(); //let files = BackupInfo::list_files(&path, &backup_dir)?; diff --git a/src/backup/backup_info.rs b/src/backup/backup_info.rs index 4dcf897a..023625f5 100644 --- a/src/backup/backup_info.rs +++ b/src/backup/backup_info.rs @@ -2,9 +2,10 @@ use crate::tools; use anyhow::{bail, format_err, Error}; use regex::Regex; +use std::convert::TryFrom; use std::os::unix::io::RawFd; -use chrono::{DateTime, TimeZone, SecondsFormat, Utc}; +use chrono::{DateTime, LocalResult, TimeZone, SecondsFormat, Utc}; use std::path::{PathBuf, Path}; use lazy_static::lazy_static; @@ -106,7 +107,7 @@ impl BackupGroup { if file_type != nix::dir::Type::Directory { return Ok(()); } let dt = backup_time.parse::>()?; - let backup_dir = BackupDir::new(self.backup_type.clone(), self.backup_id.clone(), dt.timestamp()); + let backup_dir = BackupDir::new(self.backup_type.clone(), self.backup_id.clone(), dt.timestamp())?; let files = list_backup_files(l2_fd, backup_time)?; list.push(BackupInfo { backup_dir, files }); @@ -208,19 +209,22 @@ pub struct BackupDir { impl BackupDir { - pub fn new(backup_type: T, backup_id: U, timestamp: i64) -> Self + pub fn new(backup_type: T, backup_id: U, timestamp: i64) -> Result where T: Into, U: Into, { - // Note: makes sure that nanoseconds is 0 - Self { - group: BackupGroup::new(backup_type.into(), backup_id.into()), - backup_time: Utc.timestamp(timestamp, 0), - } + let group = BackupGroup::new(backup_type.into(), backup_id.into()); + BackupDir::new_with_group(group, timestamp) } - pub fn new_with_group(group: BackupGroup, timestamp: i64) -> Self { - Self { group, backup_time: Utc.timestamp(timestamp, 0) } + + pub fn new_with_group(group: BackupGroup, timestamp: i64) -> Result { + let backup_time = match Utc.timestamp_opt(timestamp, 0) { + LocalResult::Single(time) => time, + _ => bail!("can't create BackupDir with invalid backup time {}", timestamp), + }; + + Ok(Self { group, backup_time }) } pub fn group(&self) -> &BackupGroup { @@ -257,7 +261,7 @@ impl std::str::FromStr for BackupDir { let group = BackupGroup::new(cap.get(1).unwrap().as_str(), cap.get(2).unwrap().as_str()); let backup_time = cap.get(3).unwrap().as_str().parse::>()?; - Ok(BackupDir::from((group, backup_time.timestamp()))) + BackupDir::try_from((group, backup_time.timestamp())) } } @@ -270,9 +274,11 @@ impl std::fmt::Display for BackupDir { } } -impl From<(BackupGroup, i64)> for BackupDir { - fn from((group, timestamp): (BackupGroup, i64)) -> Self { - Self { group, backup_time: Utc.timestamp(timestamp, 0) } +impl TryFrom<(BackupGroup, i64)> for BackupDir { + type Error = Error; + + fn try_from((group, timestamp): (BackupGroup, i64)) -> Result { + BackupDir::new_with_group(group, timestamp) } } @@ -334,7 +340,7 @@ impl BackupInfo { if file_type != nix::dir::Type::Directory { return Ok(()); } let dt = backup_time.parse::>()?; - let backup_dir = BackupDir::new(backup_type, backup_id, dt.timestamp()); + let backup_dir = BackupDir::new(backup_type, backup_id, dt.timestamp())?; let files = list_backup_files(l2_fd, backup_time)?; diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs index 25376bc9..fa132037 100644 --- a/src/bin/proxmox-backup-client.rs +++ b/src/bin/proxmox-backup-client.rs @@ -377,7 +377,7 @@ async fn list_backup_groups(param: Value) -> Result { let render_last_backup = |_v: &Value, record: &Value| -> Result { let item: GroupListItem = serde_json::from_value(record.to_owned())?; - let snapshot = BackupDir::new(item.backup_type, item.backup_id, item.last_backup); + let snapshot = BackupDir::new(item.backup_type, item.backup_id, item.last_backup)?; Ok(snapshot.relative_path().to_str().unwrap().to_owned()) }; @@ -448,7 +448,7 @@ async fn list_snapshots(param: Value) -> Result { let render_snapshot_path = |_v: &Value, record: &Value| -> Result { let item: SnapshotListItem = serde_json::from_value(record.to_owned())?; - let snapshot = BackupDir::new(item.backup_type, item.backup_id, item.backup_time); + let snapshot = BackupDir::new(item.backup_type, item.backup_id, item.backup_time)?; Ok(snapshot.relative_path().to_str().unwrap().to_owned()) }; @@ -1047,7 +1047,7 @@ async fn create_backup( None }; - let snapshot = BackupDir::new(backup_type, backup_id, backup_time.timestamp()); + let snapshot = BackupDir::new(backup_type, backup_id, backup_time.timestamp())?; let mut manifest = BackupManifest::new(snapshot); let mut catalog = None; @@ -1572,7 +1572,7 @@ async fn prune_async(mut param: Value) -> Result { let render_snapshot_path = |_v: &Value, record: &Value| -> Result { let item: PruneListItem = serde_json::from_value(record.to_owned())?; - let snapshot = BackupDir::new(item.backup_type, item.backup_id, item.backup_time); + let snapshot = BackupDir::new(item.backup_type, item.backup_id, item.backup_time)?; Ok(snapshot.relative_path().to_str().unwrap().to_owned()) }; @@ -1764,8 +1764,9 @@ async fn complete_backup_snapshot_do(param: &HashMap) -> Vec