Add ActiveIssue for all named key tests to mitigate invalid memory access - #129367

Merged
vcsjones merged 3 commits into
dotnet:mainfrom
vcsjones:active-issue-129339
Jun 13, 2026
Merged

Add ActiveIssue for all named key tests to mitigate invalid memory access#129367
vcsjones merged 3 commits into
dotnet:mainfrom
vcsjones:active-issue-129339

Conversation

@vcsjones

Copy link
Copy Markdown
Member

This adds an ActiveIssue to all functional members of OpenSslNamedKeysTests because they are very likely the source of memory corruption that is showing in #129339.

Disable the tests for now so that CI is stable while permanent fixes are investigated.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates OpenSslNamedKeysTests by adding ActiveIssue annotations to skip many of the named key functional tests while issue #129339 (segfault/memory corruption in System.Security.Cryptography.Tests) is investigated, aiming to keep CI stable.

Changes:

  • Added [ActiveIssue("https://github.com/dotnet/runtime/issues/129339")] to most test methods in OpenSslNamedKeysTests.manual.cs to disable them.
  • Left some tests in the same class still runnable (e.g., argument/URI validation tests), despite the stated intent to disable the suite.
Show a summary per file
FileDescription
src/libraries/System.Security.Cryptography/tests/OpenSslNamedKeysTests.manual.csAdds ActiveIssue skips to many named key tests to mitigate suspected CI instability from #129339.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 2

CopilotAI review requested due to automatic review settings June 13, 2026 14:19

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

@vcsjones
vcsjones enabled auto-merge (squash) June 13, 2026 17:06
@vcsjones

Copy link
Copy Markdown
MemberAuthor

/ba-g timeout on unrelated platform; test only changes to disable some problematic tests.

@vcsjones
vcsjones merged commit 9405cac into dotnet:mainJun 13, 2026
92 of 95 checks passed
@vcsjones
vcsjones deleted the active-issue-129339 branch June 13, 2026 18:46
vcsjones added a commit that referenced this pull request Jun 16, 2026
Freeing an OSSL_LIB_CTX is a somewhat complicated task. It registers
per-thread callbacks to mutate thread-local state. Other OpenSSL APIs,
like random number generation, also have thread-local state. In the case
of the random number generator, it registered a thread data destructor
to clean up random state when the thread is shutting down. This, in
turn, sees that the OSSL_LIB_CTX is associated with that thread.
If the OSSL_LIB_CTX is freed before the thread has shut down, the thread
dtor will access invalid memory.
OpenSSL requires you to call `OPENSSL_thread_stop_ex` on _all_ threads
before calling `OSSL_LIB_CTX_free` to unhook the context from the
thread.
This is generally not feasible for .NET.
As a solution, we are going to not free the context, and re-use it for
each provider. Every provider loaded with
`SafeEvpPKeyHandle.OpenKeyFromProvider` will create a context and load
that provider into it. The context and provider will remain alive for
the duration of the process.
The number of providers expected to be used in any application is
generally expected to be low - likely two or three.
1. The provider and context are now cached, and kept in a heap-backed
buffer. There is no resizing strategy or pre-allocated buffer. Opening a
new provider results in re-sizing the array. This strategy is likely
fine because there will rarely ever be more than a few providers, as
already noted. The resizing is managed by a mutex to synchronize access
to the array. We could use something like `pthread_rwlock_t` if we
wanted to increase complexity. Given that contention on this mutex would
only happen from `OpenKeyFromProvider`, I expect it to be rare.
2. Since the context and provider are now alive permanently, the "extra
handle" bookkeeping gets simpler. We no longer need to pass it to
destroy, and we no longer need to ref-count it. In fact we don't even
need a struct at all anymore, the OSSL_LIB_CTX _is_ the extra handle
now.
3. This reverts #129367 since the
data dependency is resolved.
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone Jun 17, 2026
eiriktsarpalis pushed a commit that referenced this pull request Jul 15, 2026
Freeing an OSSL_LIB_CTX is a somewhat complicated task. It registers
per-thread callbacks to mutate thread-local state. Other OpenSSL APIs,
like random number generation, also have thread-local state. In the case
of the random number generator, it registered a thread data destructor
to clean up random state when the thread is shutting down. This, in
turn, sees that the OSSL_LIB_CTX is associated with that thread.
If the OSSL_LIB_CTX is freed before the thread has shut down, the thread
dtor will access invalid memory.
OpenSSL requires you to call `OPENSSL_thread_stop_ex` on _all_ threads
before calling `OSSL_LIB_CTX_free` to unhook the context from the
thread.
This is generally not feasible for .NET.
As a solution, we are going to not free the context, and re-use it for
each provider. Every provider loaded with
`SafeEvpPKeyHandle.OpenKeyFromProvider` will create a context and load
that provider into it. The context and provider will remain alive for
the duration of the process.
The number of providers expected to be used in any application is
generally expected to be low - likely two or three.
1. The provider and context are now cached, and kept in a heap-backed
buffer. There is no resizing strategy or pre-allocated buffer. Opening a
new provider results in re-sizing the array. This strategy is likely
fine because there will rarely ever be more than a few providers, as
already noted. The resizing is managed by a mutex to synchronize access
to the array. We could use something like `pthread_rwlock_t` if we
wanted to increase complexity. Given that contention on this mutex would
only happen from `OpenKeyFromProvider`, I expect it to be rare.
2. Since the context and provider are now alive permanently, the "extra
handle" bookkeeping gets simpler. We no longer need to pass it to
destroy, and we no longer need to ref-count it. In fact we don't even
need a struct at all anymore, the OSSL_LIB_CTX _is_ the extra handle
now.
3. This reverts #129367 since the
data dependency is resolved.
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jul 18, 2026
Sign up for freeto 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.

4 participants

@vcsjones@jkotas@PranavSenthilnathan
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Add ActiveIssue for all named key tests to mitigate invalid memory access - #129367

Merged
vcsjones merged 3 commits into
dotnet:mainfrom
vcsjones:active-issue-129339
Jun 13, 2026
Merged

Add ActiveIssue for all named key tests to mitigate invalid memory access#129367
vcsjones merged 3 commits into
dotnet:mainfrom
vcsjones:active-issue-129339

Conversation

@vcsjones

Copy link
Copy Markdown
Member

This adds an ActiveIssue to all functional members of OpenSslNamedKeysTests because they are very likely the source of memory corruption that is showing in #129339.

Disable the tests for now so that CI is stable while permanent fixes are investigated.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates OpenSslNamedKeysTests by adding ActiveIssue annotations to skip many of the named key functional tests while issue #129339 (segfault/memory corruption in System.Security.Cryptography.Tests) is investigated, aiming to keep CI stable.

Changes:

  • Added [ActiveIssue("https://github.com/dotnet/runtime/issues/129339")] to most test methods in OpenSslNamedKeysTests.manual.cs to disable them.
  • Left some tests in the same class still runnable (e.g., argument/URI validation tests), despite the stated intent to disable the suite.
Show a summary per file
FileDescription
src/libraries/System.Security.Cryptography/tests/OpenSslNamedKeysTests.manual.csAdds ActiveIssue skips to many named key tests to mitigate suspected CI instability from #129339.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 2

CopilotAI review requested due to automatic review settings June 13, 2026 14:19

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

@vcsjones
vcsjones enabled auto-merge (squash) June 13, 2026 17:06
@vcsjones

Copy link
Copy Markdown
MemberAuthor

/ba-g timeout on unrelated platform; test only changes to disable some problematic tests.

@vcsjones
vcsjones merged commit 9405cac into dotnet:mainJun 13, 2026
92 of 95 checks passed
@vcsjones
vcsjones deleted the active-issue-129339 branch June 13, 2026 18:46
vcsjones added a commit that referenced this pull request Jun 16, 2026
Freeing an OSSL_LIB_CTX is a somewhat complicated task. It registers
per-thread callbacks to mutate thread-local state. Other OpenSSL APIs,
like random number generation, also have thread-local state. In the case
of the random number generator, it registered a thread data destructor
to clean up random state when the thread is shutting down. This, in
turn, sees that the OSSL_LIB_CTX is associated with that thread.
If the OSSL_LIB_CTX is freed before the thread has shut down, the thread
dtor will access invalid memory.
OpenSSL requires you to call `OPENSSL_thread_stop_ex` on _all_ threads
before calling `OSSL_LIB_CTX_free` to unhook the context from the
thread.
This is generally not feasible for .NET.
As a solution, we are going to not free the context, and re-use it for
each provider. Every provider loaded with
`SafeEvpPKeyHandle.OpenKeyFromProvider` will create a context and load
that provider into it. The context and provider will remain alive for
the duration of the process.
The number of providers expected to be used in any application is
generally expected to be low - likely two or three.
1. The provider and context are now cached, and kept in a heap-backed
buffer. There is no resizing strategy or pre-allocated buffer. Opening a
new provider results in re-sizing the array. This strategy is likely
fine because there will rarely ever be more than a few providers, as
already noted. The resizing is managed by a mutex to synchronize access
to the array. We could use something like `pthread_rwlock_t` if we
wanted to increase complexity. Given that contention on this mutex would
only happen from `OpenKeyFromProvider`, I expect it to be rare.
2. Since the context and provider are now alive permanently, the "extra
handle" bookkeeping gets simpler. We no longer need to pass it to
destroy, and we no longer need to ref-count it. In fact we don't even
need a struct at all anymore, the OSSL_LIB_CTX _is_ the extra handle
now.
3. This reverts #129367 since the
data dependency is resolved.
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone Jun 17, 2026
eiriktsarpalis pushed a commit that referenced this pull request Jul 15, 2026
Freeing an OSSL_LIB_CTX is a somewhat complicated task. It registers
per-thread callbacks to mutate thread-local state. Other OpenSSL APIs,
like random number generation, also have thread-local state. In the case
of the random number generator, it registered a thread data destructor
to clean up random state when the thread is shutting down. This, in
turn, sees that the OSSL_LIB_CTX is associated with that thread.
If the OSSL_LIB_CTX is freed before the thread has shut down, the thread
dtor will access invalid memory.
OpenSSL requires you to call `OPENSSL_thread_stop_ex` on _all_ threads
before calling `OSSL_LIB_CTX_free` to unhook the context from the
thread.
This is generally not feasible for .NET.
As a solution, we are going to not free the context, and re-use it for
each provider. Every provider loaded with
`SafeEvpPKeyHandle.OpenKeyFromProvider` will create a context and load
that provider into it. The context and provider will remain alive for
the duration of the process.
The number of providers expected to be used in any application is
generally expected to be low - likely two or three.
1. The provider and context are now cached, and kept in a heap-backed
buffer. There is no resizing strategy or pre-allocated buffer. Opening a
new provider results in re-sizing the array. This strategy is likely
fine because there will rarely ever be more than a few providers, as
already noted. The resizing is managed by a mutex to synchronize access
to the array. We could use something like `pthread_rwlock_t` if we
wanted to increase complexity. Given that contention on this mutex would
only happen from `OpenKeyFromProvider`, I expect it to be rare.
2. Since the context and provider are now alive permanently, the "extra
handle" bookkeeping gets simpler. We no longer need to pass it to
destroy, and we no longer need to ref-count it. In fact we don't even
need a struct at all anymore, the OSSL_LIB_CTX _is_ the extra handle
now.
3. This reverts #129367 since the
data dependency is resolved.
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jul 18, 2026
Sign up for freeto 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.

4 participants

