Skip to content

Commit

Permalink
object: Improve correctness
Browse files Browse the repository at this point in the history
Due to the fact that `object::File` takes a reference to the mmap-ed
file, we needed to do some awful lifetime hacks to fit it in the API we
wanted. While getting rid of the raw pointer and drop impl, Ricardo
noticed that there was some lurking UB too, so this fixes it as well.

Co-authored-by: Ricardo Delfin <[email protected]>

Test Plan
=========

CI
  • Loading branch information
javierhonduco committed Nov 4, 2024
1 parent d5a5da1 commit 33390b0
Showing 1 changed file with 13 additions and 16 deletions.
29 changes: 13 additions & 16 deletions lightswitch-object/src/object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,33 +35,30 @@ pub struct ElfLoad {

#[derive(Debug)]
pub struct ObjectFile<'a> {
leaked_mmap_ptr: *const Mmap,
/// Warning! `object` must always go above `mmap` to ensure it will be dropped
/// before. Rust guarantees that fields are dropped in the order they are defined.
object: object::File<'a>,
mmap: Box<Mmap>,
code_hash: Digest,
}

impl Drop for ObjectFile<'_> {
fn drop(&mut self) {
unsafe {
let _to_free = Box::from_raw(self.leaked_mmap_ptr as *mut Mmap);
}
}
}

impl ObjectFile<'_> {
pub fn new(path: &Path) -> Result<Self> {
let file = fs::File::open(path)?;
let mmap = unsafe { Mmap::map(&file) }?;
let mmap = Box::new(mmap);
let leaked = Box::leak(mmap);
let object = object::File::parse(&**leaked)?;
// Rust offers no guarantees on whether a "move" is done virtually or by memcpying,
// so to ensure that the memory value is valid we store it in the heap.
let mmap = Box::new(unsafe { Mmap::map(&file) }?);
let object = object::File::parse(&**mmap)?;
// Safety: The lifetime of `object` will outlive `mmap`'s. We ensure `mmap` lives as long as
// `object` by defining `object` before.
let object =
unsafe { std::mem::transmute::<object::File<'_>, object::File<'static>>(object) };
let Some(code_hash) = code_hash(&object) else {
return Err(anyhow!("code hash is None"));
};

Ok(ObjectFile {
leaked_mmap_ptr: leaked as *const Mmap,
object,
mmap,
code_hash,
})
}
Expand Down Expand Up @@ -115,7 +112,7 @@ impl ObjectFile<'_> {
}

pub fn elf_load_segments(&self) -> Result<Vec<ElfLoad>> {
let mmap = unsafe { &**self.leaked_mmap_ptr };
let mmap = &**self.mmap;

match FileKind::parse(mmap) {
Ok(FileKind::Elf32) => {
Expand Down

0 comments on commit 33390b0

Please sign in to comment.