Bug: LocalFree

[jxy-s]

Which crate is this about?

windows

Crate version

master

Summary

LocalFree has incorrect error handling. LocalFree returns NULL on success. This will return an error when LocalFree succeeds:

https://github.com/microsoft/windows-rs/blob/2e7f99d9ca80bb8f1216db7964062daf42e80158/crates/libs/windows/src/Windows/Win32/System/Memory/mod.rs#L465-L472

Toolchain version/configuration

No response

Reproducible example

No response

Crate manifest

No response

Expected behavior

No response

Actual behavior

No response

Additional comments

No response

[kennykerr]

See microsoft/windows-rs#2392

[jxy-s]

I understand the sentiment, it's certainly an odd use case.

On the other hand, the only reason LocalFree would fail is if you passed an invalid handle, so checking the result isn't particularly useful.

Some find it useful to catch invalid handle errors. The way LocalFree is handled it's wonky to do this.

I think it would as simple as a 1 character change to conform to the LocalFree contract.

-::windows_core::imp::then(!result__.is_invalid(), || result__).ok_or_else(::windows_core::Error::from_win32)
+::windows_core::imp::then(result__.is_invalid(), || result__).ok_or_else(::windows_core::Error::from_win32)

[kennykerr]

Sure, but the code is generated from metadata and I prefer to influence this through the metadata if needed.

https://kennykerr.ca/rust-getting-started/how-are-crates-built.html

[jxy-s]

Perhaps the metadata generation isn't aware of SAL? LocalFree is decorated with _Success_(return=0):
https://github.com/microsoft/win32metadata/blob/main/generation/WinSDK/RecompiledIdlHeaders/um/WinBase.h#L1301-L1308

With all due respect, I believe instances like this should be labeled as "bugs"/"tooling defects"/etc. The work here is already adopted by many and Microsoft should ensure correctness. Dismissing or ignoring the these types of problems are going to reach a critical mass and will reflect poorly on Microsoft+Rust.

[kennykerr]

If the metadata marks the function as PreserveSig = true then we could avoid the transformation. I'll forward to the Win32 metadata repo for their consideration.

[mikebattista]

Should this be marked with [CanReturnMultipleSuccessValues] or [CanReturnErrorsAsSuccess]? We don't use preservesig.

[kennykerr]

Sure, I don't recall the difference between these two attributes.

[mikebattista]

#1315

[kennykerr]

Either works for me. I just treat them all as "don't transform signature" in Rust, but I guess "CanReturnErrorsAsSuccess" sounds like its closest to the intent here. @riverar may have a better answer as he invented this distinction.

[riverar]

This API returns a handle, not an error code, and we annotate the invalid handle values. So it's not clear what metadata changes are needed here. Looks like a crate bug?

public static extern HLOCAL LocalFree([Optional][In] HLOCAL hMem);

[RAIIFree("LocalFree")]
[InvalidHandleValue(-1L)]
[InvalidHandleValue(0L)]
[NativeTypedef]
public struct HLOCAL
{
	public IntPtr Value;
}

Edit: Am re-reading microsoft/windows-rs#2392 maybe I missed something.

[kennykerr]

See microsoft/windows-rs#2392 (comment)

[riverar]

I see, this API is a mutant and returns invalid handles on success. That's unfortunate.

Agree, [CanReturnErrorsAsSuccess] is the appropriate attribute here. Also agree developers should ignore the return value as they can detect for invalid handles earlier and that's the only useful return out of this thing.

[jxy-s]

At times you don't know the handle is invalid or corrupt until you pass it to an API to do something with it. The user of the API has no insight, really, into the internal handle table.

Yes, 0 and -1 are "invalid". But failure on a rogue handle can't be known until it crosses the API boundary and is inspected by the component owning the handle table.

[jxy-s]

Also, I hate to be pedantic here, but accuracy for this project matters. And technically... 0 is a "valid handle" to pass per the API contract. As the SAL implies _Frees_ptr_opt_ it's optional, and a NULL value is safe and is documented as such:

