-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improve efficiency and correctness of SSL handshake #1949
Changes from all commits
181ea8d
8157f40
1e476b6
5d90789
a2ec343
b3a89db
2aeb19b
1fcd5b3
4647886
b57f102
c10ce1f
a93072d
4ef1590
253a61f
56caff2
322b853
7ee46a8
e1cc5f7
71caee3
dc68696
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -396,7 +396,7 @@ internal static unsafe int InitializeSecurityContext( | |
string targetName, | ||
Interop.SspiCli.ContextFlags inFlags, | ||
Interop.SspiCli.Endianness endianness, | ||
ReadOnlySpan<SecurityBuffer> inSecBuffers, | ||
InputSecurityBuffers inSecBuffers, | ||
ref SecurityBuffer outSecBuffer, | ||
ref Interop.SspiCli.ContextFlags outFlags) | ||
{ | ||
|
@@ -413,7 +413,7 @@ internal static unsafe int InitializeSecurityContext( | |
throw new ArgumentNullException(nameof(inCredentials)); | ||
} | ||
|
||
Interop.SspiCli.SecBufferDesc inSecurityBufferDescriptor = new Interop.SspiCli.SecBufferDesc(inSecBuffers.Length); | ||
Interop.SspiCli.SecBufferDesc inSecurityBufferDescriptor = new Interop.SspiCli.SecBufferDesc(inSecBuffers.Count); | ||
Interop.SspiCli.SecBufferDesc outSecurityBufferDescriptor = new Interop.SspiCli.SecBufferDesc(1); | ||
|
||
// Actually, this is returned in outFlags. | ||
|
@@ -431,34 +431,38 @@ internal static unsafe int InitializeSecurityContext( | |
SafeFreeContextBuffer outFreeContextBuffer = null; | ||
try | ||
{ | ||
Span<Interop.SspiCli.SecBuffer> inUnmanagedBuffer = stackalloc Interop.SspiCli.SecBuffer[inSecurityBufferDescriptor.cBuffers]; | ||
inUnmanagedBuffer.Clear(); | ||
// Allocate always maximum to allow better code optimization. | ||
Span<Interop.SspiCli.SecBuffer> inUnmanagedBuffer = stackalloc Interop.SspiCli.SecBuffer[3]; | ||
for (int index = 0; index < inSecBuffers.Count; ++index) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we assert anything about the relationship between inSecBuffers.Count and 3? |
||
{ | ||
inUnmanagedBuffer[index].BufferType = inSecBuffers.GetBuffer(index).Type; | ||
inUnmanagedBuffer[index].cbBuffer = inSecBuffers.GetBuffer(index).Token.Length; | ||
inUnmanagedBuffer[index].pvBuffer = inSecBuffers.GetBuffer(index).UnmanagedToken != null ? | ||
(IntPtr)inSecBuffers.GetBuffer(index).UnmanagedToken.DangerousGetHandle() : | ||
IntPtr.Zero; | ||
} | ||
|
||
fixed (void* inUnmanagedBufferPtr = inUnmanagedBuffer) | ||
fixed (void* pinnedToken0 = inSecBuffers.Length > 0 ? inSecBuffers[0].token : null) | ||
fixed (void* pinnedToken1 = inSecBuffers.Length > 1 ? inSecBuffers[1].token : null) | ||
fixed (void* pinnedToken2 = inSecBuffers.Length > 2 ? inSecBuffers[2].token : null) // pin all buffers, even if null or not used, to avoid needing to allocate GCHandles | ||
fixed (void* pinnedToken0 = inSecBuffers.GetBuffer(0).Token) | ||
fixed (void* pinnedToken1 = inSecBuffers.GetBuffer(1).Token) | ||
fixed (void* pinnedToken2 = inSecBuffers.GetBuffer(2).Token) | ||
{ | ||
Debug.Assert(inSecBuffers.Length <= 3); | ||
|
||
// Fix Descriptor pointer that points to unmanaged SecurityBuffers. | ||
inSecurityBufferDescriptor.pBuffers = inUnmanagedBufferPtr; | ||
for (int index = 0; index < inSecurityBufferDescriptor.cBuffers; ++index) | ||
// Updated pvBuffer with pinned address. UnmanagedToken takes precedence. | ||
if (inSecBuffers.Count > 2 && inUnmanagedBuffer[2].pvBuffer == IntPtr.Zero) | ||
{ | ||
ref readonly SecurityBuffer securityBuffer = ref inSecBuffers[index]; | ||
inUnmanagedBuffer[2].pvBuffer = (IntPtr)pinnedToken2; | ||
} | ||
|
||
// Copy the SecurityBuffer content into unmanaged place holder. | ||
inUnmanagedBuffer[index].cbBuffer = securityBuffer.size; | ||
inUnmanagedBuffer[index].BufferType = securityBuffer.type; | ||
|
||
// Use the unmanaged token if it's not null; otherwise use the managed buffer. | ||
inUnmanagedBuffer[index].pvBuffer = | ||
securityBuffer.unmanagedToken != null ? securityBuffer.unmanagedToken.DangerousGetHandle() : | ||
securityBuffer.token == null || securityBuffer.token.Length == 0 ? IntPtr.Zero : | ||
Marshal.UnsafeAddrOfPinnedArrayElement(securityBuffer.token, securityBuffer.offset); | ||
#if TRACE_VERBOSE | ||
if (NetEventSource.IsEnabled) NetEventSource.Info(null, $"SecBuffer: cbBuffer:{securityBuffer.size} BufferType:{securityBuffer.type}"); | ||
#endif | ||
if (inSecBuffers.Count > 1 && inUnmanagedBuffer[1].pvBuffer == IntPtr.Zero) | ||
{ | ||
inUnmanagedBuffer[1].pvBuffer = (IntPtr)pinnedToken1; | ||
} | ||
|
||
if (inSecBuffers.Count > 0 && inUnmanagedBuffer[0].pvBuffer == IntPtr.Zero) | ||
{ | ||
inUnmanagedBuffer[0].pvBuffer = (IntPtr)pinnedToken0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it be easier to move the earlier block that created and filled in the span to here? They wouldn't need to first assign IntPtr.Zero and then check for IntPtr.Zero, you'd just assign the right value to begin with (either the UnmanagedToken's handle or the pinned pointer). |
||
} | ||
|
||
fixed (byte* pinnedOutBytes = outSecBuffer.token) | ||
|
@@ -626,7 +630,7 @@ internal static unsafe int AcceptSecurityContext( | |
ref SafeDeleteSslContext refContext, | ||
Interop.SspiCli.ContextFlags inFlags, | ||
Interop.SspiCli.Endianness endianness, | ||
ReadOnlySpan<SecurityBuffer> inSecBuffers, | ||
InputSecurityBuffers inSecBuffers, | ||
ref SecurityBuffer outSecBuffer, | ||
ref Interop.SspiCli.ContextFlags outFlags) | ||
{ | ||
|
@@ -643,7 +647,8 @@ internal static unsafe int AcceptSecurityContext( | |
throw new ArgumentNullException(nameof(inCredentials)); | ||
} | ||
|
||
Interop.SspiCli.SecBufferDesc inSecurityBufferDescriptor = new Interop.SspiCli.SecBufferDesc(inSecBuffers.Length); | ||
Debug.Assert(inSecBuffers.Count <= 3); | ||
Interop.SspiCli.SecBufferDesc inSecurityBufferDescriptor = new Interop.SspiCli.SecBufferDesc(inSecBuffers.Count); | ||
Interop.SspiCli.SecBufferDesc outSecurityBufferDescriptor = new Interop.SspiCli.SecBufferDesc(count: 2); | ||
|
||
// Actually, this is returned in outFlags. | ||
|
@@ -663,35 +668,38 @@ internal static unsafe int AcceptSecurityContext( | |
outUnmanagedBuffer[1].pvBuffer = IntPtr.Zero; | ||
try | ||
{ | ||
Span<Interop.SspiCli.SecBuffer> inUnmanagedBuffer = stackalloc Interop.SspiCli.SecBuffer[inSecurityBufferDescriptor.cBuffers]; | ||
inUnmanagedBuffer.Clear(); | ||
// Allocate always maximum to allow better code optimization. | ||
Span<Interop.SspiCli.SecBuffer> inUnmanagedBuffer = stackalloc Interop.SspiCli.SecBuffer[3]; | ||
for (int index = 0; index < inSecBuffers.Count; ++index) | ||
{ | ||
inUnmanagedBuffer[index].BufferType = inSecBuffers.GetBuffer(index).Type; | ||
inUnmanagedBuffer[index].cbBuffer = inSecBuffers.GetBuffer(index).Token.Length; | ||
inUnmanagedBuffer[index].pvBuffer = inSecBuffers.GetBuffer(index).UnmanagedToken != null ? | ||
(IntPtr)inSecBuffers.GetBuffer(index).UnmanagedToken.DangerousGetHandle() : | ||
IntPtr.Zero; | ||
} | ||
|
||
fixed (void* inUnmanagedBufferPtr = inUnmanagedBuffer) | ||
fixed (void* outUnmanagedBufferPtr = outUnmanagedBuffer) | ||
fixed (void* pinnedToken0 = inSecBuffers.Length > 0 ? inSecBuffers[0].token : null) | ||
fixed (void* pinnedToken1 = inSecBuffers.Length > 1 ? inSecBuffers[1].token : null) | ||
fixed (void* pinnedToken2 = inSecBuffers.Length > 2 ? inSecBuffers[2].token : null) // pin all buffers, even if null or not used, to avoid needing to allocate GCHandles | ||
fixed (void* pinnedToken0 = inSecBuffers.GetBuffer(0).Token) | ||
fixed (void* pinnedToken1 = inSecBuffers.GetBuffer(1).Token) | ||
fixed (void* pinnedToken2 = inSecBuffers.GetBuffer(2).Token) | ||
{ | ||
Debug.Assert(inSecBuffers.Length <= 3); | ||
|
||
// Fix Descriptor pointer that points to unmanaged SecurityBuffers. | ||
inSecurityBufferDescriptor.pBuffers = inUnmanagedBufferPtr; | ||
for (int index = 0; index < inSecurityBufferDescriptor.cBuffers; ++index) | ||
// Updated pvBuffer with pinned address. UnmanagedToken takes precedence. | ||
if (inSecBuffers.Count > 2 && inUnmanagedBuffer[2].pvBuffer == IntPtr.Zero) | ||
{ | ||
ref readonly SecurityBuffer securityBuffer = ref inSecBuffers[index]; | ||
inUnmanagedBuffer[2].pvBuffer = (IntPtr)pinnedToken2; | ||
} | ||
|
||
// Copy the SecurityBuffer content into unmanaged place holder. | ||
inUnmanagedBuffer[index].cbBuffer = securityBuffer.size; | ||
inUnmanagedBuffer[index].BufferType = securityBuffer.type; | ||
|
||
// Use the unmanaged token if it's not null; otherwise use the managed buffer. | ||
inUnmanagedBuffer[index].pvBuffer = | ||
securityBuffer.unmanagedToken != null ? securityBuffer.unmanagedToken.DangerousGetHandle() : | ||
securityBuffer.token == null || securityBuffer.token.Length == 0 ? IntPtr.Zero : | ||
Marshal.UnsafeAddrOfPinnedArrayElement(securityBuffer.token, securityBuffer.offset); | ||
#if TRACE_VERBOSE | ||
if (NetEventSource.IsEnabled) NetEventSource.Info(null, $"SecBuffer: cbBuffer:{securityBuffer.size} BufferType:{securityBuffer.type}"); | ||
#endif | ||
if (inSecBuffers.Count > 1 && inUnmanagedBuffer[1].pvBuffer == IntPtr.Zero) | ||
{ | ||
inUnmanagedBuffer[1].pvBuffer = (IntPtr)pinnedToken1; | ||
} | ||
|
||
if (inSecBuffers.Count > 0 && inUnmanagedBuffer[0].pvBuffer == IntPtr.Zero) | ||
{ | ||
inUnmanagedBuffer[0].pvBuffer = (IntPtr)pinnedToken0; | ||
} | ||
|
||
fixed (byte* pinnedOutBytes = outSecBuffer.token) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: comment isn't necessary