Skip to content

Commit

Permalink
Use Layout::from_size_align for aligned page calculation.
Browse files Browse the repository at this point in the history
  • Loading branch information
Pavel-Durov committed Apr 14, 2024
1 parent 6f4bdc3 commit 8e00241
Showing 1 changed file with 29 additions and 22 deletions.
51 changes: 29 additions & 22 deletions ykrt/src/trace/swt/patch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,10 @@
//! execution flow of functions at runtime. Improper use can lead to
//! undefined behaviour, memory corruption, or crashes.
use libc::{mprotect, size_t, sysconf, PROT_EXEC, PROT_READ, PROT_WRITE};
use std::mem;
use std::{ffi::c_void, sync::Once};

use crate::trace::swt::yk_trace_basicblock;
use libc::{mprotect, size_t, sysconf, PROT_EXEC, PROT_READ, PROT_WRITE, _SC_PAGESIZE};
use std::alloc::Layout;
use std::{ffi::c_void, sync::Once};

// This is used to ensure that the original instructions are only saved once.
static ORIGINAL_INSTRUCTIONS_INIT: Once = Once::new();
Expand Down Expand Up @@ -61,24 +60,32 @@ unsafe fn save_original_instructions(
/// * `size` - A size_t indicating the number of bytes to copy from `code`.
///
unsafe fn patch_function(function_ptr: usize, code: *const u8, size: size_t) {
let page_size = sysconf(libc::_SC_PAGESIZE) as usize;

let func_address = ((function_ptr as usize) & !(page_size - 1)) as *mut c_void;
let page_size_aligned = (((function_ptr as usize) + mem::size_of_val(&function_ptr))
- (func_address as usize)) as usize;

// Set function memory region to be writable
let result = mprotect(func_address, page_size_aligned, PROT_READ | PROT_WRITE);
if result != 0 {
panic!("Failed to change memory protection to be writable");
}

// Set function memory region back to be non-writable
std::ptr::copy_nonoverlapping(code, function_ptr as *mut u8, size);

let result = mprotect(func_address, page_size_aligned, PROT_READ | PROT_EXEC);
if result != 0 {
panic!("Failed to change memory protection to not writable");
let page_size = sysconf(_SC_PAGESIZE) as usize;

let page_address = (function_ptr & !(page_size - 1)) as *mut c_void;
let start_offset = function_ptr - (page_address as usize) + size;

This comment has been minimized.

Copy link
@ltratt

ltratt Apr 14, 2024

Contributor

Doesn't the +size make this the end_offset?

This comment has been minimized.

Copy link
@Pavel-Durov

Pavel-Durov Apr 18, 2024

Author Contributor

I think you're right :no_mouth
removed πŸ‘‰ 18b7fee

This comment has been minimized.

Copy link
@Pavel-Durov

Pavel-Durov Apr 18, 2024

Author Contributor

Added a few comments πŸ‘‰ e1be48c


match Layout::from_size_align(start_offset, page_size) {

This comment has been minimized.

Copy link
@ltratt

ltratt Apr 14, 2024

Contributor

It's fine to do Layout::from_size_align(start_offset, page_size).unwrap() provided you add a comment explaining why you believe the unwrap can't fail. Indeed, it's better to do that than the panic below, because this takes up way more screen space. I also generally suggest avoiding a generic message in the panic (i.e. unless the message contains some dynamic information I don't see the point): people can look up a line number to see what's gone wrong.

This comment has been minimized.

Copy link
@Pavel-Durov

Pavel-Durov Apr 18, 2024

Author Contributor

updated πŸ‘‰ ada68bb

Ok(layout) => {
// Set function memory page as writable
let result = mprotect(page_address, layout.size(), PROT_READ | PROT_WRITE);
if result != 0 {
panic!("Failed to change memory protection to be writable");

This comment has been minimized.

Copy link
@ltratt

ltratt Apr 14, 2024

Contributor

We could recover from this error.

This comment has been minimized.

Copy link
@Pavel-Durov

Pavel-Durov Apr 18, 2024

Author Contributor

updated πŸ‘‰ 3e7d023

This comment has been minimized.

Copy link
@Pavel-Durov

Pavel-Durov Apr 18, 2024

Author Contributor

By recover do you mean just ignore this error and try to continue the execution?

}
// Copy the new code over
std::ptr::copy_nonoverlapping(code, function_ptr as *mut u8, size);
// Set function memory page as readable
let result = mprotect(page_address, layout.size(), PROT_READ | PROT_EXEC);

This comment has been minimized.

Copy link
@ltratt

ltratt Apr 14, 2024

Contributor

Since we don't use result I'd suggest inlining if mprotect(...) != 0 { ... }. In this case, if mprotect does fail, we are screwed, but if the mprotect above fails, we could continue.

This comment has been minimized.

Copy link
@Pavel-Durov

Pavel-Durov Apr 18, 2024

Author Contributor

updated πŸ‘‰ 3e7d023

if result != 0 {
panic!("Failed to change memory protection back to executable");
}
}
Err(e) => {
panic!(
"Failed to create layout for fuction instuction patch: {:?}",
e
);
}
}
}

Expand Down

0 comments on commit 8e00241

Please sign in to comment.