Mutable reference to isize conversion and in reverse soundness

code review
1 
let installed_apps = platform::list_installed_apps();
let running_apps_offset = installed_apps.len();

let mut incorporable_apps = Self {
    inner: installed_apps,
    running_apps_offset
};

// SAFETY: The mutable reference to `incorporable_apps` is created inside the function argument,
// therefore it gets moved into the function.
unsafe {
    platform::list_running_apps(&mut incorporable_apps);
}

incorporable_apps.inner.shrink_to_fit();

incorporable_apps

// SAFETY: `incorporable_apps` (the mutable reference) must be moved into the function when called,
// and not be reborrowed.
pub unsafe fn list_running_apps(incorporable_apps: &mut IncorporableApps) {
    // SAFETY: `lparam` is &mut IncorporableApps casted to LPARAM according to `enum_windows_proc`
    // safety requirement. Since `incorporable_apps` is invalid after calling EnumWindows,
    // we call drop on it beforehand
    let res = unsafe { 
        let lparam = LPARAM(incorporable_apps as *mut _ as isize);

        drop(incorporable_apps);

        EnumWindows(Some(enum_windows_proc), lparam)
    };
}

// SAFETY: `lparam` must be a valid &mut IncorporableApps casted to a LPARAM 
// according to the following process:
// &mut IncorporableApps -> *mut IncorporableApps -> isize -> LPARAM(isize)
unsafe extern "system" fn enum_windows_proc(hwnd: HWND, lparam: LPARAM) -> BOOL {
    let skip = BOOL(1);

    if !is_window_visible(hwnd) {
        return skip;
    }

     // SAFETY: `lparam` must be a valid &mut IncorporableApps casted to a LPARAM as remarked
    let incorporable_apps = unsafe { 
        &mut *(lparam.0 as *mut IncorporableApps) 
    };

    let exe_path = unsafe {
        let mut pid = 0u32;
        let is_err = GetWindowThreadProcessId(hwnd, Some(&mut pid)) == 0;

        if is_err {
            eprintln!("Error getting PID: {}", WinError::from_thread());
            return skip;
        }

        let process_handle = match OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, false, pid) {
            Ok(process_handle) => process_handle,
            Err(err) => {
                eprintln!("Error getting process handle: {err}");
                return skip;                
            }
        };       

        let mut exe_path_buf = [0u16; MAX_PATH as usize];
        let mut path_len = exe_path_buf.len() as u32;

        let res = {
            let win32_path_format = PROCESS_NAME_FORMAT(0);

            QueryFullProcessImageNameW(
                process_handle, win32_path_format, PWSTR(exe_path_buf.as_mut_ptr()), &mut path_len
            )
        };
            
        if let Err(err) = res {
            eprintln!("Error querying process image name: {err}");
            return skip
        }

        if let Err(err) = CloseHandle(process_handle) {
            eprintln!("Error closing process handle: {err}");
            return skip;
        }
        if exe_path_already_exists(incorporable_apps.running_apps(), &exe_path_buf, path_len as usize) {
            return skip;
        }

        String::from_utf16_lossy(&exe_path_buf[..path_len as usize])
    };
    let name = get_file_elegant_name(exe_path.as_ref())
        .expect("`exe_path` resolves to a executable file");

    let icon = match fetch_icon_in_bytes2(exe_path.as_ref()) {
        Ok(icon_in_bytes) => icon_in_bytes,
        Err(err) => {
            eprintln!("Error getting icon from lnk;\nPath to lnk: {exe_path:?};\nErr: {err:?}");

            return skip;
        }               
    };


    let app = App::from(name, exe_path, icon);
    let incorporable_app = IncorporableApp::new_with(app);

    incorporable_apps.inner.push(incorporable_app);

    skip
}

Are the invariants stated by the function enough for soundness? And therefore is my code sound? Ignore the calls to Windows API functions (except EnumWindows)

2

Don’t do drop(incorporable_apps) to try to get rid of the mutable reference. I get why you thought to do that, but the rule about “at most one mutable reference at a time” might be better phrased as “at most one active mutable reference at a time”.

When a mutable reference is “reborrowed”, you can get two mutable references to the same value at the same time! How does this work?!? The answer: the source mutable reference must not be used while the derived one is.

In other words, usage of pointers/references must be “well-nested”, and using a parent mutable reference invalidates its previously-obtained children pointers, or however you want to think about that.

That call to drop actually counts as a usage of the original mutable reference, invalidating the pointer derived from it!

Other than that, it’s probably alright. Sure is a shame that the API forces you to cast a pointer to an integer and back. I strongly prefer avoiding as-casts, though; I recommend explicitly using ptr::from_mut(_).expose_provenance().cast_signed() for &mut T -> isize, and similar in the other direction. as-casts just do so many different operations, so by “principle of least power” and whatnot, it’s better to use more precise functions.

3

Question: Is this not sound?

let a = 5;
let a_ref = &mut a;
let a_ptr = a as *mut _;