@vcsjones@jkotas@PranavSenthilnathan
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add ActiveIssue for all named key tests to mitigate invalid memory access - #129367

Merged
vcsjones merged 3 commits into
dotnet:mainfrom
vcsjones:active-issue-129339
Jun 13, 2026
Merged

Add ActiveIssue for all named key tests to mitigate invalid memory access#129367
vcsjones merged 3 commits into
dotnet:mainfrom
vcsjones:active-issue-129339

Conversation

@vcsjones

Copy link
Copy Markdown
Member

This adds an ActiveIssue to all functional members of OpenSslNamedKeysTests because they are very likely the source of memory corruption that is showing in #129339.

Disable the tests for now so that CI is stable while permanent fixes are investigated.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates OpenSslNamedKeysTests by adding ActiveIssue annotations to skip many of the named key functional tests while issue #129339 (segfault/memory corruption in System.Security.Cryptography.Tests) is investigated, aiming to keep CI stable.

Changes:

  • Added [ActiveIssue("https://github.com/dotnet/runtime/issues/129339")] to most test methods in OpenSslNamedKeysTests.manual.cs to disable them.
  • Left some tests in the same class still runnable (e.g., argument/URI validation tests), despite the stated intent to disable the suite.
Show a summary per file
FileDescription
src/libraries/System.Security.Cryptography/tests/OpenSslNamedKeysTests.manual.csAdds ActiveIssue skips to many named key tests to mitigate suspected CI instability from #129339.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 2

CopilotAI review requested due to automatic review settings June 13, 2026 14:19

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

@vcsjones
vcsjones enabled auto-merge (squash) June 13, 2026 17:06
@vcsjones

Copy link
Copy Markdown
MemberAuthor

/ba-g timeout on unrelated platform; test only changes to disable some problematic tests.

@vcsjones
vcsjones merged commit 9405cac into dotnet:mainJun 13, 2026
92 of 95 checks passed
@vcsjones
vcsjones deleted the active-issue-129339 branch June 13, 2026 18:46
vcsjones added a commit that referenced this pull request Jun 16, 2026
Freeing an OSSL_LIB_CTX is a somewhat complicated task. It registers
per-thread callbacks to mutate thread-local state. Other OpenSSL APIs,
like random number generation, also have thread-local state. In the case
of the random number generator, it registered a thread data destructor
to clean up random state when the thread is shutting down. This, in
turn, sees that the OSSL_LIB_CTX is associated with that thread.
If the OSSL_LIB_CTX is freed before the thread has shut down, the thread
dtor will access invalid memory.
OpenSSL requires you to call `OPENSSL_thread_stop_ex` on _all_ threads
before calling `OSSL_LIB_CTX_free` to unhook the context from the
thread.
This is generally not feasible for .NET.
As a solution, we are going to not free the context, and re-use it for
each provider. Every provider loaded with
`SafeEvpPKeyHandle.OpenKeyFromProvider` will create a context and load
that provider into it. The context and provider will remain alive for
the duration of the process.
The number of providers expected to be used in any application is
generally expected to be low - likely two or three.
1. The provider and context are now cached, and kept in a heap-backed
buffer. There is no resizing strategy or pre-allocated buffer. Opening a
new provider results in re-sizing the array. This strategy is likely
fine because there will rarely ever be more than a few providers, as
already noted. The resizing is managed by a mutex to synchronize access
to the array. We could use something like `pthread_rwlock_t` if we
wanted to increase complexity. Given that contention on this mutex would
only happen from `OpenKeyFromProvider`, I expect it to be rare.
2. Since the context and provider are now alive permanently, the "extra
handle" bookkeeping gets simpler. We no longer need to pass it to
destroy, and we no longer need to ref-count it. In fact we don't even
need a struct at all anymore, the OSSL_LIB_CTX _is_ the extra handle
now.
3. This reverts #129367 since the
data dependency is resolved.
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone Jun 17, 2026
eiriktsarpalis pushed a commit that referenced this pull request Jul 15, 2026
Freeing an OSSL_LIB_CTX is a somewhat complicated task. It registers
per-thread callbacks to mutate thread-local state. Other OpenSSL APIs,
like random number generation, also have thread-local state. In the case
of the random number generator, it registered a thread data destructor
to clean up random state when the thread is shutting down. This, in
turn, sees that the OSSL_LIB_CTX is associated with that thread.
If the OSSL_LIB_CTX is freed before the thread has shut down, the thread
dtor will access invalid memory.
OpenSSL requires you to call `OPENSSL_thread_stop_ex` on _all_ threads
before calling `OSSL_LIB_CTX_free` to unhook the context from the
thread.
This is generally not feasible for .NET.
As a solution, we are going to not free the context, and re-use it for
each provider. Every provider loaded with
`SafeEvpPKeyHandle.OpenKeyFromProvider` will create a context and load
that provider into it. The context and provider will remain alive for
the duration of the process.
The number of providers expected to be used in any application is
generally expected to be low - likely two or three.
1. The provider and context are now cached, and kept in a heap-backed
buffer. There is no resizing strategy or pre-allocated buffer. Opening a
new provider results in re-sizing the array. This strategy is likely
fine because there will rarely ever be more than a few providers, as
already noted. The resizing is managed by a mutex to synchronize access
to the array. We could use something like `pthread_rwlock_t` if we
wanted to increase complexity. Given that contention on this mutex would
only happen from `OpenKeyFromProvider`, I expect it to be rare.
2. Since the context and provider are now alive permanently, the "extra
handle" bookkeeping gets simpler. We no longer need to pass it to
destroy, and we no longer need to ref-count it. In fact we don't even
need a struct at all anymore, the OSSL_LIB_CTX _is_ the extra handle
now.
3. This reverts #129367 since the
data dependency is resolved.
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jul 18, 2026
Sign up for freeto 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.

4 participants

@vcsjones@jkotas@PranavSenthilnathan
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add ActiveIssue for all named key tests to mitigate invalid memory access - #129367

Merged
vcsjones merged 3 commits into
dotnet:mainfrom
vcsjones:active-issue-129339
Jun 13, 2026
Merged

Add ActiveIssue for all named key tests to mitigate invalid memory access#129367
vcsjones merged 3 commits into
dotnet:mainfrom
vcsjones:active-issue-129339

Conversation

@vcsjones

Copy link
Copy Markdown
Member

This adds an ActiveIssue to all functional members of OpenSslNamedKeysTests because they are very likely the source of memory corruption that is showing in #129339.

Disable the tests for now so that CI is stable while permanent fixes are investigated.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates OpenSslNamedKeysTests by adding ActiveIssue annotations to skip many of the named key functional tests while issue #129339 (segfault/memory corruption in System.Security.Cryptography.Tests) is investigated, aiming to keep CI stable.

Changes:

  • Added [ActiveIssue("https://github.com/dotnet/runtime/issues/129339")] to most test methods in OpenSslNamedKeysTests.manual.cs to disable them.
  • Left some tests in the same class still runnable (e.g., argument/URI validation tests), despite the stated intent to disable the suite.
Show a summary per file
FileDescription
src/libraries/System.Security.Cryptography/tests/OpenSslNamedKeysTests.manual.csAdds ActiveIssue skips to many named key tests to mitigate suspected CI instability from #129339.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 2

CopilotAI review requested due to automatic review settings June 13, 2026 14:19

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

@vcsjones
vcsjones enabled auto-merge (squash) June 13, 2026 17:06
@vcsjones

Copy link
Copy Markdown
MemberAuthor

/ba-g timeout on unrelated platform; test only changes to disable some problematic tests.

@vcsjones
vcsjones merged commit 9405cac into dotnet:mainJun 13, 2026
92 of 95 checks passed
@vcsjones
vcsjones deleted the active-issue-129339 branch June 13, 2026 18:46
vcsjones added a commit that referenced this pull request Jun 16, 2026
Freeing an OSSL_LIB_CTX is a somewhat complicated task. It registers
per-thread callbacks to mutate thread-local state. Other OpenSSL APIs,
like random number generation, also have thread-local state. In the case
of the random number generator, it registered a thread data destructor
to clean up random state when the thread is shutting down. This, in
turn, sees that the OSSL_LIB_CTX is associated with that thread.
If the OSSL_LIB_CTX is freed before the thread has shut down, the thread
dtor will access invalid memory.
OpenSSL requires you to call `OPENSSL_thread_stop_ex` on _all_ threads
before calling `OSSL_LIB_CTX_free` to unhook the context from the
thread.
This is generally not feasible for .NET.
As a solution, we are going to not free the context, and re-use it for
each provider. Every provider loaded with
`SafeEvpPKeyHandle.OpenKeyFromProvider` will create a context and load
that provider into it. The context and provider will remain alive for
the duration of the process.
The number of providers expected to be used in any application is
generally expected to be low - likely two or three.
1. The provider and context are now cached, and kept in a heap-backed
buffer. There is no resizing strategy or pre-allocated buffer. Opening a
new provider results in re-sizing the array. This strategy is likely
fine because there will rarely ever be more than a few providers, as
already noted. The resizing is managed by a mutex to synchronize access
to the array. We could use something like `pthread_rwlock_t` if we
wanted to increase complexity. Given that contention on this mutex would
only happen from `OpenKeyFromProvider`, I expect it to be rare.
2. Since the context and provider are now alive permanently, the "extra
handle" bookkeeping gets simpler. We no longer need to pass it to
destroy, and we no longer need to ref-count it. In fact we don't even
need a struct at all anymore, the OSSL_LIB_CTX _is_ the extra handle
now.
3. This reverts #129367 since the
data dependency is resolved.
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone Jun 17, 2026
eiriktsarpalis pushed a commit that referenced this pull request Jul 15, 2026
Freeing an OSSL_LIB_CTX is a somewhat complicated task. It registers
per-thread callbacks to mutate thread-local state. Other OpenSSL APIs,
like random number generation, also have thread-local state. In the case
of the random number generator, it registered a thread data destructor
to clean up random state when the thread is shutting down. This, in
turn, sees that the OSSL_LIB_CTX is associated with that thread.
If the OSSL_LIB_CTX is freed before the thread has shut down, the thread
dtor will access invalid memory.
OpenSSL requires you to call `OPENSSL_thread_stop_ex` on _all_ threads
before calling `OSSL_LIB_CTX_free` to unhook the context from the
thread.
This is generally not feasible for .NET.
As a solution, we are going to not free the context, and re-use it for
each provider. Every provider loaded with
`SafeEvpPKeyHandle.OpenKeyFromProvider` will create a context and load
that provider into it. The context and provider will remain alive for
the duration of the process.
The number of providers expected to be used in any application is
generally expected to be low - likely two or three.
1. The provider and context are now cached, and kept in a heap-backed
buffer. There is no resizing strategy or pre-allocated buffer. Opening a
new provider results in re-sizing the array. This strategy is likely
fine because there will rarely ever be more than a few providers, as
already noted. The resizing is managed by a mutex to synchronize access
to the array. We could use something like `pthread_rwlock_t` if we
wanted to increase complexity. Given that contention on this mutex would
only happen from `OpenKeyFromProvider`, I expect it to be rare.
2. Since the context and provider are now alive permanently, the "extra
handle" bookkeeping gets simpler. We no longer need to pass it to
destroy, and we no longer need to ref-count it. In fact we don't even
need a struct at all anymore, the OSSL_LIB_CTX _is_ the extra handle
now.
3. This reverts #129367 since the
data dependency is resolved.
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jul 18, 2026
Sign up for freeto 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.

4 participants

@vcsjones@jkotas@PranavSenthilnathan
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Add ActiveIssue for all named key tests to mitigate invalid memory access - #129367

Merged
vcsjones merged 3 commits into
dotnet:mainfrom
vcsjones:active-issue-129339
Jun 13, 2026
Merged

Add ActiveIssue for all named key tests to mitigate invalid memory access#129367
vcsjones merged 3 commits into
dotnet:mainfrom
vcsjones:active-issue-129339

Conversation

@vcsjones

Copy link
Copy Markdown
Member

This adds an ActiveIssue to all functional members of OpenSslNamedKeysTests because they are very likely the source of memory corruption that is showing in #129339.

Disable the tests for now so that CI is stable while permanent fixes are investigated.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates OpenSslNamedKeysTests by adding ActiveIssue annotations to skip many of the named key functional tests while issue #129339 (segfault/memory corruption in System.Security.Cryptography.Tests) is investigated, aiming to keep CI stable.

Changes:

  • Added [ActiveIssue("https://github.com/dotnet/runtime/issues/129339")] to most test methods in OpenSslNamedKeysTests.manual.cs to disable them.
  • Left some tests in the same class still runnable (e.g., argument/URI validation tests), despite the stated intent to disable the suite.
Show a summary per file
FileDescription
src/libraries/System.Security.Cryptography/tests/OpenSslNamedKeysTests.manual.csAdds ActiveIssue skips to many named key tests to mitigate suspected CI instability from #129339.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 2

CopilotAI review requested due to automatic review settings June 13, 2026 14:19

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

@vcsjones
vcsjones enabled auto-merge (squash) June 13, 2026 17:06
@vcsjones

Copy link
Copy Markdown
MemberAuthor

/ba-g timeout on unrelated platform; test only changes to disable some problematic tests.

@vcsjones
vcsjones merged commit 9405cac into dotnet:mainJun 13, 2026
92 of 95 checks passed
@vcsjones
vcsjones deleted the active-issue-129339 branch June 13, 2026 18:46
vcsjones added a commit that referenced this pull request Jun 16, 2026
Freeing an OSSL_LIB_CTX is a somewhat complicated task. It registers
per-thread callbacks to mutate thread-local state. Other OpenSSL APIs,
like random number generation, also have thread-local state. In the case
of the random number generator, it registered a thread data destructor
to clean up random state when the thread is shutting down. This, in
turn, sees that the OSSL_LIB_CTX is associated with that thread.
If the OSSL_LIB_CTX is freed before the thread has shut down, the thread
dtor will access invalid memory.
OpenSSL requires you to call `OPENSSL_thread_stop_ex` on _all_ threads
before calling `OSSL_LIB_CTX_free` to unhook the context from the
thread.
This is generally not feasible for .NET.
As a solution, we are going to not free the context, and re-use it for
each provider. Every provider loaded with
`SafeEvpPKeyHandle.OpenKeyFromProvider` will create a context and load
that provider into it. The context and provider will remain alive for
the duration of the process.
The number of providers expected to be used in any application is
generally expected to be low - likely two or three.
1. The provider and context are now cached, and kept in a heap-backed
buffer. There is no resizing strategy or pre-allocated buffer. Opening a
new provider results in re-sizing the array. This strategy is likely
fine because there will rarely ever be more than a few providers, as
already noted. The resizing is managed by a mutex to synchronize access
to the array. We could use something like `pthread_rwlock_t` if we
wanted to increase complexity. Given that contention on this mutex would
only happen from `OpenKeyFromProvider`, I expect it to be rare.
2. Since the context and provider are now alive permanently, the "extra
handle" bookkeeping gets simpler. We no longer need to pass it to
destroy, and we no longer need to ref-count it. In fact we don't even
need a struct at all anymore, the OSSL_LIB_CTX _is_ the extra handle
now.
3. This reverts #129367 since the
data dependency is resolved.
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone Jun 17, 2026
eiriktsarpalis pushed a commit that referenced this pull request Jul 15, 2026
Freeing an OSSL_LIB_CTX is a somewhat complicated task. It registers
per-thread callbacks to mutate thread-local state. Other OpenSSL APIs,
like random number generation, also have thread-local state. In the case
of the random number generator, it registered a thread data destructor
to clean up random state when the thread is shutting down. This, in
turn, sees that the OSSL_LIB_CTX is associated with that thread.
If the OSSL_LIB_CTX is freed before the thread has shut down, the thread
dtor will access invalid memory.
OpenSSL requires you to call `OPENSSL_thread_stop_ex` on _all_ threads
before calling `OSSL_LIB_CTX_free` to unhook the context from the
thread.
This is generally not feasible for .NET.
As a solution, we are going to not free the context, and re-use it for
each provider. Every provider loaded with
`SafeEvpPKeyHandle.OpenKeyFromProvider` will create a context and load
that provider into it. The context and provider will remain alive for
the duration of the process.
The number of providers expected to be used in any application is
generally expected to be low - likely two or three.
1. The provider and context are now cached, and kept in a heap-backed
buffer. There is no resizing strategy or pre-allocated buffer. Opening a
new provider results in re-sizing the array. This strategy is likely
fine because there will rarely ever be more than a few providers, as
already noted. The resizing is managed by a mutex to synchronize access
to the array. We could use something like `pthread_rwlock_t` if we
wanted to increase complexity. Given that contention on this mutex would
only happen from `OpenKeyFromProvider`, I expect it to be rare.
2. Since the context and provider are now alive permanently, the "extra
handle" bookkeeping gets simpler. We no longer need to pass it to
destroy, and we no longer need to ref-count it. In fact we don't even
need a struct at all anymore, the OSSL_LIB_CTX _is_ the extra handle
now.
3. This reverts #129367 since the
data dependency is resolved.
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jul 18, 2026
Sign up for freeto 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.

4 participants

