Improve allocations in NegotiateStreamPal - #71280

Merged
wfurt merged 5 commits into
dotnet:mainfrom
filipnavara:memorify-security-buffer
Jun 29, 2022
Merged

Improve allocations in NegotiateStreamPal#71280
wfurt merged 5 commits into
dotnet:mainfrom
filipnavara:memorify-security-buffer

Conversation

@filipnavara

@filipnavarafilipnavara commented Jun 25, 2022•
edited
Loading

Copy link
Copy Markdown
Member

This reduces allocations during NTLM/Negotiate authentication by reusing an existing buffer. It saves about 3Kb of allocated memory for a typical NTLM authentication exchange on Windows.

It also converts a couple of the internals to use Span/Memory. This would be a prerequisite for offering public API for encryption/decryption on NegotiateAuthentication class. As a side effect, it removes a big chunk of cumbersome interop marshaling on Windows.

@ghostghost added area-System.Net.Security community-contribution Indicates that the PR has been added by a community member labels Jun 25, 2022
@ghost

ghost commented Jun 25, 2022

Copy link
Copy Markdown

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

Issue Details

This reduces allocations during NTLM/Negotiate authentication by reusing an existing buffer. It saves about 3Kb of allocated memory for a typical NTLM authentication exchange.

It also converts a couple of the internals to use Span/Memory. This would be a prerequisite for offering public API for encryption/decryption on NegotiateAuthentication class. As a side effect, it removes a big chunk of cumbersome interop marshaling on Windows.

Author:filipnavara
Assignees:-
Labels:

area-System.Net.Security

Milestone:-

@filipnavara

filipnavara commented Jun 25, 2022•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

While refactoring the code I found a couple of bugs that I plan to fix in subsequent PRs:

  • Negotiate authentication on SMTP only works by happy coincidence, the last token in authentication is not processed correctly
  • Incorrect error code is used on the wire for NegotiateStream to indicate denied login. This happened during conversion to support multiple platforms
  • Some of the code in NegotiateStream is reading numbers using BitConverter which would produce incorrect results on big-endian machines

@filipnavara
filipnavaraforce-pushed the memorify-security-buffer branch from 52b3db7 to 1a2892eCompareJune 25, 2022 15:33
filipnavara
filipnavara commented Jun 25, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/Security/ReadWriteAdapter.cs Outdated
@filipnavara
filipnavaraforce-pushed the memorify-security-buffer branch from 1a2892e to 3b4aff6CompareJune 25, 2022 16:44
wfurt
wfurt reviewed Jun 27, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Unix/System.Net.Security.Native/Interop.NetSecurityNative.cs
out Status minorStatus,
SafeGssContextHandle? contextHandle,
byte[] inputBytes,
byte* inputBytes,

@wfurtwfurtJun 27, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need offset any more? It feels like if we pass pointer we can just do count.

@filipnavarafilipnavaraJun 27, 2022•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

We don't. Unfortunately, dotnet/sqlclient uses the native APIs, so I didn't feel confident in changing it. I am quite sure they don't use this particular API though.

@MichalPetrykaMichalPetrykaJun 27, 2022

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.

Wouldn't the pointer change by itself be breaking for them?

@filipnavarafilipnavaraJun 27, 2022•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It doesn't change the native code side, the prototype remains unchanged there. They basically copied the managed side of the interop. (We cannot change the signature, add parameters, remove parameters, or change their types. We can change how they are marshalled on the C# side though.)

@wfurtwfurtJun 27, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

cc: @JRahnama just in case.

