Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes undefined behavior due to improper use of &mut #54

Merged
merged 3 commits into from
Mar 19, 2024
Merged

Fixes undefined behavior due to improper use of &mut #54

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
759861a
Wrap the Cache in an UnsafeCell
sosthene-nitrokey Dec 5, 2023
04c38b2
Wrap FileAllocation Cache in an UnsafeCelll
sosthene-nitrokey Dec 6, 2023
76b8e62
Switched to accessing alloc.state using addr_of_mut! to preserve shar…
icmccorm Mar 9, 2024
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
93 changes: 66 additions & 27 deletions src/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
//! Experimental Filesystem version using closures.

use core::{cell::RefCell, cmp, mem, slice};

use bitflags::bitflags;
use core::ptr::addr_of;
use core::ptr::addr_of_mut;
use core::{
cell::{RefCell, UnsafeCell},
cmp, mem, slice,
};
use generic_array::typenum::marker_traits::Unsigned;
use littlefs2_sys as ll;
use serde::{Deserialize, Serialize};
Expand All @@ -18,10 +22,10 @@ use crate::{
};

struct Cache<Storage: driver::Storage> {
read: Bytes<Storage::CACHE_SIZE>,
write: Bytes<Storage::CACHE_SIZE>,
read: UnsafeCell<Bytes<Storage::CACHE_SIZE>>,
write: UnsafeCell<Bytes<Storage::CACHE_SIZE>>,
// lookahead: aligned::Aligned<aligned::A4, Bytes<Storage::LOOKAHEAD_SIZE>>,
lookahead: generic_array::GenericArray<u64, Storage::LOOKAHEAD_SIZE>,
lookahead: UnsafeCell<generic_array::GenericArray<u64, Storage::LOOKAHEAD_SIZE>>,
}

