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

RangeDetails: don't return dangling pointers #121

Merged
merged 2 commits into from
Jan 4, 2024
Merged
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
2 changes: 2 additions & 0 deletions frida-gum/src/memory_range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ pub struct ScanResult {
pub address: usize,
pub size: usize,
}

#[derive(Clone)]
pub struct MemoryRange {
pub(crate) memory_range: gum_sys::GumMemoryRange,
}
Expand Down
56 changes: 34 additions & 22 deletions frida-gum/src/range_details.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use {
use alloc::boxed::Box;

/// The memory protection of an unassociated page.
#[derive(FromPrimitive)]
#[derive(Clone, FromPrimitive)]
#[repr(u32)]
pub enum PageProtection {
NoAccess = gum_sys::_GumPageProtection_GUM_PAGE_NO_ACCESS as u32,
Expand All @@ -40,32 +40,43 @@ pub enum PageProtection {
}

/// The file association to a page.
#[derive(Clone)]
pub struct FileMapping<'a> {
file_mapping: *const gum_sys::GumFileMapping,
path: &'a str,
size: usize,
offset: u64,
phantom: PhantomData<&'a gum_sys::GumFileMapping>,
}

impl<'a> FileMapping<'a> {
pub(crate) fn from_raw(file: *const gum_sys::GumFileMapping) -> Self {
Self {
file_mapping: file,
phantom: PhantomData,
pub(crate) fn from_raw(file: *const gum_sys::GumFileMapping) -> Option<Self> {
if file.is_null() {
None
} else {
Some(unsafe {
Self {
path: CStr::from_ptr((*file).path).to_str().unwrap(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will the path variable live long enough/not be freed at some point as well? Maybe a clone() here or even to_string_lossy would be safer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@domenukk To my eyes, the MemoryRange does not need special treatment, as its from_raw dereferences the pointer into a copy of the object. Correct?

size: (*file).size as usize,
offset: (*file).offset,
phantom: PhantomData,
}
})
}
}

/// The path of the file mapping on disk.
pub fn path(&self) -> &str {
unsafe { CStr::from_ptr((*self.file_mapping).path).to_str().unwrap() }
self.path
}

/// The offset into the file mapping.
pub fn offset(&self) -> u64 {
unsafe { (*self.file_mapping).offset }
self.offset
}

/// The size of the mapping.
pub fn size(&self) -> u64 {
unsafe { (*self.file_mapping).size as u64 }
pub fn size(&self) -> usize {
self.size as usize
}
}

Expand Down Expand Up @@ -104,15 +115,21 @@ unsafe extern "C" fn enumerate_ranges_stub(

/// Details a range of virtual memory.
pub struct RangeDetails<'a> {
range_details: *const gum_sys::GumRangeDetails,
range: MemoryRange,
protection: PageProtection,
file: Option<FileMapping<'a>>,
phantom: PhantomData<&'a gum_sys::GumRangeDetails>,
}

impl<'a> RangeDetails<'a> {
pub(crate) fn from_raw(range_details: *const gum_sys::GumRangeDetails) -> Self {
Self {
range_details,
phantom: PhantomData,
unsafe {
Self {
range: MemoryRange::from_raw((*range_details).range),
protection: num::FromPrimitive::from_u32((*range_details).protection).unwrap(),
file: FileMapping::from_raw((*range_details).file),
phantom: PhantomData,
}
}
}

Expand Down Expand Up @@ -154,21 +171,16 @@ impl<'a> RangeDetails<'a> {

/// The range of memory that is detailed.
pub fn memory_range(&self) -> MemoryRange {
unsafe { MemoryRange::from_raw((*self.range_details).range) }
self.range.clone()
}

/// The page protection of the range.
pub fn protection(&self) -> PageProtection {
let protection = unsafe { (*self.range_details).protection };
num::FromPrimitive::from_u32(protection).unwrap()
self.protection.clone()
}

/// The associated file mapping, if present.
pub fn file_mapping(&self) -> Option<FileMapping> {
if self.range_details.is_null() {
None
} else {
Some(unsafe { FileMapping::from_raw((*self.range_details).file) })
}
self.file.clone()
}
}
Loading