@vcsjones@jkotas@PranavSenthilnathan
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add ActiveIssue for all named key tests to mitigate invalid memory access - #129367

Merged
vcsjones merged 3 commits into
dotnet:mainfrom
vcsjones:active-issue-129339
Jun 13, 2026
Merged

Add ActiveIssue for all named key tests to mitigate invalid memory access#129367
vcsjones merged 3 commits into
dotnet:mainfrom
vcsjones:active-issue-129339

Conversation

@vcsjones

Copy link
Copy Markdown
Member

This adds an ActiveIssue to all functional members of OpenSslNamedKeysTests because they are very likely the source of memory corruption that is showing in #129339.

Disable the tests for now so that CI is stable while permanent fixes are investigated.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates OpenSslNamedKeysTests by adding ActiveIssue annotations to skip many of the named key functional tests while issue #129339 (segfault/memory corruption in System.Security.Cryptography.Tests) is investigated, aiming to keep CI stable.

Changes:

  • Added [ActiveIssue("https://github.com/dotnet/runtime/issues/129339")] to most test methods in OpenSslNamedKeysTests.manual.cs to disable them.
  • Left some tests in the same class still runnable (e.g., argument/URI validation tests), despite the stated intent to disable the suite.
Show a summary per file
FileDescription
src/libraries/System.Security.Cryptography/tests/OpenSslNamedKeysTests.manual.csAdds ActiveIssue skips to many named key tests to mitigate suspected CI instability from #129339.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 2

CopilotAI review requested due to automatic review settings June 13, 2026 14:19

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

@vcsjones
vcsjones enabled auto-merge (squash) June 13, 2026 17:06
@vcsjones

Copy link
Copy Markdown
MemberAuthor

/ba-g timeout on unrelated platform; test only changes to disable some problematic tests.

@vcsjones
vcsjones merged commit 9405cac into dotnet:mainJun 13, 2026
92 of 95 checks passed
@vcsjones
vcsjones deleted the active-issue-129339 branch June 13, 2026 18:46
vcsjones added a commit that referenced this pull request Jun 16, 2026
Freeing an OSSL_LIB_CTX is a somewhat complicated task. It registers
per-thread callbacks to mutate thread-local state. Other OpenSSL APIs,
like random number generation, also have thread-local state. In the case
of the random number generator, it registered a thread data destructor
to clean up random state when the thread is shutting down. This, in
turn, sees that the OSSL_LIB_CTX is associated with that thread.
If the OSSL_LIB_CTX is freed before the thread has shut down, the thread
dtor will access invalid memory.
OpenSSL requires you to call `OPENSSL_thread_stop_ex` on _all_ threads
before calling `OSSL_LIB_CTX_free` to unhook the context from the
thread.
This is generally not feasible for .NET.
As a solution, we are going to not free the context, and re-use it for
each provider. Every provider loaded with
`SafeEvpPKeyHandle.OpenKeyFromProvider` will create a context and load
that provider into it. The context and provider will remain alive for
the duration of the process.
The number of providers expected to be used in any application is
generally expected to be low - likely two or three.
1. The provider and context are now cached, and kept in a heap-backed
buffer. There is no resizing strategy or pre-allocated buffer. Opening a
new provider results in re-sizing the array. This strategy is likely
fine because there will rarely ever be more than a few providers, as
already noted. The resizing is managed by a mutex to synchronize access
to the array. We could use something like `pthread_rwlock_t` if we
wanted to increase complexity. Given that contention on this mutex would
only happen from `OpenKeyFromProvider`, I expect it to be rare.
2. Since the context and provider are now alive permanently, the "extra
handle" bookkeeping gets simpler. We no longer need to pass it to
destroy, and we no longer need to ref-count it. In fact we don't even
need a struct at all anymore, the OSSL_LIB_CTX _is_ the extra handle
now.
3. This reverts #129367 since the
data dependency is resolved.
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone Jun 17, 2026
eiriktsarpalis pushed a commit that referenced this pull request Jul 15, 2026
Freeing an OSSL_LIB_CTX is a somewhat complicated task. It registers
per-thread callbacks to mutate thread-local state. Other OpenSSL APIs,
like random number generation, also have thread-local state. In the case
of the random number generator, it registered a thread data destructor
to clean up random state when the thread is shutting down. This, in
turn, sees that the OSSL_LIB_CTX is associated with that thread.
If the OSSL_LIB_CTX is freed before the thread has shut down, the thread
dtor will access invalid memory.
OpenSSL requires you to call `OPENSSL_thread_stop_ex` on _all_ threads
before calling `OSSL_LIB_CTX_free` to unhook the context from the
thread.
This is generally not feasible for .NET.
As a solution, we are going to not free the context, and re-use it for
each provider. Every provider loaded with
`SafeEvpPKeyHandle.OpenKeyFromProvider` will create a context and load
that provider into it. The context and provider will remain alive for
the duration of the process.
The number of providers expected to be used in any application is
generally expected to be low - likely two or three.
1. The provider and context are now cached, and kept in a heap-backed
buffer. There is no resizing strategy or pre-allocated buffer. Opening a
new provider results in re-sizing the array. This strategy is likely
fine because there will rarely ever be more than a few providers, as
already noted. The resizing is managed by a mutex to synchronize access
to the array. We could use something like `pthread_rwlock_t` if we
wanted to increase complexity. Given that contention on this mutex would
only happen from `OpenKeyFromProvider`, I expect it to be rare.
2. Since the context and provider are now alive permanently, the "extra
handle" bookkeeping gets simpler. We no longer need to pass it to
destroy, and we no longer need to ref-count it. In fact we don't even
need a struct at all anymore, the OSSL_LIB_CTX _is_ the extra handle
now.
3. This reverts #129367 since the
data dependency is resolved.
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jul 18, 2026
Sign up for freeto 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.

4 participants

