rodrigocfd / winsafe

Windows API and GUI in safe, idiomatic Rust.
https://crates.io/crates/winsafe
MIT License
520 stars 30 forks source link

add HandlerFunctionEx #100

Closed TomzBench closed 1 year ago

TomzBench commented 1 year ago

The winsvc.h has API's to start/stop/install windows system services. Windows services are similar to normal windows in that a service must acknowledge messages from the Windows Kernel.

The first PR attemps to add the types used by the Windows Kernel when communicating with a Windows service. Windows service control messages so far seem all defined in user feature much like other windowing messages.

A later PR would ideally implement the Kernel API's necessary for registering a service with the Windows kernel.

Service Control Message

Functions

TomzBench commented 1 year ago

@rodrigocfd

I added a basic start to deal with the Service Control Manager (SCM) https://learn.microsoft.com/en-us/windows/win32/services/service-control-manager

I would like to add stuff for registering services, however the API at these point should get some feedback if there is potential for adding here. Here is an example API for starting a windows service and listening to messages from the SCM

use winsafe::prelude::*

pub struct MyService {}

impl Service for MyService {
    fn service(&mut self, scm: ServiceControlManager) {
        let (tx, rx) = std::sync::mpsc::channel();

        // Register some callbacks to receive messages from the SCM
        scm.on_stop(|| tx.send(()).unwrap());

        // Let SCM know we are ready
        scm.set_status_running();

        // Wait for shutdown signal from SCM
        rx.recv().unwrap();

        // Exit
        self.set_status_stopped();
    }
}

fn main() {
    let srv = MyService{};

    // Start 1 or more services under a shared process
    winsafe::start_services!(srv);
}

This is kind of early thoughts on the API. It is similar to the window API where using callbacks. I tried to make the api as thin as possible while preventing the user from calling an unsafe function directly. I'm not sure if I'll run into any problems trying to implement this API so it might change.

rodrigocfd commented 1 year ago

Hi Tom,

As for the constants: if they are used by functions from advapi (which is included in kernel), they should be declared in kernel. The GUIDs, AFAIK, may assume a dependency on ole, specially if there is any function returning HRESULT. If this is true, the whole stuff would be dependent of ole.

As for the enums: they have no documentation, so I have no idea what they are used for.

As for the guards: you create a new macro to declare 3 guards, which are very specifically close to each other. I'm not convinced this is necessary.

As for the structs: again, another macro for specific close stuff. See, if I create a macro for each code repetition I find, there would be some many macros that maintenance would be a total nightmare. I avoid creating macros as much as possible. And again, if they will be used in advapi functions, they should really belong to kernel.

As for the Service API: are you trying to create a high-level wrapper to the whole Windows SCM API? If so, that's not the goal of WinSafe, it would be better suited as a separated library. WinSafe is a safe wrapper for native Windows functions (with the notable exception of the gui module).

So far, your approach is not what I would do. As I said, I would start small: implementing just one function. And then another one, and so on... WinSafe is fundamentally a wrapper for functions, everything else is accessory to them. Your top-approach looks very risky, you're trying to implement everything at once.

rodrigocfd commented 1 year ago

I just implemented OpenSCManager function, so you'll have a concrete example of what I'm talking about.

TomzBench commented 1 year ago

@rodrigocfd

Thanks for all the feedback

As for the constants: if they are used by functions from advapi (which is included in kernel), they should be declared in kernel. The GUIDs, AFAIK, may assume a dependency on ole, specially if there is any function returning HRESULT. If this is true, the whole stuff would be dependent of ole.

So far all the constants I added are used when processing SCM messages. So I thought that would be in the user feature. Most were defined in winuser.h Except the SERVICE_CONTROL constants, which I placed in the kernel area.

EDIT the constants are used by functions stemming from advapi which belongs in kernel. However, the constants are part of callback API which is a user declared function handler using mostly values from winuser.h so the ambiguity lead me to think user is the place to go. But will move them to kernel if that is better

As for the enums: they have no documentation, so I have no idea what they are used for.

Every constant and enum I created so far stem from this API https://learn.microsoft.com/en-us/windows/win32/api/winsvc/nc-winsvc-lphandler_function_ex. Which is the SCM message handler function interface. Should missing docs be the only complaint then I can add the docs.

As for the structs: again, another macro for specific clos...

I can remove all the macros, what about macros declared locally to where they are used? Though the macro only declares 3 guards I would rather have that in a macro right where it is used then type the same code three times. Though if you prefer that then I can remove the macro just as well!

As for the Service API: are you trying to create a high-level wrapper to the whole Windows SCM API? I

No, only parts of the API that I need and add them as I need them. I can move this part to a seperate crate, though I would like to know best way to wrap this structure: https://learn.microsoft.com/en-us/windows/win32/api/winsvc/ns-winsvc-service_table_entryw

You pass the above table to https://learn.microsoft.com/en-us/windows/win32/api/winsvc/nf-winsvc-startservicectrldispatcherw. My goal with the proposed API was to wrap StartServiceCtrlDispatcher with a macro to avoid any heap allocations and to remove all the boiler plate needed to populate the SERVICE_TABLE_ENTRY (which is a null terminated blob of pointers to strings and functions). Since this struct very unsafe I thought the macro approach would be ok.

TomzBench commented 1 year ago

So far, your approach is not what I would do. As I said, I would start small: implementing just one function. And then another one, and so on... WinSafe is fundamentally a wrapper for functions, everything else is accessory to them. Your top-approach looks very risky, you're trying to implement everything at once.

I believe I am chosing one thing at a time. It is just that I chose a pretty big thing. LPHANDLER_FUNCTION_EX

But again the higher level stuff can certainly be well in another crate, but that other crate would ideally use safe abstractions from here. So I would only need the SERVICE_TABLE_ENTRYW Which is very unsafe so my proposal I thought a good solution but if you can wrap it more simply perhaps you can do that like you just did for the OpenSCManger handle

rodrigocfd commented 1 year ago

This is my first attempt:

Is is possible to test just these, so we have our first step?

TomzBench commented 1 year ago

@rodrigocfd

Is is possible to test just these, so we have our first step?

You can use a dummy service. However you don't need these functions right away because you can use sc.exe to create and remove services.

I was thinking too start with StartServiceCtrlDispatcher and RegisterServiceCtrlHandlerEx and use sc.exe to do your testing.

For testing HandlerEx I would like to find out if there is a way to get windows to simulate message because it's not possible to generate them all. Ideally you can find some binary blobs online somewhere that represent scm messages and use them in a unit test

Helpful general overview of service API ecosystem https://learn.microsoft.com/en-us/windows/win32/services/service-functions

TomzBench commented 1 year ago

Regarding testing. I asked microsoft help for some solution: https://learn.microsoft.com/en-us/answers/questions/1405149/how-can-i-test-my-lphandle-ex-callback-function

I never used this forum before so i have no idea what to expect.

TomzBench commented 1 year ago

I nuked everything with a force push to update with master and was going to start off simpler with SetServiceStatus however, github closed the PR when I did a force push for some reason. I'll try and add SetServiceStatus with a less ambitious PR. However, please let me know your thoughts on wrapping RegisterServiceCtrlHandlerEx, and StartServiceCtrlDispatcher, and by proxy SERVICE_TABLE_ENTRY