*a_ref = 4;
unsafe { *a_ptr = 6; }
*a_ref = 7;
unsafe { *a_ptr = 1; }
// and so on

Because the pointer is derived from the reference, and they both belong to the same "family". I recall reading if the pointer and reference have the same provenance, using them sequentially, regardless if they are the parent or child, like the example given is sound. Of course in my case casting it to a isize removes any provenance tag if I am correct.

4

There were a couple of compilation errors; fixing them as I assume is your intent, I get:

fn main() {
    let mut a = 5;
    let a_ref = &mut a;
    let a_ptr = a_ref as *mut _;
    
    *a_ref = 4;
    unsafe { *a_ptr = 6; }
    *a_ref = 7;
    unsafe { *a_ptr = 1; }
}

If you run this program in Miri, you will get

error: Undefined Behavior: attempting a write access using <396> at alloc200[0x0], but that tag does not exist in the borrow stack for this location
 --> src/main.rs:7:14
  |
7 |     unsafe { *a_ptr = 6; }
  |              ^^^^^^^^^^ this error occurs as part of an access at alloc200[0x0..0x4]
  |
  = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
  = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <396> was created by a SharedReadWrite retag at offsets [0x0..0x4]
 --> src/main.rs:4:17
  |
4 |     let a_ptr = a_ref as *mut _;
  |                 ^^^^^
help: <396> was later invalidated at offsets [0x0..0x4] by a write access
 --> src/main.rs:6:5
  |
6 |     *a_ref = 4;
  |     ^^^^^^^^^^

If you run with MIRIFLAGS=-Zmiri-tree-borrows, it completes without error. So, empirically, this is in the intermediate zone where we don’t confidently know whether Rust’s final semantics will declare this UB or not.

In the stricter model (Stacked Borrows), “same family” is not acceptable reasoning about references; the raw pointer is a “child” of the reference and the reference must not be used until the raw pointer you got from it is no longer in use.

You can do “same family” reasoning about multiple raw pointers, but not mixed raw pointers and references.

5

Ok, thank you for your response.

6

many of the unsafe are unnecessary. EnumWindows calls the callback sequentially and synchronously, it's very easy to make a simple wrapper that is idiomatic and type safe, you don't even need Box, Send or Sync, just a regular FnMut() closure:

use std::ops::ControlFlow;

use windows_sys::Win32::Foundation::{FALSE, HWND, LPARAM};
use windows_sys::Win32::UI::WindowsAndMessaging::EnumWindows;
use windows_sys::core::BOOL;

/// safe (but still low-level) wrapper for `EnumWindows`
/// 
/// personally I prefer an `enum` over a boolean, but a boolean also works.
/// 
/// for example, with the `ControlFlow` enum, I can guess what the 
/// variant `Continue` or `Break` means, easily and confidently,
/// but I can never remember what the boolean value of `true`
/// or `false` means for a specific API.
pub fn safe_enum_windows<F>(mut callback: F) -> bool
where
	F: FnMut(HWND) -> ControlFlow<(), ()>,
{
	let ok = unsafe {
		EnumWindows(
			Some(callback_shim::<F>),
			(&raw mut callback).expose_provenance() as isize,
		)
	};
	ok != FALSE
}

/// use monomophized `shim` as a type safe abi adapter for the callback
unsafe extern "system" fn callback_shim<F>(hwnd: HWND, lparam: LPARAM) -> BOOL
where
	F: FnMut(HWND) -> ControlFlow<(), ()>,
{
	// SAFETY:
	// compile time checked type erasure
	// the api `enum_windows()` guarantees the pointee of `lparam` matches `F`
	let f: &mut F = unsafe { &mut *std::ptr::with_exposed_provenance_mut(lparam as usize) };
	// according to MSDN, returning `TRUE` means the enumeration continues 
	f(hwnd).is_continue().into()
}

then you can create completely safe apis with higher level semantics, for example, your list_running_apps() is just filter_map().

fn list_running_apps() -> Vec<App> {
    enum_windows_filter_map(|hwnd| {
        if !is_window_visiable(hwnd) {
            return None;
        }
        let (exe_path, name, icon) = ...;
        Some(App::new(name, exe_path, icon))
    })
}

here's an example how higher level apis can be implemented:

pub fn enum_windows_filter_map<F, T>(mut callback: F) -> Vec<T>
where
	F: FnMut(HWND) -> Option<T>,
{
	let mut collection = vec![];
	safe_enum_windows(|hwnd| {
		if let Some(value) = callback(hwnd) {
			collection.push(value);
		}
		ControlFlow::Continue(())
	});
	collection
}

pub fn enum_windows_find_map<F, T>(mut callback: F) -> Option<T>
where
	F: FnMut(HWND) -> Option<T>,
{
	let mut found = None;
	safe_enum_windows(|hwnd| {
		if let Some(value) = callback(hwnd) {
			found = Some(value);
			ControlFlow::Break(())
		} else {
			ControlFlow::Continue(())
		}
	});
	found
}