@vcsjones@jkotas@PranavSenthilnathan
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add ActiveIssue for all named key tests to mitigate invalid memory access - #129367

Merged
vcsjones merged 3 commits into
dotnet:mainfrom
vcsjones:active-issue-129339
Jun 13, 2026
Merged

Add ActiveIssue for all named key tests to mitigate invalid memory access#129367
vcsjones merged 3 commits into
dotnet:mainfrom
vcsjones:active-issue-129339

Conversation

@vcsjones

Copy link
Copy Markdown
Member

This adds an ActiveIssue to all functional members of OpenSslNamedKeysTests because they are very likely the source of memory corruption that is showing in #129339.

Disable the tests for now so that CI is stable while permanent fixes are investigated.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates OpenSslNamedKeysTests by adding ActiveIssue annotations to skip many of the named key functional tests while issue #129339 (segfault/memory corruption in System.Security.Cryptography.Tests) is investigated, aiming to keep CI stable.

Changes:

  • Added [ActiveIssue("https://github.com/dotnet/runtime/issues/129339")] to most test methods in OpenSslNamedKeysTests.manual.cs to disable them.
  • Left some tests in the same class still runnable (e.g., argument/URI validation tests), despite the stated intent to disable the suite.
Show a summary per file
FileDescription
src/libraries/System.Security.Cryptography/tests/OpenSslNamedKeysTests.manual.csAdds ActiveIssue skips to many named key tests to mitigate suspected CI instability from #129339.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 2

CopilotAI review requested due to automatic review settings June 13, 2026 14:19

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

@vcsjones
vcsjones enabled auto-merge (squash) June 13, 2026 17:06
@vcsjones

Copy link
Copy Markdown
MemberAuthor

/ba-g timeout on unrelated platform; test only changes to disable some problematic tests.

@vcsjones
vcsjones merged commit 9405cac into dotnet:mainJun 13, 2026
92 of 95 checks passed
@vcsjones
vcsjones deleted the active-issue-129339 branch June 13, 2026 18:46
vcsjones added a commit that referenced this pull request Jun 16, 2026
Freeing an OSSL_LIB_CTX is a somewhat complicated task. It registers
per-thread callbacks to mutate thread-local state. Other OpenSSL APIs,
like random number generation, also have thread-local state. In the case
of the random number generator, it registered a thread data destructor
to clean up random state when the thread is shutting down. This, in
turn, sees that the OSSL_LIB_CTX is associated with that thread.
If the OSSL_LIB_CTX is freed before the thread has shut down, the thread
dtor will access invalid memory.
OpenSSL requires you to call `OPENSSL_thread_stop_ex` on _all_ threads
before calling `OSSL_LIB_CTX_free` to unhook the context from the
thread.
This is generally not feasible for .NET.
As a solution, we are going to not free the context, and re-use it for
each provider. Every provider loaded with
`SafeEvpPKeyHandle.OpenKeyFromProvider` will create a context and load
that provider into it. The context and provider will remain alive for
the duration of the process.
The number of providers expected to be used in any application is
generally expected to be low - likely two or three.
1. The provider and context are now cached, and kept in a heap-backed
buffer. There is no resizing strategy or pre-allocated buffer. Opening a
new provider results in re-sizing the array. This strategy is likely
fine because there will rarely ever be more than a few providers, as
already noted. The resizing is managed by a mutex to synchronize access
to the array. We could use something like `pthread_rwlock_t` if we
wanted to increase complexity. Given that contention on this mutex would
only happen from `OpenKeyFromProvider`, I expect it to be rare.
2. Since the context and provider are now alive permanently, the "extra
handle" bookkeeping gets simpler. We no longer need to pass it to
destroy, and we no longer need to ref-count it. In fact we don't even
need a struct at all anymore, the OSSL_LIB_CTX _is_ the extra handle
now.
3. This reverts #129367 since the
data dependency is resolved.
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone Jun 17, 2026
eiriktsarpalis pushed a commit that referenced this pull request Jul 15, 2026
Freeing an OSSL_LIB_CTX is a somewhat complicated task. It registers
per-thread callbacks to mutate thread-local state. Other OpenSSL APIs,
like random number generation, also have thread-local state. In the case
of the random number generator, it registered a thread data destructor
to clean up random state when the thread is shutting down. This, in
turn, sees that the OSSL_LIB_CTX is associated with that thread.
If the OSSL_LIB_CTX is freed before the thread has shut down, the thread
dtor will access invalid memory.
OpenSSL requires you to call `OPENSSL_thread_stop_ex` on _all_ threads
before calling `OSSL_LIB_CTX_free` to unhook the context from the
thread.
This is generally not feasible for .NET.
As a solution, we are going to not free the context, and re-use it for
each provider. Every provider loaded with
`SafeEvpPKeyHandle.OpenKeyFromProvider` will create a context and load
that provider into it. The context and provider will remain alive for
the duration of the process.
The number of providers expected to be used in any application is
generally expected to be low - likely two or three.
1. The provider and context are now cached, and kept in a heap-backed
buffer. There is no resizing strategy or pre-allocated buffer. Opening a
new provider results in re-sizing the array. This strategy is likely
fine because there will rarely ever be more than a few providers, as
already noted. The resizing is managed by a mutex to synchronize access
to the array. We could use something like `pthread_rwlock_t` if we
wanted to increase complexity. Given that contention on this mutex would
only happen from `OpenKeyFromProvider`, I expect it to be rare.
2. Since the context and provider are now alive permanently, the "extra
handle" bookkeeping gets simpler. We no longer need to pass it to
destroy, and we no longer need to ref-count it. In fact we don't even
need a struct at all anymore, the OSSL_LIB_CTX _is_ the extra handle
now.
3. This reverts #129367 since the
data dependency is resolved.
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jul 18, 2026
Sign up for freeto 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.

4 participants

