Re: [PATCH proxmox-backup 6/9] datastore: check that index files are regular files

[Christian Ebner <c.ebner@proxmox.com>]

Two nits inline.

On 7/31/26 4:25 PM, Robert Obkircher wrote:
> Return an explicit error message when trying to mmap something other
> than a regular file. Previously, this was caught implicitly by the
> non-zero size check or by a failing seek.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
>   pbs-datastore/src/dynamic_index.rs | 16 ++++++++--------
>   pbs-datastore/src/fixed_index.rs   | 15 ++++++++-------
>   2 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/pbs-datastore/src/dynamic_index.rs b/pbs-datastore/src/dynamic_index.rs
> index 7ee8bdcab..dd0b86f3f 100644
> --- a/pbs-datastore/src/dynamic_index.rs
> +++ b/pbs-datastore/src/dynamic_index.rs
> @@ -8,6 +8,7 @@ use std::sync::{Arc, Mutex};
>   use std::task::Context;
>   
>   use anyhow::{Error, bail, format_err};
> +use nix::sys::stat::{SFlag, fstat};
>   
>   use proxmox_io::ReadExt;
>   use proxmox_sys::mmap::Mmap;
> @@ -96,17 +97,16 @@ impl DynamicIndexReader {
>       }
>   
>       pub fn new(mut file: std::fs::File) -> Result<Self, Error> {
> +        let stat = fstat(file.as_raw_fd()).map_err(|e| format_err!("fstat failed - {e}"))?;
> +        if (stat.st_mode & SFlag::S_IFMT.bits()) != SFlag::S_IFREG.bits() {
> +            bail!("not a regular file");
> +        }
> +
>           // FIXME: This is NOT OUR job! Check the callers of this method and remove this!
>           file.seek(SeekFrom::Start(0))?;
>   
>           let header_size = std::mem::size_of::<DynamicIndexHeader>();
>   
> -        let rawfd = file.as_raw_fd();
> -        let stat = match nix::sys::stat::fstat(rawfd) {
> -            Ok(stat) => stat,
> -            Err(err) => bail!("fstat failed - {}", err),
> -        };
> -
>           let size = stat.st_size as usize;
>   
>           if size < header_size {
> @@ -611,11 +611,11 @@ mod tests {
>   
>           check_error_contains(
>               DynamicIndexReader::open(Path::new("/dev/stdin")),
> -            "Illegal seek",
> +            "not a regular file",
>           );
>           check_error_contains(
>               DynamicIndexReader::open(Path::new("/dev/zero")),
> -            "index too small (0)",
> +            "not a regular file",
>           );

nit: this check is now somewhat redundant? The check for to small index 
is already performed further below, so this can be dropped IMO.

>   
>           let path = dir.path().join("file");
> diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
> index 0145f93d3..1d3d0170a 100644
> --- a/pbs-datastore/src/fixed_index.rs
> +++ b/pbs-datastore/src/fixed_index.rs
> @@ -5,6 +5,7 @@ use std::path::{Path, PathBuf};
>   use std::ptr::NonNull;
>   
>   use anyhow::{Context, Error, bail, format_err};
> +use nix::sys::stat::{SFlag, fstat};
>   
>   use proxmox_io::ReadExt;
>   use proxmox_uuid::Uuid;
> @@ -60,15 +61,15 @@ impl FixedIndexReader {
>       }
>   
>       pub fn new(mut file: std::fs::File) -> Result<Self, Error> {
> +        let stat = fstat(file.as_raw_fd()).map_err(|e| format_err!("fstat failed - {e}"))?;
> +        if (stat.st_mode & SFlag::S_IFMT.bits()) != SFlag::S_IFREG.bits() {
> +            bail!("not a regular file");
> +        }
> +
>           file.seek(SeekFrom::Start(0))?;
>   
>           let header_size = std::mem::size_of::<FixedIndexHeader>();
>   
> -        let stat = match nix::sys::stat::fstat(file.as_raw_fd()) {
> -            Ok(stat) => stat,
> -            Err(err) => bail!("fstat failed - {}", err),
> -        };
> -
>           let size = stat.st_size as usize;
>   
>           if size < header_size {
> @@ -608,11 +609,11 @@ mod tests {
>   
>           check_error_contains(
>               FixedIndexReader::open(Path::new("/dev/stdin")),
> -            "Illegal seek",
> +            "not a regular file",
>           );
>           check_error_contains(
>               FixedIndexReader::open(Path::new("/dev/zero")),
> -            "index too small (0)",
> +            "not a regular file",
>           );

nit: same as above, the second error testcase is now redundant.

>   
>           let path = dir.path().join("file");