rodrigocfd / winsafe

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

Public access ot LocalFreeGuard::new or LocalFree #92

Closed Hawk777 closed 1 year ago

Hawk777 commented 1 year ago

The documentation for LocalFreeGuard::new says:

This method is used internally by the library, and not intended to be used externally.

However, as far as I can tell, there is no other way to either create a LocalFreeGuard or call LocalFree directly. Those are useful things to do, for example, if you get a fixed-address HLOCAL that’s created for you by NCryptProtectSecret. Of course winsafe doesn’t wrap that function, but still, it seems reasonable that one should be able to use non-winsafe APIs (either from the windows crate or by creating extern declarations by hand) for the few things where they are necessary, and then move to winsafe wrappers ASAP for better safety while holding onto the allocated buffer.

rodrigocfd commented 1 year ago

WinSafe is designed to be interoperable with other libraries, so it has many escape hatches, like Handle::ptr and LocalFreeGuard::new. Indeed, I wrote that they are not meant to be used externally, but maybe it's an overconservative comment, because they can – that's why they're all pub. They must be unsafe, however, since they can easily trigger UB.

In your specific case, you can construct the HLOCAL and store the guard like this:

use winsafe::{self as w, prelude::*};

let pp: *mut std::ffi::c_void; // initialized somewhere

let my_handle = unsafe { w::HLOCAL::from_ptr(pp) };
let my_guard = unsafe { w::guard::LocalFreeGuard::new(my_handle) };

Or, if you just need to call LocalFree immediately:

use winsafe::{self as w, prelude::*};

let pp: *mut std::ffi::c_void; // initialized somewhere

let _ = unsafe { w::guard::LocalFreeGuard::new(w::HLOCAL::from_ptr(pp)) };

I will review the docs, and make it explicit that you can use those stuff to interop with other libraries.

Is this OK?

Hawk777 commented 1 year ago

Yeah, it’s just the comment that I was concerned about. I’m already doing exactly what you suggested in your first example. I agree that new should be unsafe. Thanks!