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

hold reference to SslContextHandle to prevent crashes #73972

Merged
merged 2 commits into from
Aug 16, 2022

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Aug 15, 2022

This is speculative fix for #73621 and #69125.
I was unable to reproduce it locally so this is based on code inspection, experiments and discussions on #73621.
The assumption is that this is regression caused by #64369

The theory is like this:
OpenSSL has its own referencing mechanism. The particular SSL structure it can still hold reference to SSL_CTX it was created with. That for example allows to use TLS session cache attached to SSL_CTX.
Now, from .NET prospective the SafeSslContextHandle representing SSL_CTX can be released as soon as we create SafeSslHandle - we don't use it for anything else but initial setup. Calling ReleaseHandle would free the GCHandle and if any of the callbacks fire after that, they can step on memory that holds garbage.

This change grabs reference on SafeSslContextHandle (if eligible for TLS resume) to preserve it while the session is in use to mimic OpenSSL life cycle. With that, the native callbacks should point to valid memory.

The crashes are happening daily in CI so we should know in next few days if this fixes the crashes.

@wfurt wfurt added area-System.Net.Security os-linux Linux OS (any supported distro) labels Aug 15, 2022
@wfurt wfurt added this to the 7.0.0 milestone Aug 15, 2022
@wfurt wfurt requested review from stephentoub and a team August 15, 2022 22:03
@wfurt wfurt self-assigned this Aug 15, 2022
@ghost
Copy link

ghost commented Aug 15, 2022

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

Issue Details

This is speculative fix for #73621 and #69125.
I was unable to reproduce it locally so this is based on code inspection, experiments and discussions on #73621.
The assumption is that this is regression caused by #64369

The theory is like this:
OpenSSL has its own referencing mechanism. The particular SSL structure it can still hold reference to SSL_CTX it was created with. That for example allows to use TLS session cache attached to SSL_CTX.
Now, from .NET prospective the SafeSslContextHandle representing SSL_CTX can be released as soon as we create SafeSslHandle - we don't use it for anything else but initial setup. Calling ReleaseHandle would free the GCHandle and if any of the callbacks fire after that, they can step on memory that holds garbage.

This change grabs reference on SafeSslContextHandle (if eligible for TLS resume) to preserve it while the session is in use to mimic OpenSSL life cycle. With that, the native callbacks should point to valid memory.

The crashes are happening daily in CI so we should know in next few days if this fixes the crashes.

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Security, os-linux

Milestone: 7.0.0

…aphy.Native/Interop.OpenSsl.cs

Co-authored-by: Stephen Toub <stoub@microsoft.com>
@wfurt wfurt merged commit 2235858 into dotnet:main Aug 16, 2022
@wfurt wfurt deleted the sslCacheContext branch August 16, 2022 19:36
@karelz karelz modified the milestones: 7.0.0, 8.0.0 Aug 22, 2022
@karelz
Copy link
Member

karelz commented Aug 22, 2022

@wfurt I fixed the milestone -- the PR was merged after 7.0 fork, so it won't be part of 7.0.
If we need it in 7.0, we need to backport it.

@wfurt
Copy link
Member Author

wfurt commented Aug 22, 2022

/backport to release/7.0-rc1

1 similar comment
@wfurt
Copy link
Member Author

wfurt commented Aug 22, 2022

/backport to release/7.0-rc1

@github-actions
Copy link
Contributor

Started backporting to release/7.0-rc1: https://github.com/dotnet/runtime/actions/runs/2906074446

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Security os-linux Linux OS (any supported distro)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants