[pbs-devel] [PATCH proxmox-backup v8 4/4] fix: api: avoid race condition in set_backup_owner

[Christian Ebner]

On 3/24/25 13:51, Shannon Sterz wrote:
> when two clients change the owner of a backup store, a race condition
> arose. add locking to avoid this.
> 
> Signed-off-by: Shannon Sterz <s.sterz at proxmox.com>
> ---
>   src/api2/admin/datastore.rs | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 483e595c..3e532636 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -7,7 +7,7 @@ use std::os::unix::ffi::OsStrExt;
>   use std::path::{Path, PathBuf};
>   use std::sync::Arc;
>   
> -use anyhow::{bail, format_err, Error};
> +use anyhow::{bail, format_err, Context, Error};
>   use futures::*;
>   use hyper::http::request::Parts;
>   use hyper::{header, Body, Response, StatusCode};
> @@ -2347,10 +2347,9 @@ pub async fn set_backup_owner(
>           let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;
>   
>           let backup_group = datastore.backup_group(ns, backup_group);
> +        let owner = backup_group.get_owner()?;

this needs to read the content from the owner file

>   
>           if owner_check_required {
> -            let owner = backup_group.get_owner()?;
> -
>               let allowed = match (owner.is_token(), new_owner.is_token()) {
>                   (true, true) => {
>                       // API token to API token, owned by same user
> @@ -2397,6 +2396,14 @@ pub async fn set_backup_owner(
>               );
>           }
>   
> +        let _guard = backup_group
> +            .lock()
> +            .with_context(|| format!("while setting the owner of group '{backup_group:?}'"))?;
> +
> +        if owner != backup_group.get_owner()? {

same here.

> +            bail!("{owner} does not own this group anymore");
> +        }
> +
>           backup_group.set_owner(&new_owner, true)?;
>   
>           Ok(())

reading from the filesystem is more expensive than the ownership checks 
if required, so IMO it is better to lock the whole operation instead of 
reading the owner file twice. Or is this done to avoid locking over and 
over by unauthorized users?