If the hMem parameter is NULL, LocalFree ignores the parameter and returns NULL.

[jxy-s]

Let's consider it in the abstract.

Any handle on the system is subject to being manipulated by another process or the kernel. A handle in a process can be closed by a separate process that isn't the one who owns it.

Or, an even simpler case of a closing a handle twice.

These are situations where error codes should be propagated clearly such that bugs have visibility and not receiving a correct error code can be detrimental for visibility and debug-ability of any system.

[riverar]

HLOCAL is just a HANDLE typedef therefore it logically inherits the same invalid value characteristics. That is, 0 and -1 are both integers used to represent invalid handles. (0 is sourced from NULL, while -1 is covered by INVALID_HANDLE_VALUE.) We encode that into the metadata and the Rust crate generates a helper .is_invalid() -> bool function that developers can use to check if their HANDLE or HLOCAL is invalid as it sits.

Also, I hate to be pedantic here, but accuracy for this project matters. And technically... 0 is a "valid handle" to pass per the API contract. As the SAL implies Frees_ptr_opt it's optional, and a NULL value is safe and is documented as such

Not quite. Acceptance of invalid handle values does not make the handles valid. It just means the API accepts invalid handles and handles them in the documented manner.

[jxy-s]

Thanks for taking care of this issue @mikebattista.

@riverar I think my point is being overlooked, I may have done a poor job at expressing it.

Not quite. Acceptance of invalid handle values does not make the handles valid. It just means the API accepts invalid handles and handles them in the documented manner.

Yes, that's what I said. Perhaps the quotes didn't read the way I intended them to...

The point I'd like to make clear: SAL an excellent source of documentation (usually better than MSDN). Everything you need to know about the input and output of the API is right on the signature. And from what I've gathered the metadata doesn't take that into account. It should.

Regardless, I appreciate everyone's effort. Thank you.

[mikebattista]

We process SAL annotations that ClangSharp scrapes. It doesn't scrape them in this case.

@tannergooding FYI.

[riverar]

SAL an excellent source of documentation (usually better than MSDN) [...] And from what I've gathered the metadata doesn't take that into account. It should.

Agreed. We do some minimal handling of some SAL variants in win32metadata, but there's certainly room for improvement. I believe clangsharp spits out all the raw data we need, we just need to build a SAL lexer/parser.

[mikebattista]

No it's not scraping _Success_(return==0).

[0vercl0k]

I stumbled upon this issue as I ran into the same behavior that jxy-s reported (thank you!) but according to the above conversations; it seems like this is fixed?

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error { code: HRESULT(0x00000000), message: "The operation completed successfully." }', src\thread.rs:56:62

I ran into this with the latest windows crate (v0.51.1) - am I doing something wrong / missing something? 🤔

Cheers

[riverar]

Looks like commit 2047b58 is part of v54.0.44-preview. windows-rs 0.51.1 is still on 53.x https://github.com/microsoft/windows-rs/tree/0.51.1/crates/libs/bindgen/default

[0vercl0k]

That makes sense - thanks for the details, I'll wait patiently for a new release :) Cheers

…

On Mon, Aug 21, 2023 at 9:30 PM Rafael Rivera ***@***.***> wrote: Looks like commit 2047b58 <2047b58> is part of v54.0.44-preview. windows-rs 0.51.1 is still on 53.x https://github.com/microsoft/windows-rs/tree/0.51.1/crates/libs/bindgen/default — Reply to this email directly, view it on GitHub <#1557 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALIOROHKSHF25PEFZIG35LXWQYVZANCNFSM6AAAAAAXS6Z4OA> . You are receiving this because you commented.Message ID: ***@***.***>

[riverar]

@0vercl0k I think in the interim, we can get that merged into the master branch soon (today?) so you can point to the repository in your Cargo.toml.

[0vercl0k]

I'd love that - if you end up making this happen let me know and I'll be happy to try it out! Cheers

…

On Tue, Aug 22, 2023 at 8:39 AM Rafael Rivera ***@***.***> wrote: @0vercl0k <https://github.com/0vercl0k> I think in the interim, we can get that merged into the master branch soon (today?) so you can point to the repository in your Cargo.toml. — Reply to this email directly, view it on GitHub <#1557 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALIORM6NQCAY7I4SVB3AGTXWTHDVANCNFSM6AAAAAAXS6Z4OA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[riverar]

Ah, Rust is currently blocked waiting for 1b2ffca to make its way to a release.

cc: @mikebattista for awareness

[0vercl0k]

First, apologies for bumping this old thread.

Maybe I am missing something, but I expected the new windows-sys v0.52 version to include the changes from the metadata to fix this issue but.. I am still hitting this 🥺.

What am I missing 😅?

Cheers

[riverar]

We haven't brought in the new metadata yet on the Rust side, apologies. There's a known issue with the current metadata that is blocking our ingest, and we're waiting for the next metadata release.

[0vercl0k]

I see, bummer :( Thank you for letting me know! Cheers

…

On Sat, Dec 2, 2023 at 5:43 PM Rafael Rivera ***@***.***> wrote: We haven't brought in the new metadata yet on the Rust side, apologies. There's a known issue with the current metadata that is blocking our ingest, and we're waiting for the next metadata release. — Reply to this email directly, view it on GitHub <#1557 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALIORKWOD42YZITISFCFTDYHNLDDAVCNFSM6AAAAAAXS6Z4OCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZXGE4TSMJVHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[mikebattista]

@riverar we released an update 3 weeks ago that contains this fix. Or are you blocked on something else?

[riverar]

@mikebattista The latest release has the LsaRegisterLogonProcess change (#1735) that was reverted hours later that didn't make it into that release. So we don't want to release a new Rust crate with that bug in it.

[mikebattista]

There's no further changes there. It was reverted by design since Reserved wasn't appropriate.

[riverar]

@mikebattista Ah, there was some confusion here because the release notes still have the change listed. So it didn't look like the revert made it in.

But looking at GitHub, it did! https://github.com/microsoft/win32metadata/commits/v56.0.13-preview

We'll get the wheels spinning on the Rust side, thanks for the ping.

[mikebattista]

Excellent. Glad you're unblocked.

[0vercl0k]

Rafael, let me know if there is / when there is an issue or something I can subscribe to to follow the work :) Thank you all!

…

On Tue, Dec 5, 2023 at 9:52 PM Mike Battista ***@***.***> wrote: Excellent. Glad you're unblocked. — Reply to this email directly, view it on GitHub <#1557 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALIORJBUZINAZSX2CDPTSDYH6CR3AVCNFSM6AAAAAAXS6Z4OCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBRGU4TIMJWGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[riverar]

Will do momentarily!

[riverar]

@0vercl0k PR for updating crate metadata microsoft/windows-rs#2724, if all goes well should have that in today.

[0vercl0k]

Amazing, thank you so much Rafael 🙏

[riverar]

Reopening. [CanReturnErrorsAsSuccess] is not used by Rust. This attribute is used to markup APIs that return errors during common usage. (This would result in exceptions in C# otherwise, for example.) LocalFree has an abnormal form here and should really be marked [PreserveSig] so that all language projections treat it the same.

[mikebattista]

We had a long discussion about these attributes and PreserveSig at #1315.

I'm not sure PreserveSig is correct here based on that discussion.

[riverar]

@mikebattista In this case, the API doesn't return an error code or HRESULT, but rather a handle, so I think both of the attributes are off the table. This one appears to need real PreserveSig treatment.

(Recall [CanReturnMultipleSuccessValues] is for APIs that return S_FALSE, S_OK, etc. which requires special handling in Rust, while [CanReturnErrorsAsSuccess] is for APIs that return errors that could throw in C#, like DXGI_ERROR_NOT_FOUND, during the normal course of operations.)