Support for Raw Pointers in windows_process_extensions_raw_attribute Implementation

[michaelvanstraten]

As extensively discussed in issue #114854, the current implementation of the unstable windows_process_extensions_raw_attribute features lacks support for passing a raw pointer.

This PR addresses this issue by allowing the passing of raw pointer types to the std::os::windows::CommandExt::raw_attribute method, which is directly handed to the UpdateProcThreadAttribute Win32 API call.

This change does introduce some concerns with the necessity to pass a raw pointer, but considering this should be treated as a raw interface to set ProcThreadAttributes, there is no real alternative besides drastically increasing the complexity of the interface.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[michaelvanstraten]

I was not able to test this on Windows today because I did not have access to a machine, but I will try to do so tomorrow.

[michaelvanstraten]

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

Where process_handle = parent.0.as_raw_handle().

[michaelvanstraten]

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

Where process_handle = parent.as_raw_handle()

[dtolnay]

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.

[michaelvanstraten]

r? @ChrisDenton

[ChrisDenton]

This is a big enough change in an (albeit unstable) API that it probably should have someone from libs-api giving it a sanity check. cc @dtolnay maybe?

This changes a Windows-only extension to Command. The Windows API has a function, UpdateProcThreadAttribute, that allows setting attribute/value pairs which controls certain features of a new process. This has a fairly generic design so it can be extended in the future (and it has been in the past).

BOOL UpdateProcThreadAttribute(
  [in, out]       LPPROC_THREAD_ATTRIBUTE_LIST lpAttributeList,
  [in]            DWORD                        dwFlags,
  [in]            DWORD_PTR                    Attribute,
  [in]            PVOID                        lpValue,
  [in]            SIZE_T                       cbSize,
  // ...reserved parameters
)

Previously an attempt at a safer (although still unsafe) wrapper was made by taking ownership of a value and then internally passing a pointer to it to the underlying API:

unsafe fn raw_attribute<T: Copy + Send + Sync + 'static>(&mut self, attribute: usize, value: T)

However, this has certain problems. The most notable of which is that a newer attribute got clever and asks you to stuff an integer value into the pointer, which is not possible to do via this API.

This PR changes it to take a raw pointer and size value, which more closely represents the underlying API:

unsafe fn raw_attribute<T: 'static>(&mut self, attribute: usize, value_ptr: *const T, value_size: usize)

This is more unsafe but I think it's the right call for now. If nothing else it allows for experimentation outside of std.

[dtolnay]

@ChrisDenton, this looks great to me. Feel free to approve (with the addr_of changes that Michael has already flagged).

Thank you for the writeup of the underlying Windows API, the current Rust way it's exposed, and the problem that's precipitating changing it.

[dtolnay]

Suggested change

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

[dtolnay]

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.

[dtolnay]

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?

[dtolnay]

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.

[michaelvanstraten]

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.

[michaelvanstraten]

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)));
    }
}

[michaelvanstraten]

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.

[michaelvanstraten]

I've gone ahead and abstracted away the implementation into its own struct michaelvanstraten@4134d3e, let me know if I should abandon this PR in favor.

[michaelvanstraten]

#123604

[Dylan-DPC]

Closing this as it's superseded by #123604