-
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
Make System.Formats.Asn1 library public #36729
Conversation
AsnReader partials are named AsnDecoder to better match the final name for the core logic they contain.
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
src/libraries/Common/src/System/Security/Cryptography/Asn1Reader/AsnValueReader.cs
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/ref/System.Formats.Asn1.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj
Outdated
Show resolved
Hide resolved
<Project Sdk="Microsoft.NET.Sdk"> | ||
<PropertyGroup> | ||
<IsPartialFacadeAssembly Condition="'$(TargetsNetFx)' == 'true'">true</IsPartialFacadeAssembly> | ||
<TargetFrameworks>netstandard2.0</TargetFrameworks> |
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.
if you want the facades to come into play when this is used in netframework, we should cross target this for netfx.
cc: @joperezr
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.
That is correct. That said, I still have to merge all of the netstandard facades work into dotnet/runtime, so I don't mind taking care of this one as well once I do that so as to not introduce m ore changes into this PR.
<IncludeDllSafeSearchPathAttribute>true</IncludeDllSafeSearchPathAttribute> | ||
<NoWarn>$(NoWarn);CS1574;CS3016;CA5379;CA5384</NoWarn> | ||
<Nullable>enable</Nullable> | ||
<TargetFrameworks>netstandard2.0</TargetFrameworks> |
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.
This one as well I guess? @joperezr
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.
Same thing, you technically do want them but I'm fine with adding them later if you don't want to introduce more changes into your PR.
src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/ref/System.Formats.Asn1.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/tests/System.Formats.Asn1.Tests.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnCharacterStringEncodings.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnCharacterStringEncodings.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnContentException.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnContentException.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnContentException.cs
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnDecoder.Enumerated.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnDecoder.GeneralizedTime.cs
Show resolved
Hide resolved
} | ||
} | ||
else | ||
{ | ||
Debug.Fail($"Unhandled value '{octet:X2}' in {nameof(FracState)}"); | ||
throw new CryptographicException(); | ||
throw new AsnContentException(); |
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.
Why both the Debug.Fail and the throw?
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.
Yeah, I don't think this Debug.Fail
should be here. In general, invalid data from the client is "expected" in that we intend for it to go through graceful exception handling code paths. It doesn't violate invariants in our code.
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.
The fail is appropriate in that the only things which should transition into FracState are seeing a . or a , and both were already handled (an invalid character would have been processed at the point it decided if it was going to the Frac or Offset or Z states). This could be folded with the comma case as an else (as an assert that it's a comma) if there's a strong desire to do so.
Large PR :) Did a bunch of spot checking. Generally looks good. |
src/libraries/System.Formats.Asn1/ref/System.Formats.Asn1.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/pkg/System.Formats.Asn1.pkgproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj
Outdated
Show resolved
Hide resolved
All infrastructure code looks good to me. Just that comment regarding removing refs from package. |
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnContentException.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Security/Cryptography/Asn1/Pbkdf2SaltChoice.xml.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnDecoder.Integer.cs
Outdated
Show resolved
Hide resolved
{ | ||
ushort bigEndianValue = BinaryPrimitives.ReadUInt16BigEndian(contents); | ||
const ushort RedundancyMask = 0b1111_1111_1000_0000; | ||
ushort masked = (ushort)(bigEndianValue & RedundancyMask); |
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.
Let me know offline if this is a hot path and we can work to bit-twiddle this in a more optimized fashion.
@@ -22,8 +22,11 @@ public sealed partial class AsnWriter | |||
private Stack<StackFrame>? _nestingStack; | |||
|
|||
/// <summary> | |||
/// The <see cref="AsnEncodingRules"/> in use by this writer. | |||
/// Gets the encoding rules in use by this writer. |
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.
There are some other methods in this file (e.g., Reverse
on line 596) that won't be needed once targeting netstandard2.1+ or net5.0+. Are we keeping track of these anywhere?
} | ||
|
||
bool isNegative = (contents[0] & 0x80) != 0; | ||
long accum = isNegative ? -1 : 0; |
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: long accum = (sbyte)contents[0];
, which will sign-extend the 8-bit contents[0]
value to 64 bits. Then we can drop line 449 entirely, and on line 452 start the for loop at i = 1.
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnDecoder.NamedBitList.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnDecoder.GeneralizedTime.cs
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,56 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> |
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'm not quite sure how we would do this, but presumably we want this entire project to opt out of the [SkipLocalsInit]
behavior that our libraries projects normally get.
Related: #35527
We should also consider having all of the arithmetic in this project checked by default, but the team collectively might find that too big a hammer to wield.
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.
Once #37541 is merged, looks like we'll be able to use <ShouldSkipLocalsInit>false</ShouldSkipLocalsInit>
in the .csproj to opt out of this optimization.
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.
opt out of the [SkipLocalsInit] behavior that our libraries projects normally get
For my education, why is that?
using System.Runtime.InteropServices; | ||
using System.Security.Cryptography; | ||
using System.Security.Cryptography.Asn1; | ||
|
||
namespace <xsl:value-of select="@namespace" /> | ||
{ |
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.
These structs are marked Sequential. Since they're not expected to be passed via p/invoke, would it make sense instead to annotate them as Auto so that the runtime can optimize them as it sees fit?
This same question applies to other structs, such as Asn1Tag
and AsnValueReader
. You'd need to consult with somebody more familiar with the runtime + interop to see what an appropriate annotation would be for the structs defined here.
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/SetOfValueComparer.cs
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnWriter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnWriter.Text.cs
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnDecoder.BitString.cs
Show resolved
Hide resolved
RSAKeyFormatHelper.ReadRsaPublicKey(firstValue, ignored, out RSAParameters rsaParameters); | ||
fixed (byte* ptr = &MemoryMarshal.GetReference(source)) | ||
{ | ||
using (MemoryManager<byte> manager = new PointerMemoryManager<byte>(ptr, localRead)) |
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.
Future improvement: we should consider getting rid of these call sites if we can. It appears that the only reason the method accepts a ROM<byte>
instead of a ROS<byte>
is that the memory instance is eventually plumbed through as rebind, but in cases like this where there's no ROM<byte>
field on the struct the parameter remains unused. The end effect is that the caller is forced to use unsafe code to wrap a ROM<byte>
around a ROS<byte>
, when the span itself was all that was needed in the first place.
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnDecoder.Enumerated.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnDecoder.OctetString.cs
Show resolved
Hide resolved
src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnCharacterStringEncodings.cs
Show resolved
Hide resolved
...m.Security.Cryptography.Pkcs/src/Internal/Cryptography/Pal/Windows/PkcsPalWindows.Encrypt.cs
Show resolved
Hide resolved
Some additional tests were written to increase code coverage after the changes
🎉🥳 |
Looks like I accidentally hit squash and merge anyways. About half of the moved files ended up looking like moves... so... could have been worse. Sigh. |
GitHub's compare UI suggests that as one giant squash the existing and new files are too different to count as a move+edit, so this PR is marked as No Squash so the move gets preserved into the file histories.