[asan] SEGV when you forget to close a File

[japaric]

Originally reported in #3. This ticket includes an example that can be run on x86_64 and instrumented with LLVM AddressSanitizer.

STR

use core::mem;

use littlefs2::{
    consts, driver,
    fs::{File, Filesystem},
    io::{Result, Write},
    ram_storage,
};

ram_storage!(
    name=RamStorage,
    backend=Ram,
    trait=driver::Storage,
    erase_value=0xff,
    read_size=20*5,
    write_size=20*7,
    cache_size_ty=consts::U700,
    block_size=20*35,
    block_count=32,
    lookaheadwords_size_ty=consts::U1,
    filename_max_plus_one_ty=consts::U256,
    path_max_plus_one_ty=consts::U256,
    result=Result,
);

fn main() {
    let mut ram = Ram::default();
    let mut storage = RamStorage::new(&mut ram);
    let mut alloc = Filesystem::allocate();
    Filesystem::format(&mut storage).unwrap();
    let mut fs = Filesystem::mount(&mut alloc, &mut storage).unwrap();

    foo(&mut fs, &mut storage);
    bar(&mut fs, &mut storage);
}

#[inline(never)]
fn foo<'r>(fs: &mut Filesystem<RamStorage<'r>>, storage: &mut RamStorage<'r>) {
    let mut fa = File::allocate();
    let f = File::create("a.txt", &mut fa, fs, storage).unwrap();
    // f.close(fs, storage).unwrap(); // no SEGV if uncommented
}

#[inline(never)]
fn bar<'r>(fs: &mut Filesystem<RamStorage<'r>>, storage: &mut RamStorage<'r>) {
    let mut fa = File::allocate();
    let mut f = File::create("b.txt", &mut fa, fs, storage).unwrap();
    f.write(fs, storage, b"Hello").unwrap();
    f.close(fs, storage).unwrap();
}

$ # NOTE requires a nightly compiler
$ RUSTFLAGS='-Z sanitizer=address' cargo run --target x86_64-unknown-linux-gnu
AddressSanitizer:DEADLYSIGNAL
=================================================================
==13882==ERROR: AddressSanitizer: SEGV on unknown address 0xffffffff0001000e (pc 0x56047f586f75 bp 0x7fffa0fecc00 sp 0x7fffa0fecc00 T0)
==13882==The signal is caused by a READ memory access.
    #0 0x56047f586f74 in lfs_pair_cmp
    #1 0x56047f58a14d in lfs_dir_commit
    #2 0x56047f58b51c in lfs_file_opencfg
    #3 0x56047f5692df in littlefs2::fs::OpenOptions::open::h7c50f9aa4cc22d9d
    #4 0x56047f56a373 in littlefs2::fs::File$LT$S$GT$::create::he98744fff0b12517
    #5 0x56047f56e2a4 in hello::bar::hd52d709f8193650b main.rs:47:16
    #6 0x56047f56de7a in hello::main::ha1cfdc381277c04c main.rs:34:4

[japaric]

A possible fix that doesn't rely on destructors running (you cannot rely on that because mem::forget is safe): require that the &mut FileAllocation stored in the File has a static lifetime. This ensures that even if the File is mem::forget-ten the FileAllocation won't be deallocated. Thus even if you don't close the file, the linked list in lfs / Filesystem will still point into valid memory; if the File is forgotten then you'll end with a node that will never be removed from the lfs linked list, which is not great but it's also not UB.

Here's the repro case using static lifetimes:

use core::mem;

use littlefs2::{
    consts, driver,
    fs::{File, Filesystem},
    io::{Result, Write},
    ram_storage,
};

ram_storage!(
    name=RamStorage,
    backend=Ram,
    trait=driver::Storage,
    erase_value=0xff,
    read_size=20*5,
    write_size=20*7,
    cache_size_ty=consts::U700,
    block_size=20*35,
    block_count=32,
    lookaheadwords_size_ty=consts::U1,
    filename_max_plus_one_ty=consts::U256,
    path_max_plus_one_ty=consts::U256,
    result=Result,
);

fn main() {
    let mut ram: &'static mut _ = Box::leak(Box::new(Ram::default()));
    let mut storage = RamStorage::new(ram);
    let mut alloc = Filesystem::allocate();
    Filesystem::format(&mut storage).unwrap();
    let mut fs = Filesystem::mount(&mut alloc, &mut storage).unwrap();

    foo(&mut fs, &mut storage);
    bar(&mut fs, &mut storage);

    println!("OK");
}

#[inline(never)]
fn foo(fs: &mut Filesystem<RamStorage<'static>>, storage: &mut RamStorage<'static>) {
    let mut fa: &'static mut _ = Box::leak(Box::new(File::allocate()));
    let f = File::create("a.txt", &mut fa, fs, storage).unwrap();
    mem::forget(f); // this is now OK
}

#[inline(never)]
fn bar<'r>(fs: &mut Filesystem<RamStorage<'r>>, storage: &mut RamStorage<'r>) {
    let mut fa = File::allocate();
    let mut f = File::create("b.txt", &mut fa, fs, storage).unwrap();
    f.write(fs, storage, b"Hello").unwrap();
    f.close(fs, storage).unwrap();
}

With this asan errors with a memory leak, which is OK in safe Rust.

$ RUSTFLAGS='-Z sanitizer=address' cargo r --target x86_64-unknown-linux-gnu
OK

=================================================================
==31737==ERROR: LeakSanitizer: detected memory leaks
(..)
SUMMARY: AddressSanitizer: 23232 byte(s) leaked in 2 allocation(s).

Instead of &'static mut T one could also use alloc:Box or heapless::pool::Box to patch the soundness hole.

[nickray]

Same comment as for https://github.com/nickray/littlefs2/issues/3:

• user can no longer safely construct a File
• the ..._and_then API should fix the UB raised