Skip to content

Commit 8e289bd

Browse files
committed
bugfix: read/write fd dropped unexpectedly.
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 <weizhang555.zw@gmail.com>
1 parent e58bcef commit 8e289bd

File tree

3 files changed

+147
-5
lines changed

3 files changed

+147
-5
lines changed

src/passthrough/file_handle.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -320,9 +320,7 @@ impl OpenableFileHandle {
320320
#[cfg(test)]
321321
mod tests {
322322
use super::*;
323-
use nix::unistd::getuid;
324323
use std::ffi::CString;
325-
use std::io::Read;
326324

327325
fn generate_c_file_handle(
328326
handle_bytes: usize,

src/passthrough/mod.rs

Lines changed: 145 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -980,10 +980,14 @@ mod tests {
980980
use super::*;
981981
use crate::abi::fuse_abi::CreateIn;
982982
use crate::api::filesystem::*;
983+
use crate::api::filesystem::{ZeroCopyReader, ZeroCopyWriter};
983984
use crate::api::{Vfs, VfsOptions};
985+
use crate::common::file_buf::FileVolatileSlice;
986+
use crate::common::file_traits::FileReadWriteVolatile;
987+
984988
use caps::{CapSet, Capability};
985989
use log;
986-
use std::io::Read;
990+
use std::io::{Read, Seek, SeekFrom, Write};
987991
use std::ops::Deref;
988992
use std::os::unix::prelude::MetadataExt;
989993
use vmm_sys_util::{tempdir::TempDir, tempfile::TempFile};
@@ -1479,4 +1483,144 @@ mod tests {
14791483
assert!(!fs.cfg.no_open);
14801484
assert!(!fs.cfg.writeback);
14811485
}
1486+
1487+
impl ZeroCopyReader for File {
1488+
// Copies at most count bytes from self directly into f at offset off
1489+
// without storing it in any intermediate buffers.
1490+
fn read_to(
1491+
&mut self,
1492+
f: &mut dyn FileReadWriteVolatile,
1493+
count: usize,
1494+
off: u64,
1495+
) -> io::Result<usize> {
1496+
let mut buf = vec![0_u8; count];
1497+
let slice = unsafe { FileVolatileSlice::from_raw_ptr(buf.as_mut_ptr(), count) };
1498+
1499+
// Read from self to slice.
1500+
let ret = self.read_volatile(slice)?;
1501+
if ret > 0 {
1502+
let slice = unsafe { FileVolatileSlice::from_raw_ptr(buf.as_mut_ptr(), ret) };
1503+
// Write from slice to f at offset off.
1504+
f.write_at_volatile(slice, off)
1505+
} else {
1506+
Ok(0)
1507+
}
1508+
}
1509+
}
1510+
1511+
impl ZeroCopyWriter for File {
1512+
// Copies at most count bytes from f at offset off directly into self
1513+
// without storing it in any intermediate buffers.
1514+
fn write_from(
1515+
&mut self,
1516+
f: &mut dyn FileReadWriteVolatile,
1517+
count: usize,
1518+
off: u64,
1519+
) -> io::Result<usize> {
1520+
let mut buf = vec![0_u8; count];
1521+
let slice = unsafe { FileVolatileSlice::from_raw_ptr(buf.as_mut_ptr(), count) };
1522+
// Read from f at offset off to slice.
1523+
let ret = f.read_at_volatile(slice, off)?;
1524+
1525+
if ret > 0 {
1526+
let slice = unsafe { FileVolatileSlice::from_raw_ptr(buf.as_mut_ptr(), ret) };
1527+
// Write from slice to self.
1528+
self.write_volatile(slice)
1529+
} else {
1530+
Ok(0)
1531+
}
1532+
}
1533+
1534+
fn available_bytes(&self) -> usize {
1535+
// Max usize
1536+
usize::MAX
1537+
}
1538+
}
1539+
1540+
#[test]
1541+
fn test_generic_read_write_noopen() {
1542+
let tmpdir = TempDir::new().expect("Cannot create temporary directory.");
1543+
// Prepare passthrough fs.
1544+
let fs_cfg = Config {
1545+
do_import: false,
1546+
no_open: true,
1547+
root_dir: tmpdir.as_path().to_string_lossy().to_string(),
1548+
..Default::default()
1549+
};
1550+
let fs = PassthroughFs::<()>::new(fs_cfg.clone()).unwrap();
1551+
fs.import().unwrap();
1552+
fs.init(FsOptions::ZERO_MESSAGE_OPEN).unwrap();
1553+
fs.mount().unwrap();
1554+
1555+
// Create a new file for testing.
1556+
let ctx = Context::default();
1557+
let createin = CreateIn {
1558+
flags: libc::O_CREAT as u32,
1559+
mode: 0o644,
1560+
umask: 0,
1561+
fuse_flags: 0,
1562+
};
1563+
let file_name = CString::new("test_file").unwrap();
1564+
1565+
let (entry, _, _, _) = fs
1566+
.create(&ctx, ROOT_ID, file_name.as_c_str(), createin)
1567+
.unwrap();
1568+
let ino = entry.inode;
1569+
assert_ne!(ino, 0);
1570+
assert_ne!(ino, ROOT_ID);
1571+
1572+
// Write on the inode
1573+
let data = b"hello world";
1574+
// Write to one intermidiate temp file.
1575+
let buffer_file = TempFile::new().expect("Cannot create temporary file.");
1576+
let mut buffer_file = buffer_file.into_file();
1577+
buffer_file.write_all(data).unwrap();
1578+
let _ = buffer_file.flush();
1579+
1580+
// Read back and check.
1581+
let mut newbuf = Vec::new();
1582+
buffer_file.seek(SeekFrom::Start(0)).unwrap();
1583+
buffer_file.read_to_end(&mut newbuf).unwrap();
1584+
assert_eq!(newbuf, data);
1585+
1586+
// Call fs.write to write content to the file
1587+
buffer_file.seek(SeekFrom::Start(0)).unwrap();
1588+
let write_sz = fs
1589+
.write(
1590+
&ctx,
1591+
ino,
1592+
0,
1593+
&mut buffer_file,
1594+
data.len() as u32,
1595+
0,
1596+
None,
1597+
false,
1598+
0,
1599+
0,
1600+
)
1601+
.unwrap();
1602+
assert_eq!(write_sz, data.len());
1603+
1604+
// Create a new temp file as read buffer.
1605+
let read_buffer_file = TempFile::new().expect("Cannot create temporary file.");
1606+
let mut read_buffer_file = read_buffer_file.into_file();
1607+
let read_sz = fs
1608+
.read(
1609+
&ctx,
1610+
ino,
1611+
0,
1612+
&mut read_buffer_file,
1613+
data.len() as u32,
1614+
0,
1615+
None,
1616+
0,
1617+
)
1618+
.unwrap();
1619+
assert_eq!(read_sz, data.len());
1620+
1621+
read_buffer_file.seek(SeekFrom::Start(0)).unwrap();
1622+
let mut newbuf = Vec::new();
1623+
read_buffer_file.read_to_end(&mut newbuf).unwrap();
1624+
assert_eq!(newbuf, data);
1625+
}
14821626
}

src/passthrough/sync_io.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -665,7 +665,7 @@ impl<S: BitmapSlice + Send + Sync> FileSystem for PassthroughFs<S> {
665665
// so data.file won't be closed.
666666
let f = unsafe { File::from_raw_fd(data.borrow_fd().as_raw_fd()) };
667667

668-
self.check_fd_flags(data, f.as_raw_fd(), flags)?;
668+
self.check_fd_flags(data.clone(), f.as_raw_fd(), flags)?;
669669

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

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

695-
self.check_fd_flags(data, f.as_raw_fd(), flags)?;
695+
self.check_fd_flags(data.clone(), f.as_raw_fd(), flags)?;
696696

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

0 commit comments

Comments
 (0)