-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Fix a native integer decoding bug #43885
Conversation
We weren't consuming an index when decoding type parameters or dynamic types, meaning that we could get into a scenario where the remaining flags are applying to the wrong types.
@@ -62,6 +62,7 @@ private TypeSymbol TransformType(TypeSymbol type) | |||
return TransformPointerType((PointerTypeSymbol)type); | |||
case TypeKind.TypeParameter: | |||
case TypeKind.Dynamic: | |||
IgnoreIndex(); |
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 part is the bug fix.
private void IgnoreIndex() | ||
{ | ||
var index = Increment(); | ||
Debug.Assert(!_transformFlags[index]); |
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.
Debug.Assert(!_transformFlags[index]) [](start = 12, length = 37)
This may fail for handwritten attribute data.
Please add a test.
class C<T, U, V> | ||
{{ | ||
public {sourceType} F; | ||
}} |
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.
Please consider writing out all the cases inline as separate fields in a single test, for readability and to match other tests.
}} | ||
", options: TestOptions.ReleaseDll, parseOptions: TestOptions.RegularPreview, symbolValidator: symbolValidator); | ||
|
||
void symbolValidator(ModuleSymbol module) |
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.
void [](start = 12, length = 4)
Consider making static
or inline in CompileAndVerify
to match the other tests.
@@ -125,6 +128,12 @@ private int Increment() | |||
throw new ArgumentException(); |
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.
throw new ArgumentException [](start = 12, length = 27)
If we're worried about handwritten attribute data, this could crash the compiler as well. Please add a test. #Resolved
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.
It can't crash the compiler, it's caught above. I already tried to crash it with this :). #Resolved
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 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.
…it's not understood.
@@ -71,6 +97,7 @@ private TypeSymbol TransformType(TypeSymbol type) | |||
return TransformNamedType((NamedTypeSymbol)type); | |||
default: | |||
Debug.Assert(type.TypeKind == TypeKind.Error); | |||
_decodeStatus = DecodeStatus.FailedToErrorType; | |||
throw new ArgumentException(); |
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.
Consider extracting a helper method with DecodeStatus
argument.
{ | ||
NotFailed, | ||
FailedToErrorType, | ||
FailedToBadMetadata, |
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.
Consider renaming fields, perhaps:
enum DecodeStatus
{
Succeeded,
ErrorType,
BadMetadata,
}
} | ||
|
||
private readonly ImmutableArray<bool> _transformFlags; | ||
private int _index; | ||
private DecodeStatus _decodeStatus; |
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.
DecodeStatus _decodeStatus [](start = 16, length = 26)
Is this field needed?
Rather than using a field, could we just throw ArgumentException
for cases that have error types (where we catch the exception locally and return the original type), and throw UnsupportMetadataTypeSymbol
for case where we fail completely?
if (_transformFlags[index]) | ||
{ | ||
_decodeStatus = DecodeStatus.FailedToBadMetadata; | ||
throw new ArgumentException(); |
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.
throw new ArgumentException(); [](start = 16, length = 30)
Are we testing this case?
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.
LGTM. Thanks for fixing this.
@gafter there have been more changes since your review, probably needs another look. |
src/Compilers/CSharp/Portable/Symbols/Metadata/PE/NativeIntegerTypeDecoder.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Metadata/PE/NativeIntegerTypeDecoder.cs
Show resolved
Hide resolved
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.
We should either produce successfully decoded type, or ignore the malformed attribute and return original type. This is how I believe we always handle attributes like that.
Actually, it looks like only nullable decoder ignores malformed attribute, which probably makes sense because annotations can cause only warnings. #Closed |
Yeah. My concern with bad nint data is that it will affect codegen. |
} | ||
catch (UnsupportedSignatureContent) | ||
{ | ||
return new UnsupportedMetadataTypeSymbol(); | ||
} | ||
catch (ArgumentException) |
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.
ArgumentException [](start = 19, length = 17)
Using ArgumentException
to manage these failure modes (line 88) seems a bit dangerous, as you can't have complete confidence that it was one thrown as part of decoding in this class. It might be better to make a custom exception (private?) for this purpose/
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 suggest you stop using ArgumentException
in the decoder, because when you catch it you cannot be sure it was thrown in your code on line 88. Instead perhaps use a new private exception for that purpose.
Otherwise looks good.
…rgumentException.
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.
Auto-approval
We weren't consuming an index when decoding type parameters or dynamic types, meaning that we could get into a scenario where the remaining flags are applying to the wrong types. I also tightened our assertions around ignored indexes.
@cston for review.