Reduce the nullability bypasses in CryptoPool.Return #55782

bartonjs · 2021-07-15T23:17:29Z

This is a walk over src/libraries/.../*.cs to find any calls to
CryptoPool.Return which needed a "!" because of nullability
flow.

A couple of the bypasses were identifying legitimate issues,
and were resolved by moving the Rent to the last thing before
the try.

A couple were because of the use of ArraySegment which
can't structurally promise the array is non-null. But those
were usually preceded by a call to ZeroMemory, and there
is a CryptoPool.Return(ArraySegment) which does that
already, so those "!"s were removed by changing to that
overload.

This is a walk over src/libraries/.../*.cs to find any calls to CryptoPool.Return which needed a "!" because of nullability flow. A couple of the bypasses were identifying legitimate issues, and were resolved by moving the Rent to the last thing before the try. A couple were because of the use of ArraySegment which can't structurally promise the array is non-null. But those were usually preceded by a call to ZeroMemory, and there is a CryptoPool.Return(ArraySegment) which does that already, so those "!"s were removed by changing to that overload.

ghost · 2021-07-15T23:17:36Z

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

This is a walk over src/libraries/.../*.cs to find any calls to
CryptoPool.Return which needed a "!" because of nullability
flow.

A couple of the bypasses were identifying legitimate issues,
and were resolved by moving the Rent to the last thing before
the try.

A couple were because of the use of ArraySegment which
can't structurally promise the array is non-null. But those
were usually preceded by a call to ZeroMemory, and there
is a CryptoPool.Return(ArraySegment) which does that
already, so those "!"s were removed by changing to that
overload.

Author:	bartonjs
Assignees:	bartonjs
Labels:	`area-System.Security`
Milestone:	6.0.0

bartonjs · 2021-07-15T23:18:42Z

This was the followup promised from #54282 (comment).

src/libraries/Common/src/System/Security/Cryptography/CngPkcs8.cs

src/libraries/Common/src/System/Security/Cryptography/KeyFormatHelper.Encrypted.cs

src/libraries/Common/src/System/Security/Cryptography/RSAAndroid.cs

src/libraries/Common/src/System/Security/Cryptography/RSAOpenSsl.cs

...stem.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Pkcs8PrivateKeyInfo.cs

Co-authored-by: Stephen Toub <stoub@microsoft.com>

bartonjs added this to the 6.0.0 milestone Jul 15, 2021

bartonjs requested a review from GrabYourPitchforks July 15, 2021 23:17

bartonjs self-assigned this Jul 15, 2021

dotnet-issue-labeler bot added the area-System.Security label Jul 15, 2021