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.

Sign up for GitHub

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

Support for Raw Pointers in windows_process_extensions_raw_attribute Implementation #121951

Closed
wants to merge 1 commit into from
Closed

Support for Raw Pointers in windows_process_extensions_raw_attribute Implementation #121951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 17 additions & 11 deletions library/std/src/os/windows/process.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,8 @@ pub trait CommandExt: Sealed {
/// This method allows you to specify custom attributes for a child process on Windows systems using raw attribute values.
/// Raw attributes provide extended configurability for process creation, but their usage can be complex and potentially unsafe.
///
/// The `attribute` parameter specifies the raw attribute to be set, while the `value` parameter holds the value associated with that attribute.
/// The `attribute` parameter specifies the raw attribute to be set, while the `value_ptr` parameter holds the pointer to the value associated with that attribute,
/// and the `value_size` parameter indicates the size of the value in bytes.
Comment on lines +243 to +244
Copy link
Member

Choose a reason for hiding this comment

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

I am confused about the required lifetime for value_ptr.

In the large example code block at the bottom of https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute#remarks, it demonstrates a correct UpdateProcThreadAttribute call. Roughly:

DWORD ProtectionLevel = ...;

UpdateProcThreadAttribute(...,
                          &ProtectionLevel,
                          sizeof(ProtectionLevel))

CreateProcessW(...)

The documentation for lpValue says (emphasis in the original):

A pointer to the attribute value. This value must persist until the attribute list is destroyed using the DeleteProcThreadAttributeList function.

Where does the DeleteProcThreadAttributeList call occur? Such a thing is not visible in the example, and it's not mentioned anywhere else on that page.

Jumping to conclusions, I assume that CreateProcessW calls DeleteProcThreadAttributeList, i.e. *lpValue (not just **lpValue) needs to remain usable not just for the duration of the UpdateProcThreadAttribute call, but also after it.

If I'm right about that, I think this needs to be made a lot more clear in this Rust documentation.

It wasn't an issue before because the standard library itself would heap-allocate your value for you and keep it until after spawn or whatever else does the CreateProcessW / DeleteProcThreadAttributeList.

If it's now the caller's responsibility, that needs to be a lot more clear with a better example and a discussion of the pitfall of doing .raw_attribute(..., &parent.as_raw_handle() as *const _); with a temporary.

Copy link
Contributor Author

@michaelvanstraten michaelvanstraten Apr 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Where does the DeleteProcThreadAttributeList call occur? It's not visible in the example, nor is it mentioned anywhere else on that page.

It is currently invoked as soon as the ProcThreadAttributeList is dropped at the end of the spawn call.

But you bring up a valid point that was also discussed in the original PR.

Jumping to conclusions, I assume that CreateProcessW calls DeleteProcThreadAttributeList.

Actually, no. You can use the same ProcThreadAttributeList to spawn multiple processes. Perhaps it would be better to offload the entire struct and pass a reference to it during the spawn call, something like spawn_with_attributes. This way, we could tie a lifetime to the new struct with the minimum lifetime passed through the raw_attribute interface.

I will try to write something up.

Copy link
Contributor Author

@michaelvanstraten michaelvanstraten Apr 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I in vision something like this:

struct ProcThreadAttributeListBuilder<'a> {
    values: Vec<(usize, (*const c_void, usize))>,
    _lifetime_marker: PhantomData<fn() -> &'a ()>,
}

impl<'a> ProcThreadAttributeListBuilder<'a> {
    fn new() -> Self {
        ProcThreadAttributeListBuilder {
            values: vec![],
            _lifetime_marker: PhantomData,
        }
    }

    fn attribute<T>(&mut self, attribute: usize, value: &'a T) {
        unsafe {
            self.raw_attribute(attribute, addr_of!(*value).cast::<c_void>(), size_of::<T>());
        }
    }

    unsafe fn raw_attribute<T>(&mut self, attribute: usize, value_ptr: *const T, value_size: usize) {
        self.values.push((attribute, (value_ptr.cast::<c_void>(), value_size)));
    }
}

Copy link
Contributor Author

@michaelvanstraten michaelvanstraten Apr 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think we need a builder pattern here because the length of the attribute list is immutable as soon as it is created.
We might get away with pre-allocating it and then reallocating as soon as the attribute count extends the allocated, but I don't know if windows would like that.

