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

Provide SafeHandle instead of manually calling CredFree #1011

Closed jnm2 closed 1 year ago

jnm2 commented 1 year ago

Right now, we're forced to write try...finally around a pointer.

This applies to these functions and possibly more:

public static unsafe NetworkCredential ReadGenericCredential(string targetName)
{
    if (!PInvoke.CredRead(targetName, (int)CRED_TYPE.CRED_TYPE_GENERIC, out var credentialPointer))
        throw new Win32Exception();

    try
    {
        return new NetworkCredential(
            credentialPointer->UserName.ToString(),
            credentialPointer->CredentialBlob is null
                ? null
                : Marshal.PtrToStringUni(
                    (nint)credentialPointer->CredentialBlob,
                    (int)(credentialPointer->CredentialBlobSize / UnicodeEncoding.CharSize)));
    }
    finally
    {
        PInvoke.CredFree(credentialPointer);
    }
}
AArnott commented 1 year ago

I'm afraid this is unlikely to be 'fixed' because types like CREDENTIALW are not mere handles. SafeHandle can only represent an IntPtr. Even if we derived from it to add more fields, as a class it couldn't stand in for the CREDENTIALW struct where methods took pointers to it.

jnm2 commented 1 year ago

I could see it working to have a CREDENTIALW* Pointer property on the generated SafeHandle class, but I do see how this could be worse for discoverability. It would technically pass the all hurdles you mentioned though, since you can do anything with it that you can do with the current out CREDENTIAL* pointer you get.

If you wanted to make things safer, the friendly overloads could take the safe handle instead of in CREDENTIALW/ref CREDENTIALW/CREDENTIALW* parameters. The friendly overload would do DangerousAddRef etc and pass safeHandle.Pointer to the p/invoke method.

AArnott commented 1 year ago

Thanks for describing more about what you were thinking. But I don't think we're going to go with that. If we find many more types that could fit the same pattern it might be worth it, but as a one-off case, this is too much customization to be worthwhile IMO.