Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SafeHandle to typedef conversion should not ignore AddRef failure #754

Closed
AArnott opened this issue Nov 9, 2022 Discussed in #753 · 1 comment · Fixed by #762
Closed

SafeHandle to typedef conversion should not ignore AddRef failure #754

AArnott opened this issue Nov 9, 2022 Discussed in #753 · 1 comment · Fixed by #762
Labels
bug Something isn't working

Comments

@AArnott
Copy link
Member

AArnott commented Nov 9, 2022

Discussed in #753

Originally posted by User1785604260 November 7, 2022
I needed to use DangerousGetHandle in some of my own code, and wanted to use the generated CsWin32 code as a reference to see if I was doing so correctly. In looking at the generated CsWin32 code, I'm now wondering if its DangerousAddRef/GetHandle usage is correct. I am 1000% not an expert on this, so I may be completely incorrect.

What I noticed is that the success of DangerousAddRef is only used to determine whether to call DangerousRelease. If DangerousAddRef fails, the generated code still proceeds to call DangerousGetHandle and use that handle. From what I can tell this is incorrect, as failure means that the handle is closed if I'm understanding correctly.

If failure shouldn't be ignored, I'm not sure what the correct behavior would be instead. Maybe throwing ObjectDisposedException?

For example, here's an arbitrary method I picked that uses DangerousGetHandle:

public static unsafe int DestroyPhysicalMonitor(SafeHandle hMonitor)
{
    bool hMonitorAddRef = false;
    try
    {
        winmdroot.Foundation.HANDLE hMonitorLocal;
        if (hMonitor is object)
        {
            hMonitor.DangerousAddRef(ref hMonitorAddRef);
            hMonitorLocal = (winmdroot.Foundation.HANDLE)hMonitor.DangerousGetHandle();
        }
        else
            hMonitorLocal = default(winmdroot.Foundation.HANDLE);
        int __result = PInvoke.DestroyPhysicalMonitor(hMonitorLocal);
        return __result;
    }
    finally
    {
        if (hMonitorAddRef)
            hMonitor.DangerousRelease();
    }
}

On a more advanced note, the documentation for DangerousAddRef also indicates that a Constrained Execution Region should be used. I've never touched those and don't know what proper usage of that would look like, and if it's relevant to CsWin32.

Thank you @User1785604260 for bringing this up.

@AArnott AArnott added the bug Something isn't working label Nov 9, 2022
AArnott added a commit that referenced this issue Nov 11, 2022
@AArnott
Copy link
Member Author

AArnott commented Nov 11, 2022

I'm going to move your more advance point to #763 to explore separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant