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

Tracking Issue for windows_process_extensions_raw_attribute #114854

Open
1 of 3 tasks
michaelvanstraten opened this issue Aug 15, 2023 · 29 comments
Open
1 of 3 tasks

Tracking Issue for windows_process_extensions_raw_attribute #114854

michaelvanstraten opened this issue Aug 15, 2023 · 29 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@michaelvanstraten
Copy link
Contributor

michaelvanstraten commented Aug 15, 2023

Feature Gate: #![feature(windows_process_extensions_raw_attribute)]

This is a tracking issue for adding support to attach raw attributes for process creation on Windows using the raw_attribute() method.

Public API

pub trait CommandExt {
    unsafe fn raw_attribute<T: Copy + Send + Sync + 'static>(
        &mut self,
        attribute: usize,
        value: T,
    ) -> &mut process::Command;
}

Steps / History

Unresolved Questions

  • Fix passing of raw pointers
  • Creating safe interface for setting attributes
@michaelvanstraten michaelvanstraten added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 15, 2023
@ChrisDenton
Copy link
Member

Now that there's an initial implementation on nightly people can start experimenting with it. One thing worth exploring is if there's a better API here.

Notably attribute lists can potentially be reused between calls. They are also optionally used for thread creation as well as creating a process (though this is not yet implemented). This suggests it might be helpful to have a ProcThreadAttributes struct, or similar.

The simplest thing would be something that just wraps InitializeProcThreadAttributeList, UpdateProcThreadAttribute and DeleteProcThreadAttributeList. Any such struct would however have limitations. You can only add new or overwrite existing attributes. This means, for example, you can't just append to a list. An alternative would be to use a generic map structure and only construct the attribute list at the last minute. This is much more flexible but also a little more wasteful. But process and thread creation is slow anyway so optimizing for it may not be worth it.

How ever it is implemented, it would also still be very unsafe. An attribute value can contain a struct which itself contains pointer to other data. It would be necessary for the user (or a library) to ensure that data outlives the attribute list. The same goes for other resources, e.g. ensuring HANDLEs aren't closed prematurely. We may want to expose safe wrappers for certain common attributes (e.g. PROC_THREAD_ATTRIBUTE_HANDLE_LIST or PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE).

@michaelvanstraten
Copy link
Contributor Author

michaelvanstraten commented Dec 31, 2023

I currently see an issue with the new raw_attribute API that @TrPR124 and I implemented.

With the current interface, it is not possible to pass a pointer value as an attribute, or at least it seems so. This is required, for example, when trying to start a process attached to a pseudo-console, which is weird since this was the original intent of #88193.

Does anyone see any way the interface could be used to, for example, pass a pseudo-console handle to it?

@michaelvanstraten
Copy link
Contributor Author

@ChrisDenton?

@ivansanchez-oss
Copy link

Any news on this topic?

@kazatsuyu
Copy link

With the current interface, it is not possible to pass a pointer value as an attribute, or at least it seems so.

I was experimenting with working with pseudo consoles in Windows and tried to use this feature, but found that it was not available for this reason.

Since UpdateProcThreadAttribute is basically designed to receive a pointer of some type, it is reasonable to keep it in a Box so that its value does not run out of life, and the existing API should remain as is.

However, it is not possible to handle cases where PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE is passed for attribute and a HPCON for value, so a new API may need to be added, for example as follows.

pub trait CommandExt {
    unsafe fn raw_attribute_ptr(
        &mut self,
        attribute: usize,
        ptr: PointerLikeType,
    ) -> &mut process::Command;
}

Of course, this is a more dangerous function than raw_attribute because its lifetime is unmanageable.

There are several points of question.

  • What type should PointerLikeType be?
    • usize, isize, *const c_void or any other ideas?
  • Is the corresponding value for cbSize in UpdateProcThreadAttribute necessary?
    • I am not sure if there are any use cases other than PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE that require this function.
    • Even if there is no other API than PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE, it may be used if an API is provided. In this case, it is dangerous to pass a fixed value to cbSize.
    • The Creating a Pseudoconsole session tutorial passes sizeof(hpc), which is strange. In practice, it is unlikely that the size of HPCON will be needed, so the system may not check this value, but there is no guarantee

