Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 46 additions & 3 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,9 @@
//! ld.detach().unwrap();
//! ```
use crate::bindings::{
loop_info64, LOOP_CLR_FD, LOOP_CTL_ADD, LOOP_CTL_GET_FREE, LOOP_SET_CAPACITY, LOOP_SET_FD,
LOOP_SET_STATUS64, LO_FLAGS_AUTOCLEAR, LO_FLAGS_PARTSCAN, LO_FLAGS_READ_ONLY,
loop_info64, LOOP_CLR_FD, LOOP_CTL_ADD, LOOP_CTL_GET_FREE, LOOP_GET_STATUS64,
LOOP_SET_CAPACITY, LOOP_SET_FD, LOOP_SET_STATUS64, LO_FLAGS_AUTOCLEAR, LO_FLAGS_PARTSCAN,
LO_FLAGS_READ_ONLY,
};
#[cfg(feature = "direct_io")]
use bindings::LOOP_SET_DIRECT_IO;
Expand All @@ -52,13 +53,17 @@ use std::{
path::{Path, PathBuf},
};

use loname::Name;

#[allow(non_camel_case_types)]
#[allow(dead_code)]
#[allow(non_snake_case)]
mod bindings {
include!(concat!(env!("OUT_DIR"), "/bindings.rs"));
}

mod loname;

#[cfg(all(not(target_os = "android"), not(target_env = "musl")))]
type IoctlRequest = libc::c_ulong;
#[cfg(any(target_os = "android", target_env = "musl"))]
Expand Down Expand Up @@ -243,9 +248,15 @@ impl LoopDevice {
fn attach_with_loop_info(
&self, // TODO should be mut? - but changing it is a breaking change
backing_file: impl AsRef<Path>,
info: loop_info64,
mut info: loop_info64,
) -> io::Result<()> {
let write_access = (info.lo_flags & LO_FLAGS_READ_ONLY) == 0;

// store backing file name in the info
let name = loname::Name::from_path(&backing_file).unwrap_or_default();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the purpose of this but I'm unsure why you need the special Name struct for marshalling and unmarshalling. I feel that it would be better to use existing Path and PathBuf methods, as_os_str() or something. Can you tell me why the concern w/ null terminators in the to_string method? Thanks.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern was compatibility with C code.
I found that kernel actually takes care of the terminator here.

I did a brief check that name setup through as_os_str is correctly displayed by losetup -l, but that's not a reliable source, because ...

losetup actually leverages cat /sys/block/loop0/loop/backing_file on my system. It isn't limited at 64 chars and a way more straight-forward path.

I think I need to get rid of lo_file_name logic but keep info around for future use.

Do you think it's reasonable ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting the full name from /sys/block/loop0/loop/backing_file isn't that simple, as it requires to know the loop0 part, which, in its turn, requires an info call. The only real advantage is that it allows to get the full name in all cases.
LMK if you want to see the version with as_os_str anyways

info.lo_file_name = name.0;
info.lo_crypt_name = name.0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you consider only setting lo_file_name and not lo_crypt_name here? If not, are you using io_crypt_name? I've made some preliminary inquiries and it seems not entirely clear what it is for.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you consider only setting lo_file_name and not lo_crypt_name here?
Sure, will do.

I don't use lo_crypt_name, but I found that losetup sets both and they're semantically similar.


let bf = OpenOptions::new()
.read(true)
.write(write_access)
Expand Down Expand Up @@ -312,6 +323,23 @@ impl LoopDevice {
std::fs::read_link(&p).ok()
}

/// Try to obtain the original file path used on mapping
/// The method ignores ioctl errors
///
/// # Return
/// The path expected to be stored in the loop_info64.
/// If it's not there, method returns None, otherwise - the string stored
pub fn original_path(&self) -> Option<PathBuf> {
self.info().ok().and_then(|info| {
let name = Name(info.lo_file_name).to_string();
if name.is_empty() {
None
} else {
Some(PathBuf::from(name))
}
})
}
Comment on lines +326 to +341
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Lossless original_path: build PathBuf from raw bytes, not a UTF‑8 String

Current code can mangle non‑UTF‑8 paths. Use the bytes up to the first NUL and construct an OsString via OsStringExt.

Apply this diff:

-    pub fn original_path(&self) -> Option<PathBuf> {
-        self.info().ok().and_then(|info| {
-            let name = Name(info.lo_file_name).to_string();
-            if name.is_empty() {
-                None
-            } else {
-                Some(PathBuf::from(name))
-            }
-        })
-    }
+    pub fn original_path(&self) -> Option<PathBuf> {
+        self.info().ok().and_then(|info| {
+            let raw = &info.lo_file_name;
+            let end = raw.iter().position(|&b| b == 0).unwrap_or(raw.len());
+            if end == 0 {
+                None
+            } else {
+                Some(PathBuf::from(
+                    std::os::unix::ffi::OsStringExt::from_vec(raw[..end].to_vec()),
+                ))
+            }
+        })
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// Try to obtain the original file path used on mapping
/// The method ignores ioctl errors
///
/// # Return
/// The path expected to be stored in the loop_info64.
/// If it's not there, method returns None, otherwise - the string stored
pub fn original_path(&self) -> Option<PathBuf> {
self.info().ok().and_then(|info| {
let name = Name(info.lo_file_name).to_string();
if name.is_empty() {
None
} else {
Some(PathBuf::from(name))
}
})
}
pub fn original_path(&self) -> Option<PathBuf> {
self.info().ok().and_then(|info| {
let raw = &info.lo_file_name;
let end = raw.iter().position(|&b| b == 0).unwrap_or(raw.len());
if end == 0 {
None
} else {
Some(PathBuf::from(
std::os::unix::ffi::OsStringExt::from_vec(raw[..end].to_vec()),
))
}
})
}
🤖 Prompt for AI Agents
src/lib.rs around lines 326 to 341: current original_path converts the C-style
file name to a UTF-8 String which can mangle non‑UTF‑8 file names; instead,
extract the raw byte array from info.lo_file_name up to the first NUL byte,
convert that byte slice into an OsString (using OsStringExt::from_vec on Unix),
then build and return a PathBuf from that OsString (return None if the byte
slice is empty). Ensure you avoid any UTF-8 String conversion so paths are
preserved losslessly.


/// Get the device major number
///
/// # Errors
Expand Down Expand Up @@ -390,6 +418,21 @@ impl LoopDevice {
Ok(())
}

/// Obtain loop_info64 struct for the loop device
/// # Return
/// Ok(loop_info64) - successfully obtained info
/// Err(std::io::Error) - error from ioctl
pub fn info(&self) -> Result<loop_info64, std::io::Error> {
let empty_info = Box::new(loop_info64::default());
let fd = self.device.as_raw_fd();

unsafe {
let ptr = Box::into_raw(empty_info);
let ret_code = libc::ioctl(fd.as_raw_fd(), LOOP_GET_STATUS64 as IoctlRequest, ptr);
ioctl_to_error(ret_code).map(|_| *Box::from_raw(ptr))
}
}

Comment on lines +421 to +435
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix memory leak and invalid fd usage in info()

  • Leak: Box is never freed on ioctl error.
  • fd.as_raw_fd() on a RawFd is invalid.
  • Use a stack-allocated struct and pass a mutable pointer.

Apply this diff:

-    pub fn info(&self) -> Result<loop_info64, std::io::Error> {
-        let empty_info = Box::new(loop_info64::default());
-        let fd = self.device.as_raw_fd();
-
-        unsafe {
-            let ptr = Box::into_raw(empty_info);
-            let ret_code = libc::ioctl(fd.as_raw_fd(), LOOP_GET_STATUS64 as IoctlRequest, ptr);
-            ioctl_to_error(ret_code).map(|_| *Box::from_raw(ptr))
-        }
-    }
+    pub fn info(&self) -> Result<loop_info64, std::io::Error> {
+        let mut info = loop_info64::default();
+        let ret = unsafe {
+            libc::ioctl(
+                self.device.as_raw_fd() as c_int,
+                LOOP_GET_STATUS64 as IoctlRequest,
+                &mut info,
+            )
+        };
+        ioctl_to_error(ret).map(|_| info)
+    }
🤖 Prompt for AI Agents
In src/lib.rs around lines 421-435, the info() function currently leaks memory
by boxing the struct and never freeing it on ioctl error, and incorrectly calls
as_raw_fd() on a RawFd; replace the boxed allocation with a stack-allocated
mutable loop_info64 (let mut info = loop_info64::default()), call libc::ioctl
with the raw fd (use self.device.as_raw_fd() once, no extra as_raw_fd() on an
fd), pass &mut info as a *mut loop_info64, check ioctl_to_error(ret_code) and on
success return Ok(info), ensuring no heap allocation and no leak on error.

/// Enable or disable direct I/O for the backing file.
///
/// # Errors
Expand Down
88 changes: 88 additions & 0 deletions src/loname.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
//! Configuration structures for loop device
use std::path::Path;

use crate::bindings::LO_NAME_SIZE;

/// Loop device name
#[repr(C)]
#[derive(Debug)]
pub struct Name(pub [libc::__u8; LO_NAME_SIZE as usize]);

/// Allow to construct the config easily
impl Default for Name {
fn default() -> Self {
Self([0; LO_NAME_SIZE as usize])
}
}

/// Conversions simplifiers
impl Name {
pub fn from_path<Y: AsRef<Path>>(path: Y) -> Result<Self, String> {
let s = path.as_ref().as_os_str().as_encoded_bytes();
if s.len() > LO_NAME_SIZE as usize {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned that this use of .len() is not correct, because it means the bytes the implementation has allocated, not the actual length of the characters required to encode the path. I'm going to ask another person to look at this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LO_NAME_SIZE comes from C header and limits number of bytes, AFAIU. So the comparison should be right.

return Err(format!(
"too many bytes in the provided loop dev source file path: {}, max {LO_NAME_SIZE}",
s.len()
));
}
let mut data: [u8; 64] = [0; LO_NAME_SIZE as usize];
for (idx, byte) in s.iter().enumerate() {
data[idx] = *byte;
}
Ok(Self(data))
Comment on lines +29 to +33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid hard-coding 64; use LO_NAME_SIZE and copy_from_slice

Hard-coding the array length risks mismatch if LO_NAME_SIZE changes. Also, use copy_from_slice for clarity and bounds safety (you already guard len).

Apply this diff:

-        let mut data: [u8; 64] = [0; LO_NAME_SIZE as usize];
-        for (idx, byte) in s.iter().enumerate() {
-            data[idx] = *byte;
-        }
+        let mut data = [0u8; LO_NAME_SIZE as usize];
+        data[..s.len()].copy_from_slice(s);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let mut data: [u8; 64] = [0; LO_NAME_SIZE as usize];
for (idx, byte) in s.iter().enumerate() {
data[idx] = *byte;
}
Ok(Self(data))
let mut data = [0u8; LO_NAME_SIZE as usize];
data[..s.len()].copy_from_slice(s);
Ok(Self(data))
🤖 Prompt for AI Agents
In src/loname.rs around lines 29 to 33, the code hard-codes the array length as
64 and manually copies bytes; change the array initialization to use
LO_NAME_SIZE (e.g. let mut data: [u8; LO_NAME_SIZE as usize] = [0; LO_NAME_SIZE
as usize];) and replace the for-loop with a bounds-safe copy_from_slice call
(e.g. data[..s.len()].copy_from_slice(s)); keep the existing length guard and
ensure any casts to usize use LO_NAME_SIZE as usize.

}
}

#[allow(clippy::to_string_trait_impl)]
impl ToString for Name {
fn to_string(&self) -> String {
self.0
.iter()
.filter_map(|ch| {
let ch: char = *ch as char;
if ch == '\0' {
None
} else {
Some(ch)
}
})
.collect::<String>()
}
}
Comment on lines +37 to +52
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

String conversion is lossy for non‑UTF‑8 paths; switch to Display and expose raw bytes

Casting each u8 to char and collecting into a String changes the byte sequence for values >= 0x80, breaking round‑trips for non‑UTF‑8 paths. Implement Display for human output and expose a bytes view for exact reconstruction.

Apply this diff:

-#[allow(clippy::to_string_trait_impl)]
-impl ToString for Name {
-    fn to_string(&self) -> String {
-        self.0
-            .iter()
-            .filter_map(|ch| {
-                let ch: char = *ch as char;
-                if ch == '\0' {
-                    None
-                } else {
-                    Some(ch)
-                }
-            })
-            .collect::<String>()
-    }
-}
+impl std::fmt::Display for Name {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        let end = self.0.iter().position(|&b| b == 0).unwrap_or(self.0.len());
+        let s = String::from_utf8_lossy(&self.0[..end]);
+        f.write_str(&s)
+    }
+}

And add a bytes accessor to enable lossless OS string construction:

 impl Name {
     pub fn from_path<Y: AsRef<Path>>(path: Y) -> Result<Self, String> {
         let s = path.as_ref().as_os_str().as_encoded_bytes();
         if s.len() > LO_NAME_SIZE as usize {
             return Err(format!(
                 "too many bytes in the provided loop dev source file path: {}, max {LO_NAME_SIZE}",
                 s.len()
             ));
         }
         let mut data = [0u8; LO_NAME_SIZE as usize];
         data[..s.len()].copy_from_slice(s);
         Ok(Self(data))
     }
+
+    /// Bytes up to the first NUL terminator.
+    pub fn as_bytes(&self) -> &[u8] {
+        let end = self.0.iter().position(|&b| b == 0).unwrap_or(self.0.len());
+        &self.0[..end]
+    }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#[allow(clippy::to_string_trait_impl)]
impl ToString for Name {
fn to_string(&self) -> String {
self.0
.iter()
.filter_map(|ch| {
let ch: char = *ch as char;
if ch == '\0' {
None
} else {
Some(ch)
}
})
.collect::<String>()
}
}
// Replace the lossy `ToString` impl with a `Display` impl.
impl std::fmt::Display for Name {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
let end = self.0.iter().position(|&b| b == 0).unwrap_or(self.0.len());
let s = String::from_utf8_lossy(&self.0[..end]);
f.write_str(&s)
}
}
// In the `Name` impl, add a lossless bytes accessor.
impl Name {
pub fn from_path<Y: AsRef<Path>>(path: Y) -> Result<Self, String> {
let s = path.as_ref().as_os_str().as_encoded_bytes();
if s.len() > LO_NAME_SIZE as usize {
return Err(format!(
"too many bytes in the provided loop dev source file path: {}, max {LO_NAME_SIZE}",
s.len()
));
}
let mut data = [0u8; LO_NAME_SIZE as usize];
data[..s.len()].copy_from_slice(s);
Ok(Self(data))
}
/// Bytes up to the first NUL terminator.
pub fn as_bytes(&self) -> &[u8] {
let end = self.0.iter().position(|&b| b == 0).unwrap_or(self.0.len());
&self.0[..end]
}
}
🤖 Prompt for AI Agents
In src/loname.rs around lines 37 to 52, replace the current lossy ToString impl
that casts u8s to char with a Display impl that formats the Name for humans
using String::from_utf8_lossy(&self.0) (so printing is safe) and remove the
clippy allow and the ToString impl; additionally add a public bytes accessor,
e.g. pub fn as_bytes(&self) -> &[u8] { &self.0 }, so callers can reconstruct
exact, lossless OS strings from the raw bytes.


#[cfg(test)]
mod test {
use std::path::PathBuf;

use super::Name;

#[test]
fn test_name_empty() {
let name = Name::default();
for num in name.0 {
assert_eq!(0, num);
}
}

#[test]
fn test_name_from_to() {
let path_string = "/a/b/some-file/cool name";
let path = PathBuf::from(&path_string);
let name = Name::from_path(path);

assert_eq!(path_string, name.unwrap().to_string());
}

#[test]
fn test_name_too_long() {
let path_string = "/too-long/too-long/too-long/too-long/too-long/too-long/too-long--";
let path = PathBuf::from(&path_string);
let name = Name::from_path(path);

assert_eq!(
"too many bytes in the provided loop dev source file path: 65, max 64",
name.unwrap_err()
)
}
}
51 changes: 50 additions & 1 deletion tests/integration_test.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use loopdev::{LoopControl, LoopDevice};
use std::path::PathBuf;
use std::{fs::File, os::fd::AsFd, path::PathBuf};

mod util;
use crate::util::{
Expand Down Expand Up @@ -220,3 +220,52 @@ fn add_a_loop_device() {
assert!(lc.add(1).is_ok());
assert!(lc.add(1).is_err());
}

#[test]
fn test_device_name() {
let _lock = setup();

let lc = LoopControl::open().expect("should be able to open the LoopControl device");

let file = create_backing_file(128 * 1024 * 1024);
let file_path = file.to_path_buf();
let ld0 = lc
.next_free()
.expect("should not error finding the next free loopback device");

ld0.with()
.attach(&file)
.expect("should not error attaching the backing file to the loopdev");

file.close().expect("should delete the temp backing file");

assert_eq!(
file_path,
ld0.original_path()
.expect("expected a correct loop device name")
);

assert!(ld0.info().is_ok());
}

#[test]
fn test_device_name_absent() {
let _lock = setup();
let file_size = 128 * 1024 * 1024;

let lc = LoopControl::open().expect("should be able to open the LoopControl device");

let file =
File::open(create_backing_file(file_size)).expect("should be able to open our temp file");
let ld0 = lc
.next_free()
.expect("should not error finding the next free loopback device");

ld0.with()
.attach_fd(file.as_fd())
.expect("should not error attaching the backing file to the loopdev");

assert!(ld0.info().is_ok());

assert!(ld0.original_path().is_none());
}