/// Please refer to the [`windows-rs`](https://microsoft.github.io/windows-docs-rs/doc/windows/) documentation or the [`Win32 API documentation`](https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute) for detailed information about available attributes and their meanings.
///
/// # Note
Expand All @@ -257,7 +258,8 @@ pub trait CommandExt: Sealed {
///
/// ```rust
/// #![feature(windows_process_extensions_raw_attribute)]
/// use std::os::windows::{process::CommandExt, io::AsRawHandle};
/// use std::mem;
/// use std::os::windows::{process::CommandExt, io::AsRawHandle, io::RawHandle};
/// use std::process::Command;
///
/// # struct ProcessDropGuard(std::process::Child);
Expand All @@ -266,15 +268,17 @@ pub trait CommandExt: Sealed {
/// # let _ = self.0.kill();
/// # }
/// # }
///
/// #
/// let parent = Command::new("cmd").spawn()?;
///
/// let mut child_cmd = Command::new("cmd");
///
/// const PROC_THREAD_ATTRIBUTE_PARENT_PROCESS: usize = 0x00020000;
///
/// unsafe {
/// child_cmd.raw_attribute(PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, parent.as_raw_handle() as isize);
/// child_cmd.raw_attribute(
/// PROC_THREAD_ATTRIBUTE_PARENT_PROCESS,
/// parent.as_raw_handle(),
Copy link
Contributor Author

@michaelvanstraten michaelvanstraten Mar 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Here again, it will probably be necessary to change this to core::ptr::addr_of!(process_handle).

Where process_handle = parent.as_raw_handle()

Copy link
Member

Choose a reason for hiding this comment

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

Yes. In the old code, it takes the value argument, boxes it, and passes the box's data ptr as the lpValue argument to UpdateProcThreadAttribute. In the new code it directly passes value_ptr through to UpdateProcThreadAttribute. So an extra layer of reference needs to be added somewhere.

/// mem::size_of::<RawHandle>()
/// );
/// }
/// #
/// # let parent = ProcessDropGuard(parent);
Expand All @@ -289,10 +293,11 @@ pub trait CommandExt: Sealed {
///
/// Remember that improper use of raw attributes can lead to undefined behavior or security vulnerabilities. Always consult the documentation and ensure proper attribute values are used.
#[unstable(feature = "windows_process_extensions_raw_attribute", issue = "114854")]
unsafe fn raw_attribute<T: Copy + Send + Sync + 'static>(
unsafe fn raw_attribute<T: 'static>(
&mut self,
attribute: usize,
value: T,
value_ptr: *const T,
value_size: usize,
) -> &mut process::Command;
Comment on lines +296 to 301
Copy link
Member

Choose a reason for hiding this comment

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

I guess the current thinking is to stabilize raw_attribute without any of the reserved parameters of UpdateProcThreadAttribute exposed? dwFlags, lpPreviousValue, lpReturnSize. That seems fine to me, but can you predict how we'd react if those started being used by Windows? A CommandExt::raw_attribute_with_flags, for example? Or a callback kind of thing like unix's CommandExt::pre_exec, but where it gives you access to the lpAttributeList so that you can "do it yourself" with any arbitrary call to UpdateProcThreadAttribute?

}

Expand Down Expand Up @@ -322,12 +327,13 @@ impl CommandExt for process::Command {
self
}

unsafe fn raw_attribute<T: Copy + Send + Sync + 'static>(
unsafe fn raw_attribute<T: 'static>(
&mut self,
attribute: usize,
value: T,
value_ptr: *const T,
value_size: usize,
) -> &mut process::Command {
self.as_inner_mut().raw_attribute(attribute, value);
self.as_inner_mut().raw_attribute(attribute, value_ptr, value_size);
self
}
}
Expand Down
7 changes: 5 additions & 2 deletions library/std/src/process/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,8 +482,11 @@ fn test_proc_thread_attributes() {
let mut child_cmd = Command::new("cmd");

unsafe {
child_cmd
.raw_attribute(PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, parent.0.as_raw_handle() as isize);
child_cmd.raw_attribute(
PROC_THREAD_ATTRIBUTE_PARENT_PROCESS,
parent.0.as_raw_handle(),
Copy link
Contributor Author

@michaelvanstraten michaelvanstraten Mar 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It will probably be necessary to change this to core::ptr::addr_of!(process_handle).

Where process_handle = parent.0.as_raw_handle().

mem::size_of::<HANDLE>(),
);
}

let child = ProcessDropGuard(child_cmd.spawn().unwrap());
Expand Down
15 changes: 9 additions & 6 deletions library/std/src/sys/pal/windows/process.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,14 +248,15 @@ impl Command {
self.cwd.as_ref().map(Path::new)
}

pub unsafe fn raw_attribute<T: Copy + Send + Sync + 'static>(
pub unsafe fn raw_attribute<T: 'static>(
&mut self,
attribute: usize,
value: T,
ptr: *const T,
size: usize,
) {
self.proc_thread_attributes.insert(
attribute,
ProcThreadAttributeValue { size: mem::size_of::<T>(), data: Box::new(value) },
ProcThreadAttributeValue { ptr: mem::transmute::<*const T, *const c_void>(ptr), size },
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ProcThreadAttributeValue { ptr: mem::transmute::<*const T, *const c_void>(ptr), size },
ProcThreadAttributeValue { ptr: ptr.cast::<c_void>(), size },

);
}

Expand Down Expand Up @@ -890,10 +891,13 @@ impl Drop for ProcThreadAttributeList {

/// Wrapper around the value data to be used as a Process Thread Attribute.
struct ProcThreadAttributeValue {
data: Box<dyn Send + Sync>,
ptr: *const c_void,
size: usize,
}

unsafe impl Send for ProcThreadAttributeValue {}
unsafe impl Sync for ProcThreadAttributeValue {}

fn make_proc_thread_attribute_list(
attributes: &BTreeMap<usize, ProcThreadAttributeValue>,
) -> io::Result<ProcThreadAttributeList> {
Expand Down Expand Up @@ -935,13 +939,12 @@ fn make_proc_thread_attribute_list(
// It's theoretically possible for the attribute count to exceed a u32 value.
// Therefore, we ensure that we don't add more attributes than the buffer was initialized for.
for (&attribute, value) in attributes.iter().take(attribute_count as usize) {
let value_ptr = core::ptr::addr_of!(*value.data) as _;
cvt(unsafe {
c::UpdateProcThreadAttribute(
proc_thread_attribute_list.0.as_mut_ptr() as _,
0,
attribute,
value_ptr,
value.ptr,
value.size,
ptr::null_mut(),
ptr::null_mut(),
Expand Down
Loading