-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Bring to CoreCLR the SecureString implementation from CoreFx. #7884
Conversation
f544a8c
to
ff7437d
Compare
/cc @JeremyKuhne @jkotas |
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.
Don't use BCLDebug, use Debug.Assert to keep the code aligned.
|
||
private unsafe void InitializeSecureString(char* value, int length) | ||
{ | ||
BCLDebug.Assert(length >= 0, $"Expected non-negative length, got {length}"); |
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.
Just use Debug.Assert
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.
Ah - yes! When I originally ported this, your change was not there :).
Will change it!
// Make sure the requested capacity doesn't exceed SecureString's defined limit | ||
if (capacity > MaxLength) | ||
{ | ||
throw new ArgumentOutOfRangeException("capacity", SR.ArgumentOutOfRange_Capacity); |
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: nameof(capacity)
? (I see there are some other uses of nameof
in this change, so presumably this language feature can be used in corelib.)
<Member Name="MakeReadOnly" /> | ||
<Member Name="RemoveAt(System.Int32)" /> | ||
<Member Name="SetAt(System.Int32,System.Char)" /> | ||
</Type> |
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.
What about SecureStringMarshal?
namespace System.Security.Cryptography | ||
{ | ||
[Serializable] | ||
public class CryptographicException : SystemException |
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.
So are we now exposing CryptographicException from coreclr as well? Presumably it needs to be added to model.xml as well then?
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.
I added them both to model.xml.
Once this is checked in there is work remaining in CoreFx: