Fixes undefined behavior due to improper use of &mut

[icmccorm]

These commits fix multiple instances of undefined behavior as originally discussed via email with @sosthene-nitrokey.

Issue 1 - Allocation is self-referential

The struct Allocation is initialized so that its config retains multiple references to the cache.

alloc.config.buffer = & mut alloc.cache *mut _ as *mut cty::c_void;

When this reference or any other reference to one of the caches within alloc is mutated in C, it will invalidate all other mutable references to alloc. Wrapping each cache in an UnsafeCell prevents this from occurring, since foreign writes do not affect the validity of a permission to a location if that location is covered by an UnsafeCell.

Issue 2 - Linked lists retain mutable pointers

The structure of the C API uses multiple linked lists to contain objects of type lfs_file and lfs_dir. For example:

pub struct lfs_file {
    pub next: *mut lfs_file
    ...
} 

This means that it is unsound to mutably borrow these structs in Rust, since we cannot assume unique mutable access. Using addr_of_mut! instead of &mut ensures that we never assume that we have unique access to alloc.state.

With these changes, all tests pass Miri under the current version of Tree Borrows.

[sosthene-nitrokey]

Looks good!

Thank you for the report and for the fixes.

This looks good. There are a couple of parts I'm not fully sure I understand, but otherwise this seems good!

[sosthene-nitrokey]

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

[icmccorm]

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.

[sosthene-nitrokey]

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

[sosthene-nitrokey]

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

[icmccorm]

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...

[sosthene-nitrokey]

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.

[sosthene-nitrokey]

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.

[icmccorm]

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.