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

[release/6.0] Remove OpenSSL error queue checking in SslInitializer #65501

Merged
merged 1 commit into from
Mar 10, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Feb 17, 2022

Backport of #65435 to release/6.0

Fixes #64492

/cc @rzikm

Customer Impact

Race condition between crypto stack and HttpClient/SslStream stack can cause initialization error, making HttpClient/SslStream unusable in the process going forward. When crypto stack runs first, it can leave lingering error in OpenSSL error queue (associated with the thread it ran on). If HttpClient/SslStream run on the same thread (that's the race condition), it checks (unnecessarily) the OpenSSL error queue during initialization and fails due to the lingering error left there by crypto stack.

2 customers hit the problem on regular basis when they start their services (couple of times per day).

Regression in 6.0, introduced by #46640 (which made OpenSSL initialization on-demand).

There are no good workarounds except process restart, or using private reflection.

Testing

Validated on targeted repro.

Risk

Low - removal of unnecessary checking of errors on current thread. Not needed as there was nothing in the intialization that ran and could generate such errors.

IMPORTANT: If this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

@ghost
Copy link

ghost commented Feb 17, 2022

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

Issue Details

Backport of #65435 to release/6.0

/cc @rzikm

Customer Impact

Testing

Risk

IMPORTANT: If this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Net

Milestone: -

@rzikm rzikm linked an issue Feb 17, 2022 that may be closed by this pull request
@rzikm
Copy link
Member

rzikm commented Feb 17, 2022

/cc @bartonjs

@rzikm rzikm self-assigned this Feb 17, 2022
@karelz karelz requested a review from wfurt February 21, 2022 15:10
@karelz karelz added this to the 6.0.x milestone Feb 21, 2022
@rzikm rzikm added the Servicing-consider Issue for next servicing release review label Feb 22, 2022
@rbhanda rbhanda modified the milestones: 6.0.x, 6.0.4 Feb 24, 2022
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 24, 2022
@ghost
Copy link

ghost commented Mar 1, 2022

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

Issue Details

Backport of #65435 to release/6.0

Fixes #64492

/cc @rzikm

Customer Impact

Race condition between crypto stack and HttpClient/SslStream stack can cause initialization error, making HttpClient/SslStream unusable in the process going forward. When crypto stack runs first, it can leave lingering error in OpenSSL error queue (associated with the thread it ran on). If HttpClient/SslStream run on the same thread (that's the race condition), it checks (unnecessarily) the OpenSSL error queue during initialization and fails due to the lingering error left there by crypto stack.

2 customers hit the problem on regular basis when they start their services (couple of times per day).

Regression in 6.0, introduced by #46640 (which made OpenSSL initialization on-demand).

There are no good workarounds except process restart, or using private reflection.

Testing

Validated on targeted repro.

Risk

Low - removal of unnecessary checking of errors on current thread. Not needed as there was nothing in the intialization that ran and could generate such errors.

IMPORTANT: If this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: rzikm
Labels:

Servicing-approved, area-System.Net, area-System.Net.Security

Milestone: 6.0.4

@rzikm rzikm removed their assignment Mar 1, 2022
@carlossanlop
Copy link
Member

carlossanlop commented Mar 8, 2022

@rzikm @bartonjs @karelz @wfurt @dotnet/ncl Does this change affect any OOB packages that we need to rebuild? The reason I ask is because, per the servicing instructions, we need to make sure backported changes also turn on the <GeneratePackageOnBuild> boolean property of any OOB packages that need to consume the code fix, so they get built for the next servicing release.

Also ask because the change affects a file inside a System.Net.Security.Native folder but it's for a C# file.

@bartonjs
Copy link
Member

bartonjs commented Mar 8, 2022

$ find . -name \*.csproj -exec  grep -Hn "Net.Security.Native.Interop.Initialization.cs" {} \;
./System.Net.Http/src/System.Net.Http.csproj:601:    <Compile Include="$(CommonPath)Interop\Unix\System.Net.Security.Native\Interop.Initialization.cs"
./System.Net.Http/src/System.Net.Http.csproj:602:             Link="Common\Interop\Unix\System.Net.Security.Native\Interop.Initialization.cs" />
./System.Net.Security/src/System.Net.Security.csproj:274:    <Compile Include="$(CommonPath)Interop\Unix\System.Net.Security.Native\Interop.Initialization.cs"
./System.Net.Security/src/System.Net.Security.csproj:275:             Link="Common\Interop\Unix\System.Net.Security.Native\Interop.Initialization.cs" />
./System.Net.Quic/src/System.Net.Quic.csproj:75:    <Compile Include="$(CommonPath)Interop\Unix\System.Net.Security.Native\Interop.Initialization.cs" Link="Common\Interop\Unix\System.Net.Security.Native\Interop.Initialization.cs" />

System.Net.Http and System.Net.Security are inbox. I don't know what the status for System.Net.Quic is in release/6.0. @wfurt?

@wfurt
Copy link
Member

wfurt commented Mar 8, 2022

yes. Quic is part of runtime. For 6.0, it is hidden behind "preview" flag but that does not impact packaging.

@ericstj ericstj merged commit bcd6042 into release/6.0 Mar 10, 2022
@akoeplinger akoeplinger deleted the backport/pr-65435-to-release/6.0 branch March 11, 2022 16:30
@ghost ghost locked as resolved and limited conversation to collaborators Apr 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpClient fails to initialize SSL.
7 participants