Skip to content

Commit

Permalink
bugfix: read/write fd dropped unexpectedly.
Browse files Browse the repository at this point in the history
read/write function invokes

```
self.check_fd_flags(data, f.as_raw_fd(), flags)?;
```

which takes ownership of `data` and it's inner `File`, after the function
call returns, File will be dropped and cannot be read/write later
anymore, causing a failed read/write.

This bug is triggered when `no_open` config option is set, in this case,
`self.get_data` opens a new file and it's dropped immediately after
`check_fd_flags`.

The bug can be solved by create a new clone of data by `data.clone()`,
the original data(and inner File) can be kept in whole lifetime of read/write
function.

Signed-off-by: Wei Zhang <[email protected]>
  • Loading branch information
WeiZhang555 committed Feb 5, 2024
1 parent e58bcef commit 8e289bd
Show file tree
Hide file tree
Showing 3 changed files with 147 additions and 5 deletions.
2 changes: 0 additions & 2 deletions src/passthrough/file_handle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -320,9 +320,7 @@ impl OpenableFileHandle {
#[cfg(test)]
mod tests {
use super::*;
use nix::unistd::getuid;
use std::ffi::CString;
use std::io::Read;

fn generate_c_file_handle(
handle_bytes: usize,
Expand Down
146 changes: 145 additions & 1 deletion src/passthrough/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -980,10 +980,14 @@ mod tests {
use super::*;
use crate::abi::fuse_abi::CreateIn;
use crate::api::filesystem::*;
use crate::api::filesystem::{ZeroCopyReader, ZeroCopyWriter};
use crate::api::{Vfs, VfsOptions};
use crate::common::file_buf::FileVolatileSlice;
use crate::common::file_traits::FileReadWriteVolatile;

use caps::{CapSet, Capability};
use log;
use std::io::Read;
use std::io::{Read, Seek, SeekFrom, Write};
use std::ops::Deref;
use std::os::unix::prelude::MetadataExt;
use vmm_sys_util::{tempdir::TempDir, tempfile::TempFile};
Expand Down Expand Up @@ -1479,4 +1483,144 @@ mod tests {
assert!(!fs.cfg.no_open);
assert!(!fs.cfg.writeback);
}

impl ZeroCopyReader for File {
// Copies at most count bytes from self directly into f at offset off
// without storing it in any intermediate buffers.
fn read_to(
&mut self,
f: &mut dyn FileReadWriteVolatile,
count: usize,
off: u64,
) -> io::Result<usize> {
let mut buf = vec![0_u8; count];
let slice = unsafe { FileVolatileSlice::from_raw_ptr(buf.as_mut_ptr(), count) };

// Read from self to slice.
let ret = self.read_volatile(slice)?;
if ret > 0 {
let slice = unsafe { FileVolatileSlice::from_raw_ptr(buf.as_mut_ptr(), ret) };
// Write from slice to f at offset off.
f.write_at_volatile(slice, off)
} else {
Ok(0)
}
}
}

impl ZeroCopyWriter for File {
// Copies at most count bytes from f at offset off directly into self
// without storing it in any intermediate buffers.
fn write_from(
&mut self,
f: &mut dyn FileReadWriteVolatile,
count: usize,
off: u64,
) -> io::Result<usize> {
let mut buf = vec![0_u8; count];
let slice = unsafe { FileVolatileSlice::from_raw_ptr(buf.as_mut_ptr(), count) };
// Read from f at offset off to slice.
let ret = f.read_at_volatile(slice, off)?;

if ret > 0 {
let slice = unsafe { FileVolatileSlice::from_raw_ptr(buf.as_mut_ptr(), ret) };
// Write from slice to self.
self.write_volatile(slice)
} else {
Ok(0)
}
}

fn available_bytes(&self) -> usize {
// Max usize
usize::MAX
}
}

#[test]
fn test_generic_read_write_noopen() {
let tmpdir = TempDir::new().expect("Cannot create temporary directory.");
// Prepare passthrough fs.
let fs_cfg = Config {
do_import: false,
no_open: true,
root_dir: tmpdir.as_path().to_string_lossy().to_string(),
..Default::default()
};
let fs = PassthroughFs::<()>::new(fs_cfg.clone()).unwrap();
fs.import().unwrap();
fs.init(FsOptions::ZERO_MESSAGE_OPEN).unwrap();
fs.mount().unwrap();

// Create a new file for testing.
let ctx = Context::default();
let createin = CreateIn {
flags: libc::O_CREAT as u32,
mode: 0o644,
umask: 0,
fuse_flags: 0,
};
let file_name = CString::new("test_file").unwrap();

let (entry, _, _, _) = fs
.create(&ctx, ROOT_ID, file_name.as_c_str(), createin)
.unwrap();
let ino = entry.inode;
assert_ne!(ino, 0);
assert_ne!(ino, ROOT_ID);

// Write on the inode
let data = b"hello world";
// Write to one intermidiate temp file.
let buffer_file = TempFile::new().expect("Cannot create temporary file.");
let mut buffer_file = buffer_file.into_file();
buffer_file.write_all(data).unwrap();
let _ = buffer_file.flush();

// Read back and check.
let mut newbuf = Vec::new();
buffer_file.seek(SeekFrom::Start(0)).unwrap();
buffer_file.read_to_end(&mut newbuf).unwrap();
assert_eq!(newbuf, data);

// Call fs.write to write content to the file
buffer_file.seek(SeekFrom::Start(0)).unwrap();
let write_sz = fs
.write(
&ctx,
ino,
0,
&mut buffer_file,
data.len() as u32,
0,
None,
false,
0,
0,
)
.unwrap();
assert_eq!(write_sz, data.len());

// Create a new temp file as read buffer.
let read_buffer_file = TempFile::new().expect("Cannot create temporary file.");
let mut read_buffer_file = read_buffer_file.into_file();
let read_sz = fs
.read(
&ctx,
ino,
0,
&mut read_buffer_file,
data.len() as u32,
0,
None,
0,
)
.unwrap();
assert_eq!(read_sz, data.len());

read_buffer_file.seek(SeekFrom::Start(0)).unwrap();
let mut newbuf = Vec::new();
read_buffer_file.read_to_end(&mut newbuf).unwrap();
assert_eq!(newbuf, data);
}
}
4 changes: 2 additions & 2 deletions src/passthrough/sync_io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -665,7 +665,7 @@ impl<S: BitmapSlice + Send + Sync> FileSystem for PassthroughFs<S> {
// so data.file won't be closed.
let f = unsafe { File::from_raw_fd(data.borrow_fd().as_raw_fd()) };

self.check_fd_flags(data, f.as_raw_fd(), flags)?;
self.check_fd_flags(data.clone(), f.as_raw_fd(), flags)?;

let mut f = ManuallyDrop::new(f);

Expand All @@ -692,7 +692,7 @@ impl<S: BitmapSlice + Send + Sync> FileSystem for PassthroughFs<S> {
// so data.file won't be closed.
let f = unsafe { File::from_raw_fd(data.borrow_fd().as_raw_fd()) };

self.check_fd_flags(data, f.as_raw_fd(), flags)?;
self.check_fd_flags(data.clone(), f.as_raw_fd(), flags)?;

if self.seal_size.load(Ordering::Relaxed) {
let st = stat_fd(&f, None)?;
Expand Down

0 comments on commit 8e289bd

Please sign in to comment.