-
-
Notifications
You must be signed in to change notification settings - Fork 27
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.
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
[asan] SEGV when using the ReadDir
API
#3
Comments
I tried to fix the UB in the issue description example by having I have done some more digging and have confirmed that not closing files can also cause UB. The following snippet which only uses the fn main() {
let mut fs = Filesystem::mount(&mut fsa, &mut storage).unwrap();
foo(&mut fs, &mut storage);
bar(&mut fs, &mut storage);
}
#[inline(never)]
fn foo<S>(fs: &mut Filesystem<S>, storage: &mut S) {
let mut fa = File::allocate();
forget(File::create("a.txt", &mut fa, fs, storage).unwrap());
}
#[inline(never)]
fn bar<S>(fs: &mut Filesystem<S>, storage: &mut S) {
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(); // program hangs here
} AFAICT, when you open a file littlefs uses the FileAllocation as a linked list node and appends the open file to a linked list of files (and directories?) it keeps track of. When you close the file littlefs removes the file from its linked list. If you don't close the file and free or move the FileAllocation that causes the linked list node, and the whole list, to become corrupted which leads to UB. |
ReadDir
API appears to be unsoundReadDir
API
I have updated the issue description to include a repro test case that you can run on x86_64 Linux and triggers a SIGSEV when run under asan (LLVM's AddressSanitizer). I have opened a separate ticket (#5) for the File.close soundness hole; that one also contains a repro test case that triggers a SIGSEV under asan. |
Update: I have added another repro case that's not caught be asan (it exits with return code 0 = sucess), but segfaults when run without instrumentation. The UB is, however, caught by msan.
And I think I now see why that was not enough to fix the UB. It seems that the UB is in the pub fn read_dir<P: Into<Path<Storage>>>(
&mut self,
path: P,
storage: &mut Storage,
) ->
Result<ReadDir<Storage>>
{
self.alloc.config.context = storage as *mut _ as *mut cty::c_void;
let mut read_dir = ReadDir {
state: unsafe { mem::MaybeUninit::zeroed().assume_init() },
_storage: PhantomData,
};
let return_code = unsafe {
ll::lfs_dir_open(
&mut self.alloc.state,
&mut read_dir.state,
&path.into() as *const _ as *const cty::c_char,
)
};
Error::result_from(return_code).map(|_| read_dir)
}
Fixing this one seems like it's going to require adding a |
I've started to fix this: https://github.com/nickray/littlefs2/pull/6 I think the UB issues can be summed up with: Use closures to do cleanup (closing files/readdirs) with error reporting. Will test against my use cases. One thing I'd like but can't currently is directly read the files that are being iterated over in a ReadDir. |
@japaric I believe these issues are all prevented/solved in https://github.com/nickray/littlefs2/pull/6.
I don't know of any documentation on what's allowed :/ I operate under the same mental model of a linked list of open file or directory objects. Personally, I at minimum need to read files during iteration, to filter based on contents. It would also be nice to delete files in a directory based on some condition on the file... Note that littlefs has more operations on directories than Rust-style My plan if anything turns up that must be prevented is to have I can imagine that |
I opened to https://github.com/nickray/littlefs2/issues/7 re. ReadDir. I think/hope this issue can be closed in favor of the former, pending merge of https://github.com/nickray/littlefs2/pull/6. |
UPDATE: Added Steps To Reproduce (STR) on x86_64
STR1
asan:
fs
andFile
clashSTR2
msan:
fs
clashes with itselfOriginal report
Given this directory structure:
I'm observing the following pseudo-code hang:
And a slightly different program causes the device to hard fault:
If I'm reading
littlefs-project/littlefs#164 (comment) correctly
not closing either a file or a directory can result in UB.
In the case of
ReadDir
, thelfs_dir_create
C function is called but itscounterpart,
lfs_dir_close
, is never called.Because
mem::forget
(and other ways of leaking memory) are considered "safe"(don't require
unsafe
to call), callinglfs_dir_close
fromReadDir
'sdestructor is not going to be sufficient to fix this soundness hole. See below:
I think the API would need to become closure-based to ensure
lfs_dir_close
isalways called. Something like this:
If the same issue (forget to close == UB) applies to
File
s then aclosure-based API would also be needed there to avoid UB in safe code.
As an additional note: is one allowed to modify the directory structure (e.g.
call
Filesystem::remove
) while iterating it withread_dir
? It's possible tooverlap these operations with the current
ReadDir
API but the overlap of thoseactions could be rejected at compile time if
ReadDir
mutably borrowedFilesystem
. I couldn't find an answer to this question myself -- where islittlefs API documented? One of the header files contains what appear to be doc
comments but those don't describe what one is allowed to or not to do with the
API.
The text was updated successfully, but these errors were encountered: