search

microsoft / CsWin32

A source generator to add a user-defined set of Win32 P/Invoke methods and supporting types to a C# project.
MIT License
2.07k stars 87 forks source link

ConvertStringSidToSid should probably return something disposable #1069

Closed chrischu closed 11 months ago

chrischu commented 11 months ago

According to the documentation of ConvertStringSidToSid (e.g. https://learn.microsoft.com/en-us/windows/win32/api/sddl/nf-sddl-convertstringsidtosida) the returned Sid should be freed via LocalFree. This leads me to believe that it would be best for the returned value to be disposable so users cannot forget that. Do you agree?

Thanks in advance!

AArnott commented 11 months ago

It's a nice idea, but we can't return a disposable here because we have to return a BOOL to match the win32 method. While we might be able to change the PSID out parameter to a SafeHandle-derived type, PSID itself isn't a handle, and isn't guaranteed to always need to be freed by the caller when used in a win32 API (AFAIK), as it seems perfectly reasonable to pass around PSID in other contexts, that may or may not need to be freed with LocalFree or another function.

So I'm afraid this is another case of CsWin32 being primarily for generating interop APIs -- not to make memory management easier. CsWin32 is no substitute for reading the API docs and following them.

arsdragonfly commented 1 month ago

It's generating FreeSidSafeHandle instead of LocalFreeSafeHandle. Could this be fixed?

AArnott commented 4 weeks ago

Yes, a future metadata update will just produce PSID* instead of SafeHandles, leaving cleanup up to you.