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

[QUIC] Support for OpenSSL build of MsQuic on Windows (Attempt 2) #72609

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Jul 21, 2022

Closes #69978.

The problem with the original PR (#72262) was that in case that an incompatible MsQuic version was loaded, we didn't close the API Table handle before unloading the library. This led to us unloading the library while some code was still executing on a background MsQuic thread which crashed the process (this was way more probable on Release configurations than on Debug). This PR reintroduces the feature with appropriate changes.

@ghost
Copy link

ghost commented Jul 21, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Closes #69978.

The problem with the original PR (#72262) was that in case that an incompatible MsQuic version was loaded, we didn't close the API Table handle before unloading the library. This led to us unloading the library while some code was still executing on a background MsQuic thread which crashed the process (this was way more probable on Release configurations than on Debug). This PR reintroduces the feature with appropriate changes.

Author: rzikm
Assignees: -
Labels:

area-System.Net.Quic

Milestone: -

@rzikm rzikm requested a review from ManickaP July 21, 2022 14:25
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for looking into this!

}
finally
{
if (!IsQuicSupported)
if (!IsQuicSupported && NativeLibrary.TryGetExport(msQuicHandle, "MsQuicClose", out IntPtr msQuicClose))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we had the same issue before your change as well, we just never hit the right set of conditions for it to manifest and with your original change it happened on every Windows non-TLS1.3 machine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we did not hit it before because the OS version check was before we opened the MsQuic library. I had to move the OS check later once we knew which TLS backend MsQuic is using.

We could have still hit the issue when opening an older MsQuic version on sufficiently new Windows OS

…uicApi.cs

Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>
@rzikm rzikm merged commit ed5aa3d into dotnet:main Jul 22, 2022
@karelz karelz added this to the 7.0.0 milestone Aug 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[QUIC] Support for OpenSSL build of MsQuic on Windows
3 participants