Skip to content

[release/8.0-staging] Link peer's X509 stack handle to parent SSL safe handle #115379

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

Merged
merged 7 commits into from
Jun 10, 2025

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented May 7, 2025

Backport of #113124 to release/8.0-staging

Fixes #109689

/cc @rzikm

Customer Impact

Reported by 2 customers. They experience process crashes when using specific pattern with short HTTP request timeout.

The crash happens when they dispose SslStream concurrently with an ongoing handshake operation on Linux. The concurrent dispose may occur when using HttpClient with very short timeout settings.

Regression

No -- the scenario is not too common and the timing in TLS 1.3 might be different from previous TLS versions, affecting chances of the problem happening.

Testing

Successfully reproduced on main with modified Runtime (artificial sleeps in code to increase likelihood of the race condition).
We lost customer repro environment after they updated OS and OpenSSL versions - we cannot validate on their side.

Risk

Low. The change is well contained and uses existing patterns.

@rzikm rzikm self-assigned this May 9, 2025
@karelz karelz added this to the 8.0.x milestone May 28, 2025
Copy link
Contributor

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

@karelz karelz added the Servicing-consider Issue for next servicing release review label May 28, 2025
@karelz
Copy link
Member

karelz commented May 28, 2025

Reliability problem (crash) reported by 2 customers (one of them on 8.0). Worth a fix.

@rzikm rzikm requested review from wfurt and bartonjs June 4, 2025 13:25
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM. should we port the test as well for consistency?

Copy link
Member

@bartonjs bartonjs left a comment

Choose a reason for hiding this comment

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

I agree with Tomas, seems like the test should have been included.

rzikm and others added 3 commits June 5, 2025 16:19
* Defer RemoteCertificate assignment after X509 Chain build

* Add comment
* Fix SslStreamDisposeTest.Dispose_ParallelWithHandshake_ThrowsODE test failures

* Fix build
* [Test Failure] SslStreamDisposeTest.Dispose_ParallelWithHandshake_ThrowsODE on Unix
Fixes #113833

* fixup! [Test Failure] SslStreamDisposeTest.Dispose_ParallelWithHandshake_ThrowsODE on Unix Fixes #113833

* fixup! fixup! [Test Failure] SslStreamDisposeTest.Dispose_ParallelWithHandshake_ThrowsODE on Unix Fixes #113833

* Update src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs

Co-authored-by: Copilot <[email protected]>

* Fix build

---------

Co-authored-by: Copilot <[email protected]>
@rzikm
Copy link
Member

rzikm commented Jun 5, 2025

The test in the original PR was flaky and uncovered other issues, so I had to include more PR (mostli test fixes). It should be good now.

@jozkee
Copy link
Member

jozkee commented Jun 9, 2025

@rzikm reminder: servicing code complete for 8.0.18 is today @ 3PM PST. This is still missing tactics approval.

@jozkee jozkee added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jun 10, 2025
@jozkee
Copy link
Member

jozkee commented Jun 10, 2025

Approved via email

@jozkee jozkee merged commit cbb1cd1 into release/8.0-staging Jun 10, 2025
114 of 117 checks passed
@akoeplinger akoeplinger deleted the backport/pr-113124-to-release/8.0-staging branch June 11, 2025 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants