Fix crash in GSSAPI on macOS #71484
Fix crash in GSSAPI on macOS #71484
Conversation
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/ncl, @vcsjones Issue DetailsOn macOS the gss_accept_sec_context/gss_init_sec_context APIs release the context handle when error occurs. The code didn't handle it properly and it would result in double-free and hard crash. Update the code to handle this situation properly. Fixes #71463
|
… the context handle when error occurs. The code didn't handle it properly and it would result in double-free and hard crash. Update the code to handle this situation properly.
filipnavara
force-pushed
the
gss-context-null-macos
branch
from
721a4a3 to
00b2786
Compare
| @@ -85,146 +85,6 @@ internal static string QueryContextAuthenticationPackage(SafeDeleteContext secur | |||
| } | |||
| } | |||
|
|
|||
| private static bool GssInitSecurityContext( | |||
There was a problem hiding this comment.
The GssInitSecurityContext and GssAcceptSecurityContext methods are folded into the caller. This made updating the handles substantially easier. I also removed the code that threw GssApiException only to catch it one method above in the stack and convert it to reported status code.
| @@ -45,6 +45,7 @@ public SafeDeleteNegoContext(SafeFreeNegoCredentials credential) | |||
| : base(credential) | |||
| { | |||
| Debug.Assert((null != credential), "Null credential in SafeDeleteNegoContext"); | |||
| _context = new SafeGssContextHandle(); | |||
There was a problem hiding this comment.
Instead of trying to handle null values here it's easier to just always have a non-null handle.
There was a problem hiding this comment.
I assume we need to always allocate it anyway? #69527 is trying to avoid unnecessary allocations.
There was a problem hiding this comment.
Yes, it would always get allocated right after SafeDeleteNegoContext is created.
| @@ -63,7 +65,6 @@ public SafeDeleteNegoContext(SafeFreeNegoCredentials credential, string targetNa | |||
|
|
|||
| public void SetGssContext(SafeGssContextHandle context) | |||
| { | |||
| Debug.Assert(context != null && !context.IsInvalid, "Invalid context passed to SafeDeleteNegoContext"); | |||
There was a problem hiding this comment.
We now explicitly allow invalid handles to reset the previous values so the Assert became useless. context == null is already checked by the nullability in the compiler.
|
I assume this is independent of #71373, right? |
Yep. Although it may create some merge conflict. |
src/libraries/Common/src/System/Net/Security/NegotiateStreamPal.Unix.cs
Outdated
Show resolved
Hide resolved
On macOS the gss_accept_sec_context/gss_init_sec_context APIs release the context handle when error occurs. The code didn't handle it properly and it would result in double-free and hard crash. Update the code to handle this situation properly.
Fixes #71463