impl<S: driver::Storage> Cache<S> {
Expand Down Expand Up @@ -617,7 +621,7 @@ bitflags! {

/// The state of a `File`. Pre-allocate with `File::allocate`.
pub struct FileAllocation<S: driver::Storage> {
cache: Bytes<S::CACHE_SIZE>,
cache: UnsafeCell<Bytes<S::CACHE_SIZE>>,
state: ll::lfs_file_t,
config: ll::lfs_file_config,
}
Expand All @@ -637,7 +641,9 @@ impl<S: driver::Storage> FileAllocation<S> {
}

pub struct File<'a, 'b, S: driver::Storage> {
alloc: RefCell<&'b mut FileAllocation<S>>,
// We must store a raw pointer here since the FFI retains a copy of a pointer
// to the field alloc.state, so we cannot assert unique mutable access.
alloc: RefCell<*mut FileAllocation<S>>,
Comment on lines +644 to +646
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we wrap the state in an UnsafeCell instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you wrap the every state field (Allocation, FileAllocation ReadDirAllocation) in an UnsafeCell, Miri reports the following error:

---- Foreign Error Trace ----

@ %7 = load i32, ptr %6, align 4, !dbg !296

littlefs2-sys-0.1.7/littlefs/lfs.c:255:14
littlefs2-sys-0.1.7/littlefs/lfs.c:1892:13
littlefs/lfs.c:2774:15
littlefs2-sys-0.1.7/littlefs/lfs.c:2551:15
src/fs.rs:719:27: 722:10
-----------------------------

error: Undefined Behavior: read access through <511618> is forbidden
    |
    = help: the accessed tag <511618> is foreign to the protected tag <549720> (i.e., it is not a child)
    = help: this foreign read access would cause the protected tag <549720> (currently Active) to become Disabled
    = help: protected tags must never be Disabled
help: the accessed tag <511618> was created here
   --> src/fs.rs:828:9
    |
828 |         alloc: &'b mut FileAllocation<S>,
    |         ^^^^^
help: the protected tag <549720> was created here, in the initial state Reserved (interior mutable)
   --> src/fs.rs:718:25
    |
718 |     pub unsafe fn close(self) -> Result<()> {
    |                         ^^^^
    = help: the protected tag <549720> later transitioned to Active due to a child write access at offsets [0x20c..0x210]
    = help: this transition corresponds to the first write to a 2-phase borrowed mutable reference

The function ll::lfs_file_close mutates through the pointer it retains to the state field of FileAllocation and then reads from it. My modification to Miri does not record the location in foreign code where permissions were invalidated, but it seems like this happens in lfs_dir_commit, since you have multiple loops of the following form iterating over the linked list of files in lfs_t.

for (lfs_file_t *f = (lfs_file_t*)lfs->mlist; f; f = f->next) {
    ...
}

It seemed a bit strange to me at first why this would still be an error, since we have UnsafeCell. However, the docs for UnsafeCell state the following:

Note that only the immutability guarantee for shared references is affected by UnsafeCell. The uniqueness guarantee for mutable references is unaffected.

So, UnsafeCell only protects you from aliasing *mut if you are mutating exclusively through shared, immutable references. The pointer that the FFI retains in its linked list is derived from &'b mut FileAllocation<S>, and the pointer we are trying to read through in ll::lfs_file_close is derived from self, so UnsafeCell doesn't protect us here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! That makes some sense to me though the implications are still not totally clear.

fs: &'b Filesystem<'a, S>,
}

Expand Down Expand Up @@ -710,17 +716,23 @@ impl<'a, 'b, Storage: driver::Storage> File<'a, 'b, Storage> {
pub unsafe fn close(self) -> Result<()> {
let return_code = ll::lfs_file_close(
&mut self.fs.alloc.borrow_mut().state,
&mut self.alloc.borrow_mut().state,
// We need to use addr_of_mut! here instead of & mut since
// the FFI stores a copy of a pointer to the field state,
// so we cannot assert unique mutable access.
addr_of_mut!((*(*self.alloc.borrow_mut())).state),
);
io::result_from((), return_code)
}

/// Synchronize file contents to storage.
pub fn sync(&self) -> Result<()> {
let return_code = unsafe {
// We need to use addr_of_mut! here instead of & mut since
// the FFI stores a copy of a pointer to the field state,
// so we cannot assert unique mutable access.
ll::lfs_file_sync(
&mut self.fs.alloc.borrow_mut().state,
&mut self.alloc.borrow_mut().state,
addr_of_mut!((*(*self.alloc.borrow_mut())).state),
)
};
io::result_from((), return_code)
Expand All @@ -729,9 +741,12 @@ impl<'a, 'b, Storage: driver::Storage> File<'a, 'b, Storage> {
/// Size of the file in bytes.
pub fn len(&self) -> Result<usize> {
let return_code = unsafe {
// We need to use addr_of_mut! here instead of & mut since
// the FFI stores a copy of a pointer to the field state,
// so we cannot assert unique mutable access.
ll::lfs_file_size(
&mut self.fs.alloc.borrow_mut().state,
&mut self.alloc.borrow_mut().state,
addr_of_mut!((*(*self.alloc.borrow_mut())).state),
)
};
io::result_from(return_code as usize, return_code)
Expand All @@ -748,9 +763,12 @@ impl<'a, 'b, Storage: driver::Storage> File<'a, 'b, Storage> {
/// of the intermediate data filled in with 0s.
pub fn set_len(&self, size: usize) -> Result<()> {
let return_code = unsafe {
// We need to use addr_of_mut! here instead of & mut since
// the FFI stores a copy of a pointer to the field state,
// so we cannot assert unique mutable access.
ll::lfs_file_truncate(
&mut self.fs.alloc.borrow_mut().state,
&mut self.alloc.borrow_mut().state,
addr_of_mut!((*(*self.alloc.borrow_mut())).state),
size as u32,
)
};
Expand Down Expand Up @@ -817,17 +835,19 @@ impl OpenOptions {
pub unsafe fn open<'a, 'b, S: driver::Storage>(
&self,
fs: &'b Filesystem<'a, S>,
alloc: &'b mut FileAllocation<S>,
alloc: &mut FileAllocation<S>,
path: &Path,
) -> Result<File<'a, 'b, S>> {
alloc.config.buffer = &mut alloc.cache as *mut _ as *mut cty::c_void;

alloc.config.buffer = alloc.cache.get() as *mut _;
// We need to use addr_of_mut! here instead of & mut since
// the FFI stores a copy of a pointer to the field state,
// so we cannot assert unique mutable access.
let return_code = ll::lfs_file_opencfg(
&mut fs.alloc.borrow_mut().state,
&mut alloc.state,
addr_of_mut!(alloc.state),
path.as_ptr(),
self.0.bits() as i32,
&alloc.config,
addr_of!(alloc.config),
);

let file = File {
Expand Down Expand Up @@ -920,9 +940,12 @@ impl OpenOptions {
impl<S: driver::Storage> io::Read for File<'_, '_, S> {
fn read(&self, buf: &mut [u8]) -> Result<usize> {
let return_code = unsafe {
// We need to use addr_of_mut! here instead of & mut since
// the FFI stores a copy of a pointer to the field state,
// so we cannot assert unique mutable access.
ll::lfs_file_read(
&mut self.fs.alloc.borrow_mut().state,
&mut self.alloc.borrow_mut().state,
addr_of_mut!((*(*self.alloc.borrow_mut())).state),
buf.as_mut_ptr() as *mut cty::c_void,
buf.len() as u32,
)
Expand All @@ -934,9 +957,12 @@ impl<S: driver::Storage> io::Read for File<'_, '_, S> {
impl<S: driver::Storage> io::Seek for File<'_, '_, S> {
fn seek(&self, pos: io::SeekFrom) -> Result<usize> {
let return_code = unsafe {
// We need to use addr_of_mut! here instead of & mut since
// the FFI stores a copy of a pointer to the field state,
// so we cannot assert unique mutable access.
ll::lfs_file_seek(
&mut self.fs.alloc.borrow_mut().state,
&mut self.alloc.borrow_mut().state,
addr_of_mut!((*(*self.alloc.borrow_mut())).state),
pos.off(),
pos.whence(),
)
Expand All @@ -948,9 +974,12 @@ impl<S: driver::Storage> io::Seek for File<'_, '_, S> {
impl<S: driver::Storage> io::Write for File<'_, '_, S> {
fn write(&self, buf: &[u8]) -> Result<usize> {
let return_code = unsafe {
// We need to use addr_of_mut! here instead of & mut since
// the FFI stores a copy of a pointer to the field state,
// so we cannot assert unique mutable access.
ll::lfs_file_write(
&mut self.fs.alloc.borrow_mut().state,
&mut self.alloc.borrow_mut().state,
addr_of_mut!((*(*self.alloc.borrow_mut())).state),
buf.as_ptr() as *const cty::c_void,
buf.len() as u32,
)
Expand Down Expand Up @@ -1024,7 +1053,9 @@ impl ReadDirAllocation {
}

pub struct ReadDir<'a, 'b, S: driver::Storage> {
alloc: RefCell<&'b mut ReadDirAllocation>,
// We must store a raw pointer here since the FFI retains a copy of a pointer
// to the field alloc.state, so we cannot assert unique mutable access.
alloc: RefCell<*mut ReadDirAllocation>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, why can't ReadDirAllocation be stored in an UnsafeCell instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm understanding this right, changing RefCell<&'b mut ReadDirAllocation> to RefCell<UnsafeCell<ReadDirAllocation>> would require changing the API for Filesystem::new and ReadDir::new, since there's no way of converting &mut T into UnsafeCell<T> without cloning or copying T.

fn new(alloc: &'a mut Allocation<Storage>, storage: &'a mut Storage) -> Self {
        ...
        Filesystem {
            alloc: RefCell::new(UnsafeCell::new(???)),
            storage,
        }
}

I'll try creating a minimal example to see if there another issue that appears when you use this pattern...

Copy link
Contributor

Choose a reason for hiding this comment

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

Right that makes sense. On the other hand I think we could be open to making a new major release if that is required.
But going without breaking change is always better.

fs: &'b Filesystem<'a, S>,
#[cfg(feature = "dir-entry-path")]
path: PathBuf,
Expand All @@ -1037,11 +1068,13 @@ impl<'a, 'b, S: driver::Storage> Iterator for ReadDir<'a, 'b, S> {
#[allow(unreachable_code)]
fn next(&mut self) -> Option<Self::Item> {
let mut info: ll::lfs_info = unsafe { mem::MaybeUninit::zeroed().assume_init() };

// We need to use addr_of_mut! here instead of & mut since
// the FFI stores a copy of a pointer to the field state,
// so we cannot assert unique mutable access.
let return_code = unsafe {
ll::lfs_dir_read(
&mut self.fs.alloc.borrow_mut().state,
&mut self.alloc.borrow_mut().state,
addr_of_mut!((*(*self.alloc.borrow_mut())).state),
&mut info,
)
};
Expand Down Expand Up @@ -1088,9 +1121,12 @@ impl<S: driver::Storage> ReadDir<'_, '_, S> {
// as long as ReadDir is not Copy.
pub fn close(self) -> Result<()> {
let return_code = unsafe {
// We need to use addr_of_mut! here instead of & mut since
// the FFI stores a copy of a pointer to the field state,
// so we cannot assert unique mutable access.
ll::lfs_dir_close(
&mut self.fs.alloc.borrow_mut().state,
&mut self.alloc.borrow_mut().state,
addr_of_mut!((*(*self.alloc.borrow_mut())).state),
)
};
io::result_from((), return_code)
Expand Down Expand Up @@ -1120,9 +1156,12 @@ impl<'a, Storage: driver::Storage> Filesystem<'a, Storage> {
alloc: &'b mut ReadDirAllocation,
path: &Path,
) -> Result<ReadDir<'a, 'b, Storage>> {
// ll::lfs_dir_open stores a copy of the pointer to alloc.state, so
// we must use addr_of_mut! here, since &mut alloc.state asserts unique
// mutable access, and we need shared mutable access.
let return_code = ll::lfs_dir_open(
&mut self.alloc.borrow_mut().state,
&mut alloc.state,
addr_of_mut!(alloc.state),
path.as_ptr(),
);

Expand Down Expand Up @@ -1150,9 +1189,9 @@ impl<'a, Storage: driver::Storage> Filesystem<'a, Storage> {
fn new(alloc: &'a mut Allocation<Storage>, storage: &'a mut Storage) -> Self {
alloc.config.context = storage as *mut _ as *mut cty::c_void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't that have the same issue as the state being shared in the FileAlloc?
A mutable reference to the Storage is also stored in the Filesystem struct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah—this should be the same issue. The current test suite doesn't reach a path where this results in UB, but copying &mut should always be avoided.


alloc.config.read_buffer = &mut alloc.cache.read as *mut _ as *mut cty::c_void;
alloc.config.prog_buffer = &mut alloc.cache.write as *mut _ as *mut cty::c_void;
alloc.config.lookahead_buffer = &mut alloc.cache.lookahead as *mut _ as *mut cty::c_void;
alloc.config.read_buffer = alloc.cache.read.get() as *mut cty::c_void;
alloc.config.prog_buffer = alloc.cache.write.get() as *mut cty::c_void;
alloc.config.lookahead_buffer = alloc.cache.lookahead.get() as *mut cty::c_void;

Filesystem {
alloc: RefCell::new(alloc),
Expand Down
Loading