@filipnavarafilipnavaraJun 29, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I cross-checked with the SqlClient source and this particular native method is not referenced so we can remove the offset parameter. I will do that in a follow-up PR (#71373) since I need to update the native interop there anyway.

wfurt
wfurt approved these changes Jun 27, 2022
View reviewed changes

@wfurtwfurt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice. Thanks @filipnavara

@filipnavarafilipnavara mentioned this pull request Jun 28, 2022
Merged
stephentoub
stephentoub reviewed Jun 28, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/System/Net/NTAuthentication.Common.cs
stephentoub
stephentoub reviewed Jun 28, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/System/Net/NTAuthentication.Common.cs Outdated
@filipnavara@stephentoub
Update src/libraries/Common/src/System/Net/NTAuthentication.Common.cs
46e05a4
Co-authored-by: Stephen Toub <stoub@microsoft.com>
@wfurt
wfurt merged commit 1e8eaef into dotnet:mainJun 29, 2022
@stephentoubstephentoub mentioned this pull request Jul 14, 2022
Closed
@karelzkarelz added this to the 7.0.0 milestone Jul 19, 2022
@ghostghost locked as resolved and limited conversation to collaborators Aug 18, 2022
@filipnavara
filipnavara deleted the memorify-security-buffer branch June 5, 2025 07:39
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@wfurtwfurtwfurt approved these changes
@stephentoubstephentoubstephentoub approved these changes
+1 more reviewer
@MichalPetrykaMichalPetrykaMichalPetryka left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Net.Securitycommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@filipnavara@stephentoub@wfurt@MichalPetryka@karelz
, '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
Improve allocations in NegotiateStreamPal - #71280
Merged
wfurt merged 5 commits into
dotnet:mainfrom 
filipnavara:memorify-security-buffer
Jun 29, 2022
Merged
Improve allocations in NegotiateStreamPal#71280
wfurt merged 5 commits into
dotnet:mainfrom 
filipnavara:memorify-security-buffer
Conversation
@filipnavara
@filipnavarafilipnavara
commented
Jun 25, 2022•

edited

Loading

Copy link


Copy Markdown

Member
This reduces allocations during NTLM/Negotiate authentication by reusing an existing buffer. It saves about 3Kb of allocated memory for a typical NTLM authentication exchange on Windows.
It also converts a couple of the internals to use Span/Memory. This would be a prerequisite for offering public API for encryption/decryption on NegotiateAuthentication class. As a side effect, it removes a big chunk of cumbersome interop marshaling on Windows.
@ghostghost
added

area-System.Net.Security

community-contribution
Indicates that the PR has been added by a community member
labels
Jun 25, 2022
@ghost
ghost
commented
Jun 25, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl, @vcsjones

See info in area-owners.md if you want to be subscribed.
Issue Details
This reduces allocations during NTLM/Negotiate authentication by reusing an existing buffer. It saves about 3Kb of allocated memory for a typical NTLM authentication exchange.
It also converts a couple of the internals to use Span/Memory. This would be a prerequisite for offering public API for encryption/decryption on NegotiateAuthentication class. As a side effect, it removes a big chunk of cumbersome interop marshaling on Windows.
Author:filipnavara
Assignees:-
Labels:
area-System.Net.Security
Milestone:-
@filipnavara
filipnavara
commented
Jun 25, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
While refactoring the code I found a couple of bugs that I plan to fix in subsequent PRs:
  • Negotiate authentication on SMTP only works by happy coincidence, the last token in authentication is not processed correctly
  • Incorrect error code is used on the wire for NegotiateStream to indicate denied login. This happened during conversion to support multiple platforms
  • Some of the code in NegotiateStream is reading numbers using BitConverter which would produce incorrect results on big-endian machines
@filipnavara
filipnavaraforce-pushed
the
memorify-security-buffer
branch
from
52b3db7 to
1a2892eCompareJune 25, 2022 15:33
filipnavara
filipnavara
commented
Jun 25, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/Security/ReadWriteAdapter.cs
Outdated

@filipnavara
filipnavaraforce-pushed
the
memorify-security-buffer
branch
from
1a2892e to
3b4aff6CompareJune 25, 2022 16:44
wfurt
wfurt
reviewed
Jun 27, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Unix/System.Net.Security.Native/Interop.NetSecurityNative.cs
 out Status minorStatus,
 SafeGssContextHandle? contextHandle,
 byte[] inputBytes,
 byte* inputBytes,
@wfurtwfurtJun 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Do we need offset any more? It feels like if we pass pointer we can just do count.
@filipnavarafilipnavaraJun 27, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

We don't. Unfortunately, dotnet/sqlclient uses the native APIs, so I didn't feel confident in changing it. I am quite sure they don't use this particular API though.
@MichalPetrykaMichalPetrykaJun 27, 2022

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.

Wouldn't the pointer change by itself be breaking for them?
@filipnavarafilipnavaraJun 27, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It doesn't change the native code side, the prototype remains unchanged there. They basically copied the managed side of the interop. (We cannot change the signature, add parameters, remove parameters, or change their types. We can change how they are marshalled on the C# side though.)
@wfurtwfurtJun 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

cc: @JRahnama just in case.
@filipnavarafilipnavaraJun 29, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I cross-checked with the SqlClient source and this particular native method is not referenced so we can remove the offset parameter. I will do that in a follow-up PR (#71373) since I need to update the native interop there anyway.
wfurt
wfurt
approved these changes
Jun 27, 2022
View reviewed changes
@wfurtwfurt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nice. Thanks @filipnavara
@filipnavarafilipnavara
mentioned this pull request
Jun 28, 2022
 Merged

stephentoub
stephentoub
reviewed
Jun 28, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/System/Net/NTAuthentication.Common.cs
stephentoub
stephentoub
reviewed
Jun 28, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/System/Net/NTAuthentication.Common.cs
Outdated

@filipnavara@stephentoub
Update src/libraries/Common/src/System/Net/NTAuthentication.Common.cs
46e05a4
Co-authored-by: Stephen Toub <stoub@microsoft.com>
@wfurt
wfurt
merged commit 1e8eaef
into
dotnet:mainJun 29, 2022
@stephentoubstephentoub
mentioned this pull request
Jul 14, 2022

Closed

@karelzkarelz
added this to the 7.0.0 milestone
Jul 19, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Aug 18, 2022
@filipnavara
filipnavara
deleted the

memorify-security-buffer

branch
June 5, 2025 07:39
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@wfurtwfurtwfurt approved these changes
@stephentoubstephentoubstephentoub approved these changes

+1 more reviewer

@MichalPetrykaMichalPetrykaMichalPetryka left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Securitycommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@filipnavara@stephentoub@wfurt@MichalPetryka@karelz

, '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
Improve allocations in NegotiateStreamPal - #71280
Merged
wfurt merged 5 commits into
dotnet:mainfrom 
filipnavara:memorify-security-buffer
Jun 29, 2022
Merged
Improve allocations in NegotiateStreamPal#71280
wfurt merged 5 commits into
dotnet:mainfrom 
filipnavara:memorify-security-buffer
Conversation
@filipnavara
@filipnavarafilipnavara
commented
Jun 25, 2022•

edited

Loading

Copy link


Copy Markdown

Member
This reduces allocations during NTLM/Negotiate authentication by reusing an existing buffer. It saves about 3Kb of allocated memory for a typical NTLM authentication exchange on Windows.
It also converts a couple of the internals to use Span/Memory. This would be a prerequisite for offering public API for encryption/decryption on NegotiateAuthentication class. As a side effect, it removes a big chunk of cumbersome interop marshaling on Windows.
@ghostghost
added

area-System.Net.Security

community-contribution
Indicates that the PR has been added by a community member
labels
Jun 25, 2022
@ghost
ghost
commented
Jun 25, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl, @vcsjones

See info in area-owners.md if you want to be subscribed.
Issue Details
This reduces allocations during NTLM/Negotiate authentication by reusing an existing buffer. It saves about 3Kb of allocated memory for a typical NTLM authentication exchange.
It also converts a couple of the internals to use Span/Memory. This would be a prerequisite for offering public API for encryption/decryption on NegotiateAuthentication class. As a side effect, it removes a big chunk of cumbersome interop marshaling on Windows.
Author:filipnavara
Assignees:-
Labels:
area-System.Net.Security
Milestone:-
@filipnavara
filipnavara
commented
Jun 25, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
While refactoring the code I found a couple of bugs that I plan to fix in subsequent PRs:
  • Negotiate authentication on SMTP only works by happy coincidence, the last token in authentication is not processed correctly
  • Incorrect error code is used on the wire for NegotiateStream to indicate denied login. This happened during conversion to support multiple platforms
  • Some of the code in NegotiateStream is reading numbers using BitConverter which would produce incorrect results on big-endian machines
@filipnavara
filipnavaraforce-pushed
the
memorify-security-buffer
branch
from
52b3db7 to
1a2892eCompareJune 25, 2022 15:33
filipnavara
filipnavara
commented
Jun 25, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/Security/ReadWriteAdapter.cs
Outdated

@filipnavara
filipnavaraforce-pushed
the
memorify-security-buffer
branch
from
1a2892e to
3b4aff6CompareJune 25, 2022 16:44
wfurt
wfurt
reviewed
Jun 27, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Unix/System.Net.Security.Native/Interop.NetSecurityNative.cs
 out Status minorStatus,
 SafeGssContextHandle? contextHandle,
 byte[] inputBytes,
 byte* inputBytes,
@wfurtwfurtJun 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Do we need offset any more? It feels like if we pass pointer we can just do count.
@filipnavarafilipnavaraJun 27, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

We don't. Unfortunately, dotnet/sqlclient uses the native APIs, so I didn't feel confident in changing it. I am quite sure they don't use this particular API though.
@MichalPetrykaMichalPetrykaJun 27, 2022

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.

Wouldn't the pointer change by itself be breaking for them?
@filipnavarafilipnavaraJun 27, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It doesn't change the native code side, the prototype remains unchanged there. They basically copied the managed side of the interop. (We cannot change the signature, add parameters, remove parameters, or change their types. We can change how they are marshalled on the C# side though.)
@wfurtwfurtJun 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

cc: @JRahnama just in case.
@filipnavarafilipnavaraJun 29, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I cross-checked with the SqlClient source and this particular native method is not referenced so we can remove the offset parameter. I will do that in a follow-up PR (#71373) since I need to update the native interop there anyway.
wfurt
wfurt
approved these changes
Jun 27, 2022
View reviewed changes
@wfurtwfurt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nice. Thanks @filipnavara
@filipnavarafilipnavara
mentioned this pull request
Jun 28, 2022
 Merged

stephentoub
stephentoub
reviewed
Jun 28, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/System/Net/NTAuthentication.Common.cs
stephentoub
stephentoub
reviewed
Jun 28, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/System/Net/NTAuthentication.Common.cs
Outdated

@filipnavara@stephentoub
Update src/libraries/Common/src/System/Net/NTAuthentication.Common.cs
46e05a4
Co-authored-by: Stephen Toub <stoub@microsoft.com>
@wfurt
wfurt
merged commit 1e8eaef
into
dotnet:mainJun 29, 2022
@stephentoubstephentoub
mentioned this pull request
Jul 14, 2022

Closed

@karelzkarelz
added this to the 7.0.0 milestone
Jul 19, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Aug 18, 2022
@filipnavara
filipnavara
deleted the

memorify-security-buffer

branch
June 5, 2025 07:39
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@wfurtwfurtwfurt approved these changes
@stephentoubstephentoubstephentoub approved these changes

+1 more reviewer

@MichalPetrykaMichalPetrykaMichalPetryka left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Securitycommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@filipnavara@stephentoub@wfurt@MichalPetryka@karelz

, '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
Improve allocations in NegotiateStreamPal - #71280
Merged
wfurt merged 5 commits into
dotnet:mainfrom 
filipnavara:memorify-security-buffer
Jun 29, 2022
Merged
Improve allocations in NegotiateStreamPal#71280
wfurt merged 5 commits into
dotnet:mainfrom 
filipnavara:memorify-security-buffer
Conversation
@filipnavara
@filipnavarafilipnavara
commented
Jun 25, 2022•

edited

Loading

Copy link


Copy Markdown

Member
This reduces allocations during NTLM/Negotiate authentication by reusing an existing buffer. It saves about 3Kb of allocated memory for a typical NTLM authentication exchange on Windows.
It also converts a couple of the internals to use Span/Memory. This would be a prerequisite for offering public API for encryption/decryption on NegotiateAuthentication class. As a side effect, it removes a big chunk of cumbersome interop marshaling on Windows.
@ghostghost
added

area-System.Net.Security

community-contribution
Indicates that the PR has been added by a community member
labels
Jun 25, 2022
@ghost
ghost
commented
Jun 25, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl, @vcsjones

See info in area-owners.md if you want to be subscribed.
Issue Details
This reduces allocations during NTLM/Negotiate authentication by reusing an existing buffer. It saves about 3Kb of allocated memory for a typical NTLM authentication exchange.
It also converts a couple of the internals to use Span/Memory. This would be a prerequisite for offering public API for encryption/decryption on NegotiateAuthentication class. As a side effect, it removes a big chunk of cumbersome interop marshaling on Windows.
Author:filipnavara
Assignees:-
Labels:
area-System.Net.Security
Milestone:-
@filipnavara
filipnavara
commented
Jun 25, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
While refactoring the code I found a couple of bugs that I plan to fix in subsequent PRs:
  • Negotiate authentication on SMTP only works by happy coincidence, the last token in authentication is not processed correctly
  • Incorrect error code is used on the wire for NegotiateStream to indicate denied login. This happened during conversion to support multiple platforms
  • Some of the code in NegotiateStream is reading numbers using BitConverter which would produce incorrect results on big-endian machines
@filipnavara
filipnavaraforce-pushed
the
memorify-security-buffer
branch
from
52b3db7 to
1a2892eCompareJune 25, 2022 15:33
filipnavara
filipnavara
commented
Jun 25, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/Security/ReadWriteAdapter.cs
Outdated

@filipnavara
filipnavaraforce-pushed
the
memorify-security-buffer
branch
from
1a2892e to
3b4aff6CompareJune 25, 2022 16:44
wfurt
wfurt
reviewed
Jun 27, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Unix/System.Net.Security.Native/Interop.NetSecurityNative.cs
 out Status minorStatus,
 SafeGssContextHandle? contextHandle,
 byte[] inputBytes,
 byte* inputBytes,
@wfurtwfurtJun 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Do we need offset any more? It feels like if we pass pointer we can just do count.
@filipnavarafilipnavaraJun 27, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

We don't. Unfortunately, dotnet/sqlclient uses the native APIs, so I didn't feel confident in changing it. I am quite sure they don't use this particular API though.
@MichalPetrykaMichalPetrykaJun 27, 2022

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.

Wouldn't the pointer change by itself be breaking for them?
@filipnavarafilipnavaraJun 27, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It doesn't change the native code side, the prototype remains unchanged there. They basically copied the managed side of the interop. (We cannot change the signature, add parameters, remove parameters, or change their types. We can change how they are marshalled on the C# side though.)
@wfurtwfurtJun 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

cc: @JRahnama just in case.
@filipnavarafilipnavaraJun 29, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I cross-checked with the SqlClient source and this particular native method is not referenced so we can remove the offset parameter. I will do that in a follow-up PR (#71373) since I need to update the native interop there anyway.
wfurt
wfurt
approved these changes
Jun 27, 2022
View reviewed changes
@wfurtwfurt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nice. Thanks @filipnavara
@filipnavarafilipnavara
mentioned this pull request
Jun 28, 2022
 Merged

stephentoub
stephentoub
reviewed
Jun 28, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/System/Net/NTAuthentication.Common.cs
stephentoub
stephentoub
reviewed
Jun 28, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/System/Net/NTAuthentication.Common.cs
Outdated

@filipnavara@stephentoub
Update src/libraries/Common/src/System/Net/NTAuthentication.Common.cs
46e05a4
Co-authored-by: Stephen Toub <stoub@microsoft.com>
@wfurt
wfurt
merged commit 1e8eaef
into
dotnet:mainJun 29, 2022
@stephentoubstephentoub
mentioned this pull request
Jul 14, 2022

Closed

@karelzkarelz
added this to the 7.0.0 milestone
Jul 19, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Aug 18, 2022
@filipnavara
filipnavara
deleted the

memorify-security-buffer

branch
June 5, 2025 07:39
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@wfurtwfurtwfurt approved these changes
@stephentoubstephentoubstephentoub approved these changes

+1 more reviewer

@MichalPetrykaMichalPetrykaMichalPetryka left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Securitycommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@filipnavara@stephentoub@wfurt@MichalPetryka@karelz

, '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
Improve allocations in NegotiateStreamPal - #71280
Merged
wfurt merged 5 commits into
dotnet:mainfrom 
filipnavara:memorify-security-buffer
Jun 29, 2022
Merged
Improve allocations in NegotiateStreamPal#71280
wfurt merged 5 commits into
dotnet:mainfrom 
filipnavara:memorify-security-buffer
Conversation
@filipnavara
@filipnavarafilipnavara
commented
Jun 25, 2022•

edited

Loading

Copy link


Copy Markdown

Member
This reduces allocations during NTLM/Negotiate authentication by reusing an existing buffer. It saves about 3Kb of allocated memory for a typical NTLM authentication exchange on Windows.
It also converts a couple of the internals to use Span/Memory. This would be a prerequisite for offering public API for encryption/decryption on NegotiateAuthentication class. As a side effect, it removes a big chunk of cumbersome interop marshaling on Windows.
@ghostghost
added

area-System.Net.Security

community-contribution
Indicates that the PR has been added by a community member
labels
Jun 25, 2022
@ghost
ghost
commented
Jun 25, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl, @vcsjones

See info in area-owners.md if you want to be subscribed.
Issue Details
This reduces allocations during NTLM/Negotiate authentication by reusing an existing buffer. It saves about 3Kb of allocated memory for a typical NTLM authentication exchange.
It also converts a couple of the internals to use Span/Memory. This would be a prerequisite for offering public API for encryption/decryption on NegotiateAuthentication class. As a side effect, it removes a big chunk of cumbersome interop marshaling on Windows.
Author:filipnavara
Assignees:-
Labels:
area-System.Net.Security
Milestone:-
@filipnavara
filipnavara
commented
Jun 25, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
While refactoring the code I found a couple of bugs that I plan to fix in subsequent PRs:
  • Negotiate authentication on SMTP only works by happy coincidence, the last token in authentication is not processed correctly
  • Incorrect error code is used on the wire for NegotiateStream to indicate denied login. This happened during conversion to support multiple platforms
  • Some of the code in NegotiateStream is reading numbers using BitConverter which would produce incorrect results on big-endian machines
@filipnavara
filipnavaraforce-pushed
the
memorify-security-buffer
branch
from
52b3db7 to
1a2892eCompareJune 25, 2022 15:33
filipnavara
filipnavara
commented
Jun 25, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/Security/ReadWriteAdapter.cs
Outdated

@filipnavara
filipnavaraforce-pushed
the
memorify-security-buffer
branch
from
1a2892e to
3b4aff6CompareJune 25, 2022 16:44
wfurt
wfurt
reviewed
Jun 27, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Unix/System.Net.Security.Native/Interop.NetSecurityNative.cs
 out Status minorStatus,
 SafeGssContextHandle? contextHandle,
 byte[] inputBytes,
 byte* inputBytes,
@wfurtwfurtJun 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Do we need offset any more? It feels like if we pass pointer we can just do count.
@filipnavarafilipnavaraJun 27, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

We don't. Unfortunately, dotnet/sqlclient uses the native APIs, so I didn't feel confident in changing it. I am quite sure they don't use this particular API though.
@MichalPetrykaMichalPetrykaJun 27, 2022

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.

Wouldn't the pointer change by itself be breaking for them?
@filipnavarafilipnavaraJun 27, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It doesn't change the native code side, the prototype remains unchanged there. They basically copied the managed side of the interop. (We cannot change the signature, add parameters, remove parameters, or change their types. We can change how they are marshalled on the C# side though.)
@wfurtwfurtJun 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

cc: @JRahnama just in case.
@filipnavarafilipnavaraJun 29, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I cross-checked with the SqlClient source and this particular native method is not referenced so we can remove the offset parameter. I will do that in a follow-up PR (#71373) since I need to update the native interop there anyway.
wfurt
wfurt
approved these changes
Jun 27, 2022
View reviewed changes
@wfurtwfurt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nice. Thanks @filipnavara
@filipnavarafilipnavara
mentioned this pull request
Jun 28, 2022
 Merged

stephentoub
stephentoub
reviewed
Jun 28, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/System/Net/NTAuthentication.Common.cs
stephentoub
stephentoub
reviewed
Jun 28, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/System/Net/NTAuthentication.Common.cs
Outdated

@filipnavara@stephentoub
Update src/libraries/Common/src/System/Net/NTAuthentication.Common.cs
46e05a4
Co-authored-by: Stephen Toub <stoub@microsoft.com>
@wfurt
wfurt
merged commit 1e8eaef
into
dotnet:mainJun 29, 2022
@stephentoubstephentoub
mentioned this pull request
Jul 14, 2022

Closed

@karelzkarelz
added this to the 7.0.0 milestone
Jul 19, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Aug 18, 2022
@filipnavara
filipnavara
deleted the

memorify-security-buffer

branch
June 5, 2025 07:39
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@wfurtwfurtwfurt approved these changes
@stephentoubstephentoubstephentoub approved these changes

+1 more reviewer

@MichalPetrykaMichalPetrykaMichalPetryka left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Securitycommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@filipnavara@stephentoub@wfurt@MichalPetryka@karelz

, '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
Improve allocations in NegotiateStreamPal - #71280
Merged
wfurt merged 5 commits into
dotnet:mainfrom 
filipnavara:memorify-security-buffer
Jun 29, 2022
Merged
Improve allocations in NegotiateStreamPal#71280
wfurt merged 5 commits into
dotnet:mainfrom 
filipnavara:memorify-security-buffer
Conversation
@filipnavara
@filipnavarafilipnavara
commented
Jun 25, 2022•

edited

Loading

Copy link


Copy Markdown

Member
This reduces allocations during NTLM/Negotiate authentication by reusing an existing buffer. It saves about 3Kb of allocated memory for a typical NTLM authentication exchange on Windows.
It also converts a couple of the internals to use Span/Memory. This would be a prerequisite for offering public API for encryption/decryption on NegotiateAuthentication class. As a side effect, it removes a big chunk of cumbersome interop marshaling on Windows.
@ghostghost
added

area-System.Net.Security

community-contribution
Indicates that the PR has been added by a community member
labels
Jun 25, 2022
@ghost
ghost
commented
Jun 25, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl, @vcsjones

See info in area-owners.md if you want to be subscribed.
Issue Details
This reduces allocations during NTLM/Negotiate authentication by reusing an existing buffer. It saves about 3Kb of allocated memory for a typical NTLM authentication exchange.
It also converts a couple of the internals to use Span/Memory. This would be a prerequisite for offering public API for encryption/decryption on NegotiateAuthentication class. As a side effect, it removes a big chunk of cumbersome interop marshaling on Windows.
Author:filipnavara
Assignees:-
Labels:
area-System.Net.Security
Milestone:-
@filipnavara
filipnavara
commented
Jun 25, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
While refactoring the code I found a couple of bugs that I plan to fix in subsequent PRs:
  • Negotiate authentication on SMTP only works by happy coincidence, the last token in authentication is not processed correctly
  • Incorrect error code is used on the wire for NegotiateStream to indicate denied login. This happened during conversion to support multiple platforms
  • Some of the code in NegotiateStream is reading numbers using BitConverter which would produce incorrect results on big-endian machines
@filipnavara
filipnavaraforce-pushed
the
memorify-security-buffer
branch
from
52b3db7 to
1a2892eCompareJune 25, 2022 15:33
filipnavara
filipnavara
commented
Jun 25, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/Security/ReadWriteAdapter.cs
Outdated

@filipnavara
filipnavaraforce-pushed
the
memorify-security-buffer
branch
from
1a2892e to
3b4aff6CompareJune 25, 2022 16:44
wfurt
wfurt
reviewed
Jun 27, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Unix/System.Net.Security.Native/Interop.NetSecurityNative.cs
 out Status minorStatus,
 SafeGssContextHandle? contextHandle,
 byte[] inputBytes,
 byte* inputBytes,
@wfurtwfurtJun 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Do we need offset any more? It feels like if we pass pointer we can just do count.
@filipnavarafilipnavaraJun 27, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

We don't. Unfortunately, dotnet/sqlclient uses the native APIs, so I didn't feel confident in changing it. I am quite sure they don't use this particular API though.
@MichalPetrykaMichalPetrykaJun 27, 2022

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.

Wouldn't the pointer change by itself be breaking for them?
@filipnavarafilipnavaraJun 27, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It doesn't change the native code side, the prototype remains unchanged there. They basically copied the managed side of the interop. (We cannot change the signature, add parameters, remove parameters, or change their types. We can change how they are marshalled on the C# side though.)
@wfurtwfurtJun 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

cc: @JRahnama just in case.
@filipnavarafilipnavaraJun 29, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I cross-checked with the SqlClient source and this particular native method is not referenced so we can remove the offset parameter. I will do that in a follow-up PR (#71373) since I need to update the native interop there anyway.
wfurt
wfurt
approved these changes
Jun 27, 2022
View reviewed changes
@wfurtwfurt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nice. Thanks @filipnavara
@filipnavarafilipnavara
mentioned this pull request
Jun 28, 2022
 Merged

stephentoub
stephentoub
reviewed
Jun 28, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/System/Net/NTAuthentication.Common.cs
stephentoub
stephentoub
reviewed
Jun 28, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/System/Net/NTAuthentication.Common.cs
Outdated

@filipnavara@stephentoub
Update src/libraries/Common/src/System/Net/NTAuthentication.Common.cs
46e05a4
Co-authored-by: Stephen Toub <stoub@microsoft.com>
@wfurt
wfurt
merged commit 1e8eaef
into
dotnet:mainJun 29, 2022
@stephentoubstephentoub
mentioned this pull request
Jul 14, 2022

Closed

@karelzkarelz
added this to the 7.0.0 milestone
Jul 19, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Aug 18, 2022
@filipnavara
filipnavara
deleted the

memorify-security-buffer

branch
June 5, 2025 07:39
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@wfurtwfurtwfurt approved these changes
@stephentoubstephentoubstephentoub approved these changes

+1 more reviewer

@MichalPetrykaMichalPetrykaMichalPetryka left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Securitycommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@filipnavara@stephentoub@wfurt@MichalPetryka@karelz

, '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
Improve allocations in NegotiateStreamPal - #71280
Merged
wfurt merged 5 commits into
dotnet:mainfrom 
filipnavara:memorify-security-buffer
Jun 29, 2022
Merged
Improve allocations in NegotiateStreamPal#71280
wfurt merged 5 commits into
dotnet:mainfrom 
filipnavara:memorify-security-buffer
Conversation
@filipnavara
@filipnavarafilipnavara
commented
Jun 25, 2022•

edited

Loading

Copy link


Copy Markdown

Member
This reduces allocations during NTLM/Negotiate authentication by reusing an existing buffer. It saves about 3Kb of allocated memory for a typical NTLM authentication exchange on Windows.
It also converts a couple of the internals to use Span/Memory. This would be a prerequisite for offering public API for encryption/decryption on NegotiateAuthentication class. As a side effect, it removes a big chunk of cumbersome interop marshaling on Windows.
@ghostghost
added

area-System.Net.Security

community-contribution
Indicates that the PR has been added by a community member
labels
Jun 25, 2022
@ghost
ghost
commented
Jun 25, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl, @vcsjones

See info in area-owners.md if you want to be subscribed.
Issue Details
This reduces allocations during NTLM/Negotiate authentication by reusing an existing buffer. It saves about 3Kb of allocated memory for a typical NTLM authentication exchange.
It also converts a couple of the internals to use Span/Memory. This would be a prerequisite for offering public API for encryption/decryption on NegotiateAuthentication class. As a side effect, it removes a big chunk of cumbersome interop marshaling on Windows.
Author:filipnavara
Assignees:-
Labels:
area-System.Net.Security
Milestone:-
@filipnavara
filipnavara
commented
Jun 25, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
While refactoring the code I found a couple of bugs that I plan to fix in subsequent PRs:
  • Negotiate authentication on SMTP only works by happy coincidence, the last token in authentication is not processed correctly
  • Incorrect error code is used on the wire for NegotiateStream to indicate denied login. This happened during conversion to support multiple platforms
  • Some of the code in NegotiateStream is reading numbers using BitConverter which would produce incorrect results on big-endian machines
@filipnavara
filipnavaraforce-pushed
the
memorify-security-buffer
branch
from
52b3db7 to
1a2892eCompareJune 25, 2022 15:33
filipnavara
filipnavara
commented
Jun 25, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/Security/ReadWriteAdapter.cs
Outdated

@filipnavara
filipnavaraforce-pushed
the
memorify-security-buffer
branch
from
1a2892e to
3b4aff6CompareJune 25, 2022 16:44
wfurt
wfurt
reviewed
Jun 27, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Unix/System.Net.Security.Native/Interop.NetSecurityNative.cs
 out Status minorStatus,
 SafeGssContextHandle? contextHandle,
 byte[] inputBytes,
 byte* inputBytes,
@wfurtwfurtJun 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Do we need offset any more? It feels like if we pass pointer we can just do count.
@filipnavarafilipnavaraJun 27, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

We don't. Unfortunately, dotnet/sqlclient uses the native APIs, so I didn't feel confident in changing it. I am quite sure they don't use this particular API though.
@MichalPetrykaMichalPetrykaJun 27, 2022

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.

Wouldn't the pointer change by itself be breaking for them?
@filipnavarafilipnavaraJun 27, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It doesn't change the native code side, the prototype remains unchanged there. They basically copied the managed side of the interop. (We cannot change the signature, add parameters, remove parameters, or change their types. We can change how they are marshalled on the C# side though.)
@wfurtwfurtJun 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

cc: @JRahnama just in case.
@filipnavarafilipnavaraJun 29, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I cross-checked with the SqlClient source and this particular native method is not referenced so we can remove the offset parameter. I will do that in a follow-up PR (#71373) since I need to update the native interop there anyway.
wfurt
wfurt
approved these changes
Jun 27, 2022
View reviewed changes
@wfurtwfurt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nice. Thanks @filipnavara
@filipnavarafilipnavara
mentioned this pull request
Jun 28, 2022
 Merged

stephentoub
stephentoub
reviewed
Jun 28, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/System/Net/NTAuthentication.Common.cs
stephentoub
stephentoub
reviewed
Jun 28, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/System/Net/NTAuthentication.Common.cs
Outdated

@filipnavara@stephentoub
Update src/libraries/Common/src/System/Net/NTAuthentication.Common.cs
46e05a4
Co-authored-by: Stephen Toub <stoub@microsoft.com>
@wfurt
wfurt
merged commit 1e8eaef
into
dotnet:mainJun 29, 2022
@stephentoubstephentoub
mentioned this pull request
Jul 14, 2022

Closed

@karelzkarelz
added this to the 7.0.0 milestone
Jul 19, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Aug 18, 2022
@filipnavara
filipnavara
deleted the

memorify-security-buffer

branch
June 5, 2025 07:39
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@wfurtwfurtwfurt approved these changes
@stephentoubstephentoubstephentoub approved these changes

+1 more reviewer

@MichalPetrykaMichalPetrykaMichalPetryka left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Securitycommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@filipnavara@stephentoub@wfurt@MichalPetryka@karelz

, '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
Improve allocations in NegotiateStreamPal - #71280
Merged
wfurt merged 5 commits into
dotnet:mainfrom 
filipnavara:memorify-security-buffer
Jun 29, 2022
Merged
Improve allocations in NegotiateStreamPal#71280
wfurt merged 5 commits into
dotnet:mainfrom 
filipnavara:memorify-security-buffer
Conversation
@filipnavara
@filipnavarafilipnavara
commented
Jun 25, 2022•

edited

Loading

Copy link


Copy Markdown

Member
This reduces allocations during NTLM/Negotiate authentication by reusing an existing buffer. It saves about 3Kb of allocated memory for a typical NTLM authentication exchange on Windows.
It also converts a couple of the internals to use Span/Memory. This would be a prerequisite for offering public API for encryption/decryption on NegotiateAuthentication class. As a side effect, it removes a big chunk of cumbersome interop marshaling on Windows.
@ghostghost
added

area-System.Net.Security

community-contribution
Indicates that the PR has been added by a community member
labels
Jun 25, 2022
@ghost
ghost
commented
Jun 25, 2022

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/ncl, @vcsjones

See info in area-owners.md if you want to be subscribed.
Issue Details
This reduces allocations during NTLM/Negotiate authentication by reusing an existing buffer. It saves about 3Kb of allocated memory for a typical NTLM authentication exchange.
It also converts a couple of the internals to use Span/Memory. This would be a prerequisite for offering public API for encryption/decryption on NegotiateAuthentication class. As a side effect, it removes a big chunk of cumbersome interop marshaling on Windows.
Author:filipnavara
Assignees:-
Labels:
area-System.Net.Security
Milestone:-
@filipnavara
filipnavara
commented
Jun 25, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
While refactoring the code I found a couple of bugs that I plan to fix in subsequent PRs:
  • Negotiate authentication on SMTP only works by happy coincidence, the last token in authentication is not processed correctly
  • Incorrect error code is used on the wire for NegotiateStream to indicate denied login. This happened during conversion to support multiple platforms
  • Some of the code in NegotiateStream is reading numbers using BitConverter which would produce incorrect results on big-endian machines
@filipnavara
filipnavaraforce-pushed
the
memorify-security-buffer
branch
from
52b3db7 to
1a2892eCompareJune 25, 2022 15:33
filipnavara
filipnavara
commented
Jun 25, 2022
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/Security/ReadWriteAdapter.cs
Outdated

@filipnavara
filipnavaraforce-pushed
the
memorify-security-buffer
branch
from
1a2892e to
3b4aff6CompareJune 25, 2022 16:44
wfurt
wfurt
reviewed
Jun 27, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Unix/System.Net.Security.Native/Interop.NetSecurityNative.cs
 out Status minorStatus,
 SafeGssContextHandle? contextHandle,
 byte[] inputBytes,
 byte* inputBytes,
@wfurtwfurtJun 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Do we need offset any more? It feels like if we pass pointer we can just do count.
@filipnavarafilipnavaraJun 27, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

We don't. Unfortunately, dotnet/sqlclient uses the native APIs, so I didn't feel confident in changing it. I am quite sure they don't use this particular API though.
@MichalPetrykaMichalPetrykaJun 27, 2022

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.

Wouldn't the pointer change by itself be breaking for them?
@filipnavarafilipnavaraJun 27, 2022•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

It doesn't change the native code side, the prototype remains unchanged there. They basically copied the managed side of the interop. (We cannot change the signature, add parameters, remove parameters, or change their types. We can change how they are marshalled on the C# side though.)
@wfurtwfurtJun 27, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

cc: @JRahnama just in case.
@filipnavarafilipnavaraJun 29, 2022

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I cross-checked with the SqlClient source and this particular native method is not referenced so we can remove the offset parameter. I will do that in a follow-up PR (#71373) since I need to update the native interop there anyway.
wfurt
wfurt
approved these changes
Jun 27, 2022
View reviewed changes
@wfurtwfurt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nice. Thanks @filipnavara
@filipnavarafilipnavara
mentioned this pull request
Jun 28, 2022
 Merged

stephentoub
stephentoub
reviewed
Jun 28, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/System/Net/NTAuthentication.Common.cs
stephentoub
stephentoub
reviewed
Jun 28, 2022
View reviewed changes
Comment threadsrc/libraries/Common/src/System/Net/NTAuthentication.Common.cs
Outdated

@filipnavara@stephentoub
Update src/libraries/Common/src/System/Net/NTAuthentication.Common.cs
46e05a4
Co-authored-by: Stephen Toub <stoub@microsoft.com>
@wfurt
wfurt
merged commit 1e8eaef
into
dotnet:mainJun 29, 2022
@stephentoubstephentoub
mentioned this pull request
Jul 14, 2022

Closed

@karelzkarelz
added this to the 7.0.0 milestone
Jul 19, 2022
@ghostghost
locked as resolved and limited conversation to collaborators
Aug 18, 2022
@filipnavara
filipnavara
deleted the

memorify-security-buffer

branch
June 5, 2025 07:39
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@wfurtwfurtwfurt approved these changes
@stephentoubstephentoubstephentoub approved these changes

+1 more reviewer

@MichalPetrykaMichalPetrykaMichalPetryka left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Securitycommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@filipnavara@stephentoub@wfurt@MichalPetryka@karelz