@vcsjones@jkotas@PranavSenthilnathan
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Add ActiveIssue for all named key tests to mitigate invalid memory access - #129367

Merged
vcsjones merged 3 commits into
dotnet:mainfrom
vcsjones:active-issue-129339
Jun 13, 2026
Merged

Add ActiveIssue for all named key tests to mitigate invalid memory access#129367
vcsjones merged 3 commits into
dotnet:mainfrom
vcsjones:active-issue-129339

Conversation

@vcsjones

Copy link
Copy Markdown
Member

This adds an ActiveIssue to all functional members of OpenSslNamedKeysTests because they are very likely the source of memory corruption that is showing in #129339.

Disable the tests for now so that CI is stable while permanent fixes are investigated.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates OpenSslNamedKeysTests by adding ActiveIssue annotations to skip many of the named key functional tests while issue #129339 (segfault/memory corruption in System.Security.Cryptography.Tests) is investigated, aiming to keep CI stable.

Changes:

  • Added [ActiveIssue("https://github.com/dotnet/runtime/issues/129339")] to most test methods in OpenSslNamedKeysTests.manual.cs to disable them.
  • Left some tests in the same class still runnable (e.g., argument/URI validation tests), despite the stated intent to disable the suite.
Show a summary per file
FileDescription
src/libraries/System.Security.Cryptography/tests/OpenSslNamedKeysTests.manual.csAdds ActiveIssue skips to many named key tests to mitigate suspected CI instability from #129339.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 2

CopilotAI review requested due to automatic review settings June 13, 2026 14:19

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

@vcsjones
vcsjones enabled auto-merge (squash) June 13, 2026 17:06
@vcsjones

Copy link
Copy Markdown
MemberAuthor

/ba-g timeout on unrelated platform; test only changes to disable some problematic tests.

@vcsjones
vcsjones merged commit 9405cac into dotnet:mainJun 13, 2026
92 of 95 checks passed
@vcsjones
vcsjones deleted the active-issue-129339 branch June 13, 2026 18:46
vcsjones added a commit that referenced this pull request Jun 16, 2026
Freeing an OSSL_LIB_CTX is a somewhat complicated task. It registers
per-thread callbacks to mutate thread-local state. Other OpenSSL APIs,
like random number generation, also have thread-local state. In the case
of the random number generator, it registered a thread data destructor
to clean up random state when the thread is shutting down. This, in
turn, sees that the OSSL_LIB_CTX is associated with that thread.
If the OSSL_LIB_CTX is freed before the thread has shut down, the thread
dtor will access invalid memory.
OpenSSL requires you to call `OPENSSL_thread_stop_ex` on _all_ threads
before calling `OSSL_LIB_CTX_free` to unhook the context from the
thread.
This is generally not feasible for .NET.
As a solution, we are going to not free the context, and re-use it for
each provider. Every provider loaded with
`SafeEvpPKeyHandle.OpenKeyFromProvider` will create a context and load
that provider into it. The context and provider will remain alive for
the duration of the process.
The number of providers expected to be used in any application is
generally expected to be low - likely two or three.
1. The provider and context are now cached, and kept in a heap-backed
buffer. There is no resizing strategy or pre-allocated buffer. Opening a
new provider results in re-sizing the array. This strategy is likely
fine because there will rarely ever be more than a few providers, as
already noted. The resizing is managed by a mutex to synchronize access
to the array. We could use something like `pthread_rwlock_t` if we
wanted to increase complexity. Given that contention on this mutex would
only happen from `OpenKeyFromProvider`, I expect it to be rare.
2. Since the context and provider are now alive permanently, the "extra
handle" bookkeeping gets simpler. We no longer need to pass it to
destroy, and we no longer need to ref-count it. In fact we don't even
need a struct at all anymore, the OSSL_LIB_CTX _is_ the extra handle
now.
3. This reverts #129367 since the
data dependency is resolved.
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview6 milestone Jun 17, 2026
eiriktsarpalis pushed a commit that referenced this pull request Jul 15, 2026
Freeing an OSSL_LIB_CTX is a somewhat complicated task. It registers
per-thread callbacks to mutate thread-local state. Other OpenSSL APIs,
like random number generation, also have thread-local state. In the case
of the random number generator, it registered a thread data destructor
to clean up random state when the thread is shutting down. This, in
turn, sees that the OSSL_LIB_CTX is associated with that thread.
If the OSSL_LIB_CTX is freed before the thread has shut down, the thread
dtor will access invalid memory.
OpenSSL requires you to call `OPENSSL_thread_stop_ex` on _all_ threads
before calling `OSSL_LIB_CTX_free` to unhook the context from the
thread.
This is generally not feasible for .NET.
As a solution, we are going to not free the context, and re-use it for
each provider. Every provider loaded with
`SafeEvpPKeyHandle.OpenKeyFromProvider` will create a context and load
that provider into it. The context and provider will remain alive for
the duration of the process.
The number of providers expected to be used in any application is
generally expected to be low - likely two or three.
1. The provider and context are now cached, and kept in a heap-backed
buffer. There is no resizing strategy or pre-allocated buffer. Opening a
new provider results in re-sizing the array. This strategy is likely
fine because there will rarely ever be more than a few providers, as
already noted. The resizing is managed by a mutex to synchronize access
to the array. We could use something like `pthread_rwlock_t` if we
wanted to increase complexity. Given that contention on this mutex would
only happen from `OpenKeyFromProvider`, I expect it to be rare.
2. Since the context and provider are now alive permanently, the "extra
handle" bookkeeping gets simpler. We no longer need to pass it to
destroy, and we no longer need to ref-count it. In fact we don't even
need a struct at all anymore, the OSSL_LIB_CTX _is_ the extra handle
now.
3. This reverts #129367 since the
data dependency is resolved.
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jul 18, 2026
Sign up for freeto 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.

4 participants

@vcsjones@jkotas@PranavSenthilnathan