-
Notifications
You must be signed in to change notification settings - Fork 386
Implement file cloning on Windows #4344
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ use crate::*; | |
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] | ||
pub enum PseudoHandle { | ||
CurrentThread, | ||
CurrentProcess, | ||
} | ||
|
||
/// Miri representation of a Windows `HANDLE` | ||
|
@@ -23,16 +24,19 @@ pub enum Handle { | |
|
||
impl PseudoHandle { | ||
const CURRENT_THREAD_VALUE: u32 = 0; | ||
const CURRENT_PROCESS_VALUE: u32 = 1; | ||
|
||
fn value(self) -> u32 { | ||
match self { | ||
Self::CurrentThread => Self::CURRENT_THREAD_VALUE, | ||
Self::CurrentProcess => Self::CURRENT_PROCESS_VALUE, | ||
} | ||
} | ||
|
||
fn from_value(value: u32) -> Option<Self> { | ||
match value { | ||
Self::CURRENT_THREAD_VALUE => Some(Self::CurrentThread), | ||
Self::CURRENT_PROCESS_VALUE => Some(Self::CurrentProcess), | ||
_ => None, | ||
} | ||
} | ||
|
@@ -244,6 +248,76 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { | |
interp_ok(handle.to_scalar(this)) | ||
} | ||
|
||
fn DuplicateHandle( | ||
&mut self, | ||
src_proc: &OpTy<'tcx>, // HANDLE | ||
src_handle: &OpTy<'tcx>, // HANDLE | ||
target_proc: &OpTy<'tcx>, // HANDLE | ||
target_handle: &OpTy<'tcx>, // LPHANDLE | ||
desired_access: &OpTy<'tcx>, // DWORD | ||
inherit: &OpTy<'tcx>, // BOOL | ||
options: &OpTy<'tcx>, // DWORD | ||
) -> InterpResult<'tcx, Scalar> { | ||
// ^ Returns BOOL (i32 on Windows) | ||
let this = self.eval_context_mut(); | ||
|
||
let src_proc = this.read_handle(src_proc, "DuplicateHandle")?; | ||
let src_handle = this.read_handle(src_handle, "DuplicateHandle")?; | ||
let target_proc = this.read_handle(target_proc, "DuplicateHandle")?; | ||
let target_handle_ptr = this.read_pointer(target_handle)?; | ||
// Since we only support DUPLICATE_SAME_ACCESS, this value is ignored, but should be valid | ||
let _ = this.read_scalar(desired_access)?.to_u32()?; | ||
// We don't support the CreateProcess API, so inheritable or not means nothing. | ||
// If we ever add CreateProcess support, this will need to be implemented. | ||
let _ = this.read_scalar(inherit)?; | ||
let options = this.read_scalar(options)?; | ||
|
||
if src_proc != Handle::Pseudo(PseudoHandle::CurrentProcess) { | ||
throw_unsup_format!( | ||
"`DuplicateHandle` `hSourceProcessHandle` parameter is not the current process, which is unsupported" | ||
); | ||
} | ||
|
||
if target_proc != Handle::Pseudo(PseudoHandle::CurrentProcess) { | ||
throw_unsup_format!( | ||
"`DuplicateHandle` `hSourceProcessHandle` parameter is not the current process, which is unsupported" | ||
); | ||
} | ||
|
||
if this.ptr_is_null(target_handle_ptr)? { | ||
throw_unsup_format!( | ||
"`DuplicateHandle` `lpTargetHandle` parameter is null, which is unsupported" | ||
); | ||
} | ||
|
||
if options != this.eval_windows("c", "DUPLICATE_SAME_ACCESS") { | ||
throw_unsup_format!( | ||
"`DuplicateHandle` `dwOptions` parameter is not `DUPLICATE_SAME_ACCESS`, which is unsupported" | ||
); | ||
} | ||
|
||
let new_handle = match src_handle { | ||
Handle::File(old_fd_num) => { | ||
let Some(fd) = this.machine.fds.get(old_fd_num) else { | ||
this.invalid_handle("DuplicateHandle")? | ||
}; | ||
Handle::File(this.machine.fds.insert(fd)) | ||
} | ||
Handle::Thread(_) => { | ||
throw_unsup_format!( | ||
"`DuplicateHandle` called on a thread handle, which is unsupported" | ||
); | ||
} | ||
Handle::Pseudo(pseudo) => Handle::Pseudo(pseudo), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have tests for this code path? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, we don't currently directly test any of the handle shims outside of ones used directly for filesystem stuff. I can add such tests when I get a chance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yea may be good to test these code paths, but also preexisting, so nothing urgent |
||
Handle::Null | Handle::Invalid => this.invalid_handle("DuplicateHandle")?, | ||
}; | ||
|
||
let target_place = this.deref_pointer_as(target_handle, this.machine.layouts.usize)?; | ||
this.write_scalar(new_handle.to_scalar(this), &target_place)?; | ||
|
||
interp_ok(this.eval_windows("c", "TRUE")) | ||
} | ||
|
||
fn CloseHandle(&mut self, handle_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> { | ||
let this = self.eval_context_mut(); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The official docs say
So unsupported is correct, but that doesn't have to be in the error message. The message could just say that the pointer must not be null because the handle must be written somewhere or it would get leaked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure if we wanted to consider this case 'unsupported by Miri' or 'viewed as a bug by Miri'. I'm fine with considering it to be a program bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea it's a weird balance. "unsupported" hints at "may add support", which considering the official docs I would say we won't do