From ea8b884995b793a1892e3df9dc4c890d8b25d320 Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Tue, 28 Jul 2026 17:02:50 -0300 Subject: [PATCH] fix: measure the install target's own capacity, not the devtmpfs holding its node ensure_disk_space() asked statvfs about the install target, but every caller hands it a device node: /dev/mmcblk0p2 for raw and copy, an mtd character device for flash, a ubi volume for ubifs. statvfs reports on the filesystem holding the path it is given, and for anything under /dev that is the devtmpfs the kernel sizes after the available RAM. The check therefore compared the object against a fraction of RAM and never once looked at the partition the object was about to be written to. The same number came back for every device on a machine here, 3.2G, which is what df reports for /dev, while the devices themselves are 1.8T, 511M and 14.5G. A device large enough for the update was rejected, and, worse, one too small was accepted whenever RAM happened to exceed it. Device nodes are now asked for their own capacity by seeking to the end, which block devices, mtd character devices and ubi volumes all answer with their size. That is portable in a way the ioctls are not: BLKGETSIZE64 is declared in terms of size_t, so its request number differs between 32 and 64 bit userspace while the kernel writes back 64 bits either way, and mtd and ubi would each need an ioctl of their own besides. Paths that really do live on a filesystem keep going through statvfs. Widening the statvfs fields before multiplying fixes a second way the same line could mislead. They are c_ulong and fsblkcnt_t, 32 bit on the armv7 targets this runs on, so the product wrapped for any filesystem past 4GiB and the conversion to u64 came too late to help. The count is also taken in units of f_frsize rather than f_bsize, which is what f_bfree is expressed in. The loop device test needs root, as creating one does, so it is marked ignored alongside the mtd tests that already are. Its fixture is built on the loopdev crate and on the mutex the copy and tarball tests already serialize next_free() with, both of which had to be reachable from here, rather than on a second way of doing what the crate can already do. Left alone: copy and tarball mount a filesystem onto the target and write files into it, so the device's capacity is an upper bound for them rather than an answer, ignoring both what is already there and the filesystem's own overhead. Measuring those properly means a statvfs on the mount point once it is mounted, which is a change of its own. --- updatehub/src/object/installer/mod.rs | 2 +- updatehub/src/utils/fs.rs | 115 ++++++++++++++++++++++++-- 2 files changed, 110 insertions(+), 7 deletions(-) diff --git a/updatehub/src/object/installer/mod.rs b/updatehub/src/object/installer/mod.rs index 67e408d6..d00e4f8f 100644 --- a/updatehub/src/object/installer/mod.rs +++ b/updatehub/src/object/installer/mod.rs @@ -129,7 +129,7 @@ where } #[cfg(test)] -mod tests { +pub(crate) mod tests { use super::*; use lazy_static::lazy_static; use std::{ diff --git a/updatehub/src/utils/fs.rs b/updatehub/src/utils/fs.rs index 887e9323..96300a60 100644 --- a/updatehub/src/utils/fs.rs +++ b/updatehub/src/utils/fs.rs @@ -9,7 +9,7 @@ use pkg_schema::definitions::{ target_permissions::{Gid, Uid}, }; use slog_scope::trace; -use std::{io, path::Path}; +use std::{io, io::Seek, os::unix::fs::FileTypeExt, path::Path}; use sys_mount::{Mount, Unmount, UnmountDrop}; pub(crate) struct MountGuard { @@ -25,11 +25,7 @@ impl MountGuard { pub(crate) fn ensure_disk_space(target: &Path, required: u64) -> Result<()> { trace!("looking for {} free bytes on {:?}", required, target); - let stat = nix::sys::statvfs::statvfs(target)?; - - // stat fields might be 32 or 64 bytes depending on host arch - #[allow(clippy::useless_conversion)] - let available = u64::from(stat.block_size() * stat.blocks_free()); + let available = available_space(target)?; if required > available { return Err(Error::NotEnoughSpace { available, required }); @@ -37,6 +33,30 @@ pub(crate) fn ensure_disk_space(target: &Path, required: u64) -> Result<()> { Ok(()) } +/// Returns how many bytes may be written to `target`. +/// +/// `statvfs` reports on the filesystem holding the given path, so pointing it +/// at a device node measures the devtmpfs mounted on `/dev` -- which is sized +/// after the available RAM -- instead of the device the objects are installed +/// onto. Device nodes are therefore asked for their own capacity, which a seek +/// to the end reports for block, MTD and UBI volume devices alike. +fn available_space(target: &Path) -> Result { + let file_type = std::fs::metadata(target)?.file_type(); + + if file_type.is_block_device() || file_type.is_char_device() { + trace!("{:?} is a device node, asking it for its capacity", target); + return Ok(std::fs::File::open(target)?.seek(io::SeekFrom::End(0))?); + } + + let stat = nix::sys::statvfs::statvfs(target)?; + + // stat fields might be 32 or 64 bits wide depending on the host arch, so widen + // them before multiplying to keep filesystems bigger than 4 GiB from + // overflowing on 32 bit targets + #[allow(clippy::useless_conversion)] + Ok(u64::from(stat.fragment_size()) * u64::from(stat.blocks_free())) +} + pub(crate) fn is_executable_in_path(cmd: &str) -> Result<()> { trace!("checking if {} is executable", cmd); match quale::which(cmd) { @@ -102,3 +122,86 @@ pub(crate) fn chown(path: &Path, uid: &Option, gid: &Option) -> Result gid.as_ref().map(|id| nix::unistd::Gid::from_raw(id.as_u32())), )?) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::object::installer::tests::SERIALIZE; + use pretty_assertions::assert_eq; + use std::path::PathBuf; + + // Big enough to tell a loop device apart from the devtmpfs on /dev, small + // enough to stay sparse on any builder. + const LOOP_DEVICE_SIZE: u64 = 64 * 1024 * 1024; + + struct FakeLoopDevice { + loopdev: loopdev::LoopDevice, + device: PathBuf, + _backing_file: tempfile::NamedTempFile, + } + + impl FakeLoopDevice { + fn new(size: u64) -> Result { + let backing_file = tempfile::NamedTempFile::new()?; + backing_file.as_file().set_len(size)?; + + // Loop device next_free is not thread safe + let mutex = SERIALIZE.clone(); + let _mutex = mutex.lock().unwrap(); + let loopdev = loopdev::LoopControl::open()?.next_free()?; + let device = loopdev.path().unwrap(); + loopdev.attach_file(backing_file.path())?; + + Ok(FakeLoopDevice { loopdev, device, _backing_file: backing_file }) + } + } + + impl Drop for FakeLoopDevice { + fn drop(&mut self) { + if let Err(e) = self.loopdev.detach() { + eprintln!("Failed to cleanup FakeLoopDevice, Error: {e}"); + } + } + } + + #[test] + fn regular_files_are_measured_by_their_filesystem() { + let dir = tempfile::tempdir().unwrap(); + let file = dir.path().join("object"); + std::fs::write(&file, [0; 16]).unwrap(); + + // The seek a device node is asked for would answer with the file's own + // 16 bytes instead. + assert!(available_space(&file).unwrap() > 16); + } + + #[test] + fn a_requirement_beyond_the_available_space_is_rejected() { + let dir = tempfile::tempdir().unwrap(); + let available = available_space(dir.path()).unwrap(); + + ensure_disk_space(dir.path(), available).unwrap(); + assert!(matches!( + ensure_disk_space(dir.path(), available + 1), + Err(Error::NotEnoughSpace { .. }) + )); + } + + // Requires root, as creating a loop device does. + #[test] + #[ignore] + fn device_nodes_are_measured_by_their_own_capacity() { + let loop_device = FakeLoopDevice::new(LOOP_DEVICE_SIZE).unwrap(); + + // Asking for the directory holding the node is asking the devtmpfs mounted + // on /dev, which is the answer the check used to settle for. + assert_ne!(available_space(Path::new("/dev")).unwrap(), LOOP_DEVICE_SIZE); + + assert_eq!(available_space(&loop_device.device).unwrap(), LOOP_DEVICE_SIZE); + ensure_disk_space(&loop_device.device, LOOP_DEVICE_SIZE).unwrap(); + assert!(matches!( + ensure_disk_space(&loop_device.device, LOOP_DEVICE_SIZE + 1), + Err(Error::NotEnoughSpace { .. }) + )); + } +}