Repository navigation
Fix invalid access of Target in SafeCredentialReference - #36875
Conversation
|
Tagging subscribers to this area: @dotnet/ncl |
|
you may need to rebase or merge from upstream @aik-jahoda |
9779b99 to
a660da1
Compare
|
@wfurt, thanks for the notice. All tests are passing now. |
Co-authored-by: Stephen Toub <stoub@microsoft.com>
| creds = cached.Target; | ||
| cached.Dispose(); | ||
|
|
||
| if (creds != null && !creds.IsClosed && !creds.IsInvalid && (cached = SafeCredentialReference.CreateReference(creds)) != null) |
There was a problem hiding this comment.
How would creds be null here? If there's actually a concern that cached.Target might change from non-null to null concurrently, you can move the creds = cached.Target line to above the previous if block, and then use that creds in the above if check... then it definitely won't be null here.
There was a problem hiding this comment.
Also, doesn't CreateReference already check for IsClosed and IsInvalid and return null if either is true? Why do we need to duplicate those checks here?
|
@aik-jahoda, are you still working on this? |
|
@stephentoub not actively now. However this is on our test failure list and I expect it will be prioritised soon. |
|
I continue on this PR to resolve the handles issue. |
970d63f to
46631be
Compare
|
I run the test in loop for several days with any failure. It is ready to merge and we will observe possible failures in our weekly CI report. |
ghost
locked as resolved and limited conversation to collaborators
|
@aik-jahoda is the result.txt file that was checked into this PR necessary? |
|
no, it is not. we miss it in the review IMHO as GitHub would not show binary (utf-16) diff @ViktorHofer |
|
@aik-jahoda can you please send a PR to remove the file from main? |
6 participants
The SafeCredentialReference derive from CriticalHandleMinusOneIsInvalid however it didn't owerride IsInvalid nor IsClosed and the behaviour of those method are not reflecting the real Target state.
I removed this inheritance so the problemathics methods are no longer valid and it pointed into problematic usage of SafeCredentialReference which is fixed now.
Fixes #34337