@michaelvanstraten
Copy link
Contributor Author

@kazatsuyu do you know of any place where a raw pointer is used in a std function that interfaces with a system API?

@michaelvanstraten
Copy link
Contributor Author

  • The Creating a Pseudoconsole session tutorial passes sizeof(hpc), which is strange. In practice, it is unlikely that the size of HPCON will be needed, so the system may not check this value, but there is no guarantee

This also triped me up.

@michaelvanstraten
Copy link
Contributor Author

michaelvanstraten commented Feb 3, 2024

  • I am not sure if there are any use cases other than PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE that require this function.

For example, the GROUP_AFFINITY structure has a different size than HPCON which requires us to record the size dynamically.

Windows expects a thin pointer, so using *const T in the interface is misleading because somebody could pass a pointer with a different representation than *const c_void. Not using *const T then hinders us to look up the size of T at compile time.

We would have to accept a separate size parameter, which also could lead to a similar issue.

Optimally for me, we would have something like this:

pub trait CommandExt {
    unsafe fn raw_attribute<T: Sized, *const T: Repr<*const c_void>>(
        &mut self,
        attribute: usize,
        ptr: *const T,
    ) -> &mut process::Command {
        // record size_of::<T>() and the ptr as *const c_void
    }
}

@ivansanchez-oss
Copy link

Sorry for the newbie question, I just want to understand the problem a little more, why is it a problem that HPCON is wrapped in a Box in this scenario?

cmd.raw_attribute(PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE, hpc.0);

Why is it not a problem in this other scenario that is in the documentation?

child_cmd.raw_attribute(PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, parent.as_raw_handle() as isize);

@michaelvanstraten
Copy link
Contributor Author

michaelvanstraten commented Feb 3, 2024

@Ivan-Sanchez-Diaz windows api is a bit weird here since it treats some values as literal pointers. The HPCON in the case of the pseudo console is treated as a pointer to some windows object so when wrapping it in a box and then passing a pointer to the value HPCON what we are doing is pointing to some memory in user space that holds a pointer to a windows object. So we actually need to treat HPCON as a pointer it self here hence why we have to change the interface.

Basically windows expects us to pass HPCON by value not by "reference".

Similar how it would be wrong to pass a &&str to a fn(&str).

In case of the parent process, what we are passing to UpdateProcThreadAttribute is a pointer to the parent process id, which when you think about it is similar when you think of HPCOM as a pointer to some object only we can not dereference HPOC because the value lives in kernel space.

@michaelvanstraten
Copy link
Contributor Author

michaelvanstraten commented Feb 3, 2024

Technically HPCOM is not a direct pointer but a HANDLE which is an index into a table which holds the actual address.

@ivansanchez-oss
Copy link

@Ivan-Sanchez-Diaz windows api is a bit weird here since it treats some values as literal pointers. The HPCON in the case of the pseudo console is treated as a pointer to some windows object so when wrapping it in a box and then passing a pointer to the value HPCON what we are doing is pointing to some memory in user space that holds a pointer to a windows object. So we actually need to treat HPCON as a pointer it self here hence why we have to change the interface.

Basically windows expects us to pass HPCON by value not by "reference".

Similar how it would be wrong to pass a &&str to a fn(&str).

In case of the parent process, what we are passing to UpdateProcThreadAttribute is a pointer to the parent process id, which when you think about it is similar when you think of HPCOM as a pointer to some object only we can not dereference HPOC because the value lives in kernel space.

thanks for the good explanation! now I understand that the problem is that we are passing the reference of the reference to UpdateProcThreadAttribute

@kazatsuyu
Copy link

