LocalFree returns an Err when it succeeds

[fgimian]

Summary

As per the docs for LocalFree:

If the function succeeds, the return value is NULL.

If the function fails, the return value is equal to a handle to the local memory object. To get extended error information, call GetLastError.

However, the implementation of LocalFree via the crate fails if the return value is a null pointer as shown below:

#[inline]
pub unsafe fn LocalFree<P0>(hmem: P0) -> ::windows_core::Result<HLOCAL>
where
    P0: ::windows_core::IntoParam<HLOCAL>,
{
    ::windows_targets::link!("kernel32.dll" "system" fn LocalFree(hmem : HLOCAL) -> HLOCAL);
    let result__ = LocalFree(hmem.into_param().abi());
    (!result__.is_invalid()).then(|| result__).ok_or_else(::windows_core::Error::from_win32)
}

Where is_invalid is defined as follows on HLOCAL:

impl HLOCAL {
    pub fn is_invalid(&self) -> bool {
        self.0.is_null()
    }
}

See an example below 😄

Crate manifest

[package]
name = "free-problem"
version = "0.1.0"
edition = "2021"

[dependencies]
windows-targets = "0.52.3"

[dependencies.windows]
version = "0.53.0"
features = [
    "Win32_Foundation",
    "Win32_System_Power",
    "Win32_System_Registry",
]

Crate code

use windows::{
    core::GUID,
    Win32::{
        Foundation::{LocalFree, HLOCAL},
        System::Power::PowerGetActiveScheme,
    },
};

/// An identical copy of the original LocalFree function with a print added to validate the
/// result returned from the Win32 LocalFree function.
#[inline]
pub unsafe fn MyLocalFree<P0>(hmem: P0) -> windows::core::Result<HLOCAL>
where
    P0: windows::core::IntoParam<HLOCAL>,
{
    windows_targets::link!("kernel32.dll" "system" fn LocalFree(hmem : HLOCAL) -> HLOCAL);
    let result__ = LocalFree(hmem.into_param().abi());
    println!("LocalFree result is {result__:?}");
    (!result__.is_invalid())
        .then(|| result__)
        .ok_or_else(windows::core::Error::from_win32)
}

fn main() {
    unsafe {
        let mut active_scheme_guid = std::ptr::null_mut::<GUID>();
        PowerGetActiveScheme(None, &mut active_scheme_guid).unwrap();
        LocalFree(HLOCAL(active_scheme_guid.cast())).unwrap();
        // MyLocalFree(HLOCAL(active_scheme_guid.cast())).unwrap();
    }
}

Output is as follows:

thread 'main' panicked at src\main.rs:28:54:
called `Result::unwrap()` on an `Err` value: Error { code: HRESULT(0x00000000), message: "The operation completed successfully." }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: process didn't exit successfully: `target\debug\free-problem.exe` (exit code: 101)

Also notice the message from GetLastError is "The operation completed successfully." 😄

Using MyLocalFree confirms that LocalFree is in fact succeeding with a NULL pointer being returned:

LocalFree result is HLOCAL(0x0)
thread 'main' panicked at src\main.rs:29:56:
called `Result::unwrap()` on an `Err` value: Error { code: HRESULT(0x00000000), message: "The operation completed successfully." }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Just because I doubted myself so much, I even ensured that memory was correctly being cleared via Task Manager while having the operation run over and over:

use windows::{
    core::GUID,
    Win32::{
        Foundation::{LocalFree, HLOCAL},
        System::Power::PowerGetActiveScheme,
    },
};

fn main() {
    loop {
        unsafe {
            let mut active_scheme_guid = std::ptr::null_mut::<GUID>();
            PowerGetActiveScheme(None, &mut active_scheme_guid).unwrap();
            LocalFree(HLOCAL(active_scheme_guid.cast())).ok();
        }
    }
}

Commenting out the LocalFree call leaks memory as expected.

[riverar]

NULL is an invalid value for HLOCAL so is_invalid behavior is correct. This API just returns invalid handle values as its success value.

[riverar]

Oh I read that too fast, that's in the crate! 😅

[tim-weis]

This is a recurring issue. It boils down to a messy state of affairs in how certain APIs (including this one) are represented in metadata. 0.53.0 of windows is built from 58.0.18-preview of Win32Metadata, with this description (slightly abridged):

[DllImport("KERNEL32.dll", ExactSpelling = true, PreserveSig = false, SetLastError = true)]
[CanReturnErrorsAsSuccess]
public static extern HLOCAL LocalFree([Optional][In] HLOCAL hMem);

[CanReturnErrorsAsSuccess] is intended to describe the success/failure outcome of the API, but windows-rs evaluates the PreserveSig attribute only. #2728 attempted to account for the former, but it was closed for some reason.

I haven't investigated whether PreserveSig at some point changed from true to false and I have lost track of what the current consensus is concerning metadata annotations. All I know is that the attributes chosen by win32metadata aren't orthogonal, and that's a bad situation to be in.

This specific issue could be addressed here, but I feel that moving the metadata attributes closer to SAL annotations (instead of inventing new ones) would benefit everyone in the long run. I'm equally confident that this won't be met with a lot of enthusiasm.

[kennykerr]

See #2889

[riverar]

[CanReturnErrorsAsSuccess] describes the API behavior here but the crate doesn't read it. I'll have to go review all the context around this again. 🫠

[kennykerr]

I'm not clear on what CanReturnErrorsAsSuccess is supposed to do to help Rust.

[fgimian]

Thanks for your help everyone. @kennykerr I suspect GlobalFree suffers the same issue and may be worth a look alongside this.

Cheers
Fotis