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.

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

Make mmap usage safe/sound #667

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
42 changes: 3 additions & 39 deletions src/symbolize/gimli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,15 @@

use self::gimli::read::EndianSlice;
use self::gimli::NativeEndian as Endian;
use self::mmap::Mmap;
use self::stash::Stash;
use super::BytesOrWideString;
use super::ResolveWhat;
use super::SymbolName;
use addr2line::gimli;
use core::convert::TryInto;
use core::mem;
use core::u32;
use libc::c_void;
use mystd::ffi::OsString;
use mystd::fs::File;
use mystd::path::Path;
use mystd::prelude::v1::*;

Expand All @@ -26,41 +23,14 @@ mod mystd {
#[cfg(not(backtrace_in_libstd))]
extern crate std as mystd;

cfg_if::cfg_if! {
if #[cfg(windows)] {
#[path = "gimli/mmap_windows.rs"]
mod mmap;
} else if #[cfg(target_vendor = "apple")] {
#[path = "gimli/mmap_unix.rs"]
mod mmap;
} else if #[cfg(any(
target_os = "android",
target_os = "freebsd",
target_os = "fuchsia",
target_os = "haiku",
target_os = "hurd",
target_os = "linux",
target_os = "openbsd",
target_os = "solaris",
target_os = "illumos",
target_os = "aix",
))] {
#[path = "gimli/mmap_unix.rs"]
mod mmap;
} else {
#[path = "gimli/mmap_fake.rs"]
mod mmap;
}
}

mod stash;

const MAPPINGS_CACHE_SIZE: usize = 4;

struct Mapping {
// 'static lifetime is a lie to hack around lack of support for self-referential structs.
cx: Context<'static>,
_map: Mmap,
_map: Vec<u8>,
stash: Stash,
}

Expand All @@ -74,7 +44,7 @@ impl Mapping {
/// Creates a `Mapping` by ensuring that the `data` specified is used to
/// create a `Context` and it can only borrow from that or the `Stash` of
/// decompressed sections or auxiliary data.
fn mk<F>(data: Mmap, mk: F) -> Option<Mapping>
fn mk<F>(data: Vec<u8>, mk: F) -> Option<Mapping>
where
F: for<'a> FnOnce(&'a [u8], &'a Stash) -> Option<Context<'a>>,
{
Expand All @@ -86,7 +56,7 @@ impl Mapping {

/// Creates a `Mapping` from `data`, or if the closure decides to, returns a
/// different mapping.
fn mk_or_other<F>(data: Mmap, mk: F) -> Option<Mapping>
fn mk_or_other<F>(data: Vec<u8>, mk: F) -> Option<Mapping>
where
F: for<'a> FnOnce(&'a [u8], &'a Stash) -> Option<Either<Mapping, Context<'a>>>,
{
Expand Down Expand Up @@ -184,12 +154,6 @@ impl<'data> Context<'data> {
}
}

fn mmap(path: &Path) -> Option<Mmap> {
let file = File::open(path).ok()?;
let len = file.metadata().ok()?.len().try_into().ok()?;
unsafe { Mmap::map(&file, len) }
}

cfg_if::cfg_if! {
if #[cfg(windows)] {
mod coff;
Expand Down
3 changes: 2 additions & 1 deletion src/symbolize/gimli/coff.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use super::mystd::fs;
use super::{gimli, Context, Endian, EndianSlice, Mapping, Path, Stash, Vec};
use alloc::sync::Arc;
use core::convert::TryFrom;
Expand All @@ -14,7 +15,7 @@ type Pe = object::pe::ImageNtHeaders64;

impl Mapping {
pub fn new(path: &Path) -> Option<Mapping> {
let map = super::mmap(path)?;
let map = fs::read(path).ok()?;
Mapping::mk(map, |data, stash| {
Context::new(stash, Object::parse(data)?, None, None)
})
Expand Down
10 changes: 5 additions & 5 deletions src/symbolize/gimli/elf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ type Elf = object::elf::FileHeader64<NativeEndian>;

impl Mapping {
pub fn new(path: &Path) -> Option<Mapping> {
let map = super::mmap(path)?;
let map = fs::read(path).ok()?;
Mapping::mk_or_other(map, |map, stash| {
let object = Object::parse(&map)?;

Expand All @@ -45,7 +45,7 @@ impl Mapping {

/// Load debuginfo from an external debug file.
fn new_debug(original_path: &Path, path: PathBuf, crc: Option<u32>) -> Option<Mapping> {
let map = super::mmap(&path)?;
let map = fs::read(&path).ok()?;
Mapping::mk(map, |map, stash| {
let object = Object::parse(&map)?;

Expand All @@ -56,7 +56,7 @@ impl Mapping {
// Try to locate a supplementary object file.
let mut sup = None;
if let Some((path_sup, build_id_sup)) = object.gnu_debugaltlink_path(&path) {
if let Some(map_sup) = super::mmap(&path_sup) {
if let Ok(map_sup) = fs::read(&path_sup) {
let map_sup = stash.cache_mmap(map_sup);
if let Some(sup_) = Object::parse(map_sup) {
if sup_.build_id() == Some(build_id_sup) {
Expand Down Expand Up @@ -84,7 +84,7 @@ impl Mapping {
})
.unwrap_or_else(|| "dwp".into());
path_dwp.set_extension(dwp_extension);
if let Some(map_dwp) = super::mmap(&path_dwp) {
if let Ok(map_dwp) = fs::read(&path_dwp) {
let map_dwp = stash.cache_mmap(map_dwp);
if let Some(dwp_) = Object::parse(map_dwp) {
return Some(dwp_);
Expand Down Expand Up @@ -473,7 +473,7 @@ pub(super) fn handle_split_dwarf<'data>(

path.push(convert_path(load.path.as_ref()?).ok()?);

if let Some(map_dwo) = super::mmap(&path) {
if let Ok(map_dwo) = fs::read(&path) {
let map_dwo = stash.cache_mmap(map_dwo);
if let Some(dwo) = Object::parse(map_dwo) {
return gimli::Dwarf::load(|id| -> Result<_, ()> {
Expand Down
5 changes: 3 additions & 2 deletions src/symbolize/gimli/libs_windows.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use super::super::super::windows_sys::*;
use super::mystd::fs;
use super::mystd::os::windows::prelude::*;
use super::{coff, mmap, Library, LibrarySegment, OsString};
use super::{coff, Library, LibrarySegment, OsString};
use alloc::vec;
use alloc::vec::Vec;
use core::mem;
Expand Down Expand Up @@ -75,7 +76,7 @@ unsafe fn load_library(me: &MODULEENTRY32W) -> Option<Library> {
//
// For now it appears that unlike ELF/MachO we can make do with one
// segment per library, using `modBaseSize` as the whole size.
let mmap = mmap(name.as_ref())?;
let mmap = fs::read(&name).ok()?;
let image_base = coff::get_image_base(&mmap)?;
let base_addr = me.modBaseAddr as usize;
Some(Library {
Expand Down
7 changes: 4 additions & 3 deletions src/symbolize/gimli/macho.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use super::mystd::fs;
use super::{gimli, Box, Context, Endian, EndianSlice, Mapping, Path, Stash, Vec};
use alloc::sync::Arc;
use core::convert::TryInto;
Expand All @@ -20,7 +21,7 @@ impl Mapping {
pub fn new(path: &Path) -> Option<Mapping> {
// First up we need to load the unique UUID which is stored in the macho
// header of the file we're reading, specified at `path`.
let map = super::mmap(path)?;
let map = fs::read(path).ok()?;
let (macho, data) = find_header(&map)?;
let endian = macho.endian().ok()?;
let uuid = macho.uuid(endian, data, 0).ok()?;
Expand Down Expand Up @@ -74,7 +75,7 @@ impl Mapping {
// information.
for entry in dir.read_dir().ok()? {
let entry = entry.ok()?;
let map = super::mmap(&entry.path())?;
let map = fs::read(&entry.path()).ok()?;
let candidate = Mapping::mk(map, |data, stash| {
let (macho, data) = find_header(data)?;
let endian = macho.endian().ok()?;
Expand Down Expand Up @@ -285,7 +286,7 @@ fn object_mapping(file: &object::read::ObjectMapFile<'_>) -> Option<Mapping> {
use super::mystd::ffi::OsStr;
use super::mystd::os::unix::prelude::*;

let map = super::mmap(Path::new(OsStr::from_bytes(file.path())))?;
let map = fs::read(Path::new(OsStr::from_bytes(file.path()))).ok()?;
let member_name = file.member();
Mapping::mk(map, |data, stash| {
let data = match member_name {
Expand Down
25 changes: 0 additions & 25 deletions src/symbolize/gimli/mmap_fake.rs

This file was deleted.

49 changes: 0 additions & 49 deletions src/symbolize/gimli/mmap_unix.rs

This file was deleted.

59 changes: 0 additions & 59 deletions src/symbolize/gimli/mmap_windows.rs

This file was deleted.

7 changes: 3 additions & 4 deletions src/symbolize/gimli/stash.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,14 @@
// only used on Linux right now, so allow dead code elsewhere
#![cfg_attr(not(target_os = "linux"), allow(dead_code))]

use super::Mmap;
use alloc::vec;
use alloc::vec::Vec;
use core::cell::UnsafeCell;

/// A simple arena allocator for byte buffers.
pub struct Stash {
buffers: UnsafeCell<Vec<Vec<u8>>>,
mmaps: UnsafeCell<Vec<Mmap>>,
mmaps: UnsafeCell<Vec<Vec<u8>>>,
}

impl Stash {
Expand All @@ -34,9 +33,9 @@ impl Stash {
&mut buffers[i]
}

/// Stores a `Mmap` for the lifetime of this `Stash`, returning a pointer
/// Stores a `Vec<u8>` for the lifetime of this `Stash`, returning a pointer
/// which is scoped to just this lifetime.
pub fn cache_mmap(&self, map: Mmap) -> &[u8] {
pub fn cache_mmap(&self, map: Vec<u8>) -> &[u8] {
// SAFETY: this is the only location for a mutable pointer to
// `mmaps`, and this structure isn't threadsafe to shared across
// threads either. We also never remove elements from `self.mmaps`,
Expand Down
5 changes: 3 additions & 2 deletions src/symbolize/gimli/xcoff.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use super::mystd::ffi::{OsStr, OsString};
use super::mystd::fs;
use super::mystd::os::unix::ffi::OsStrExt;
use super::mystd::str;
use super::{gimli, Context, Endian, EndianSlice, Mapping, Path, Stash, Vec};
Expand All @@ -18,7 +19,7 @@ type Xcoff = object::xcoff::FileHeader64;

impl Mapping {
pub fn new(path: &Path, member_name: &OsString) -> Option<Mapping> {
let map = super::mmap(path)?;
let map = fs::read(path).ok()?;
Mapping::mk(map, |data, stash| {
if member_name.is_empty() {
Context::new(stash, Object::parse(data)?, None, None)
Expand Down Expand Up @@ -80,7 +81,7 @@ pub fn parse_xcoff(data: &[u8]) -> Option<Image> {
}

pub fn parse_image(path: &Path, member_name: &OsString) -> Option<Image> {
let map = super::mmap(path)?;
let map = fs::read(path).ok()?;
let data = map.deref();
if member_name.is_empty() {
return parse_xcoff(data);
Expand Down
Loading