@kazatsuyu do you know of any place where a raw pointer is used in a std function that interfaces with a system API?

Sorry, I am not very familiar with the dependencies on the system API inside the std library. But it seems to me that a system function that requires passing a pointer of a different type and size, and where the size may be non-trivial, is a rather unique case.

@kazatsuyu
Copy link

@michaelvanstraten To state that the pointer is a thin pointer, it is sufficient to know that transmute::<*const c_void> can be called. In other words, we probably need transmutability.

@michaelvanstraten
Copy link
Contributor Author

I did not know that transmute checks for the size, that would be very useful here if the error message is understandable for the user.

@michaelvanstraten
Copy link
Contributor Author

michaelvanstraten commented Feb 4, 2024

@kazatsuyu I am trying to think of a T where T: Sized while size_of::<*const T>() != size_of::<*const c_void>().

@kazatsuyu
Copy link

@michaelvanstraten Of course it works, but HPCON is not a pointer.

@michaelvanstraten
Copy link
Contributor Author

michaelvanstraten commented Feb 4, 2024

@michaelvanstraten Of course it works, but HPCON is not a pointer.

You would have to cast HPCON to *const c_void, which is required because windows_sys::Win32::System::Threading::UpdateProcThreadAttribute only accepts a *const c_void. So having the cast on the user side makes it less error-prone because it is explicit.

@kazatsuyu
Copy link

You would have to cast HPCON to *const c_void, which is required because windows_sys::Win32::System::Threading::UpdateProcThreadAttribute only accepts a *const c_void. So having the cast on the user side makes it less error-prone because it is explicit.

I understand that. So we use *const T where T: Sized for pointer, the rest is how to pass the size. Using size_of::<T> is not covered GROUP_AFFINITY case, but if T is truly sized type, passing explicit size is a bit redundant. I suggest using Option<usize>, however it may be too complicated.

@michaelvanstraten
Copy link
Contributor Author

michaelvanstraten commented Feb 4, 2024

Using size_of::<T> is not covered GROUP_AFFINITY case

Why is GROUP_AFFINITY not convert?

For example if we had something like the following API:

pub trait CommandExt {
    unsafe fn raw_attribute<T: Sized + 'static>(
        &mut self,
        attribute: usize,
        ptr: *const T,
    ) -> &mut process::Command {
        // record size_of::<T>() and std::men::transmute::<_, *const c_void>(ptr)
    }
}

We could do the following:

let g = GROUP_AFFINITY {
    Mask: 0,
    Group: 6,
    Reserved: [0,0,0],
}

let mut cmd = Cmd::new("foo");

unsafe { cmd.raw_attribute(PROC_THREAD_ATTRIBUTE_GROUP_AFFINITY, addr_of!(g))} ;

If necessary, you can just cask your T to one with the right size.

@kazatsuyu
Copy link

Oh, GROUP_AFFINITY has static size. I misunderstood "to record the size dynamically".
But I read UpdateProcThreadAttribute reference again, found PROC_THREAD_ATTRIBUTE_HANDLE_LIST requires dynamic sized list.

@michaelvanstraten
Copy link
Contributor Author

Passing the slice pointer to would be false here, so the interface is correct to assume that T should be sized.

To do this correctly you would need an unstable API, namely std::raw::Slice

It would look something like this:

#![feature(raw)]

use std::raw::{self, Repr};

let handles = &[0, 1, 2];

let repr: raw::Slice<usize> = handles.repr();

let mut cmd = Cmd::new("foo");

cmd.raw_attribute(PROC_THREAD_ATTRIBUTE_HANDLE_LIST, repr.data, repr.len);

Here repr.data would have type *const usize which is the value PROC_THREAD_ATTRIBUTE_HANDLE_LIST expects.

@michaelvanstraten
Copy link
Contributor Author

This can also be done in stable rust using the as_ptr method on slice.

@kazatsuyu
Copy link

kazatsuyu commented Feb 4, 2024

On second thought,

unsafe fn raw_attribute<'a, 'b: 'a, T: Sized>(
        &'a mut self,
        attribute: usize,
        &'b value: [T],
) -> &mut process::Command {
    let lpvalue = value.as_ptr();
    let cbsize = size_of::<T>() * value.len();
}

seems to work fine in most cases. However, this will not address the HPCON case.

let hpc: HPCON;

cmd.raw_attribute(PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE, std::slice::from_raw_parts(hpc.0 as *const HPCON, 1));

Doing the above will have the same effect as passing hpc to lpvalue and sizeof(HPCON) to cbsize, but creating a slice that does not actually exist, which is pretty ugly.

@kazatsuyu
Copy link

enum Attribute<'a, T> {
     Value(T),
     Slice(&'a [T]),
     UnsafeImmediatePtrDontUseUntillUnderstandWhenYouShouldUseThis {
        lpvalue: *const T,
        cbsize: usize,
    },
}

Perhaps we should allow values to be passed through such a type?

@michaelvanstraten
Copy link
Contributor Author

michaelvanstraten commented Feb 4, 2024

Maybe, but that is probably too specific for a raw interface.
We could do that in the safe interface, I think this should be fine:

pub trait CommandExt {
    unsafe fn raw_attribute<T: Sized + 'static>(
        &mut self,
        attribute: usize,
        ptr: *const T,
        size: usize,
    ) -> &mut process::Command {
        // record size and std::men::transmute::<_, *const c_void>(ptr)
    }
}

It still does not allow for any pointers other than thin ones, and you can pass a slice using it.

@kazatsuyu
Copy link

In the end, that is the most straightforward implementation. Note that size in this case is size_of::<T>() * slice_length. Would it be enough to mention it in the documentation?

@michaelvanstraten
Copy link
Contributor Author

Would it be enough to mention it in the documentation?

It is an unsafe interface, so yes.

@cre4ture
Copy link

Hello,
following this discussion I'm wondering if its really needed to cover all use-cases in a single API-function. Before reading this discussion here, I was implementing my own solution which is now available in PR #124180.
I'm not super experienced yet with rust, so it might be a naive solution or might not be perfect. But I think some the points from this discussions would automatically solved by just providing two API functions: One a bit more high-level/abstracted and the other for low level access.

Please tell me what you think.

best regards.

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 9, 2024
…ute_list, r=<try>

Abstract `ProcThreadAttributeList` into its own struct

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

This PR wants to explore the opportunity to abstract away the `ProcThreadAttributeList` into its own struct to for one improve safety and usability and secondly make it possible to maybe also use it to spawn new threads.

try-job: x86_64-msvc
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 9, 2024
…ute_list, r=<try>

Abstract `ProcThreadAttributeList` into its own struct

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

This PR wants to explore the opportunity to abstract away the `ProcThreadAttributeList` into its own struct to for one improve safety and usability and secondly make it possible to maybe also use it to spawn new threads.

try-job: x86_64-mingw
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 9, 2024
…ute_list, r=<try>

Abstract `ProcThreadAttributeList` into its own struct

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

This PR wants to explore the opportunity to abstract away the `ProcThreadAttributeList` into its own struct to for one improve safety and usability and secondly make it possible to maybe also use it to spawn new threads.

try-job: x86_64-mingw
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 10, 2024
…ute_list, r=<try>

Abstract `ProcThreadAttributeList` into its own struct

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

This PR wants to explore the opportunity to abstract away the `ProcThreadAttributeList` into its own struct to for one improve safety and usability and secondly make it possible to maybe also use it to spawn new threads.

try-job: x86_64-mingw
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 11, 2024
…ute_list, r=<try>

Abstract `ProcThreadAttributeList` into its own struct

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

This PR wants to explore the opportunity to abstract away the `ProcThreadAttributeList` into its own struct to for one improve safety and usability and secondly make it possible to maybe also use it to spawn new threads.

try-job: x86_64-mingw
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants