Fix a native integer decoding bug by 333fred · Pull Request #43885 · dotnet/roslyn

333fred · 2020-05-01T21:26:36Z

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.

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.

333fred · 2020-05-01T21:30:15Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/NativeIntegerTypeDecoder.cs

                    return TransformPointerType((PointerTypeSymbol)type);
                case TypeKind.TypeParameter:
                case TypeKind.Dynamic:
+                    IgnoreIndex();


This part is the bug fix.

cston · 2020-05-01T21:46:15Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/NativeIntegerTypeDecoder.cs

+        private void IgnoreIndex()
+        {
+            var index = Increment();
+            Debug.Assert(!_transformFlags[index]);


Debug.Assert(!_transformFlags[index]) [](start = 12, length = 37)

This may fail for handwritten attribute data.

Please add a test.

cston · 2020-05-01T21:48:34Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_NativeInteger.cs

+class C<T, U, V>
+{{
+    public {sourceType} F;
+}}


Please consider writing out all the cases inline as separate fields in a single test, for readability and to match other tests.

cston · 2020-05-01T21:49:26Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_NativeInteger.cs

+}}
+", options: TestOptions.ReleaseDll, parseOptions: TestOptions.RegularPreview, symbolValidator: symbolValidator);
+
+            void symbolValidator(ModuleSymbol module)


void [](start = 12, length = 4)

Consider making static or inline in CompileAndVerify to match the other tests.

gafter · 2020-05-01T22:05:41Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/NativeIntegerTypeDecoder.cs

@@ -125,6 +128,12 @@ private int Increment()
            throw new ArgumentException();


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

It can't crash the compiler, it's caught above. I already tried to crash it with this :). #Resolved

There should be existing tests for this case.

In reply to: 418757779 [](ancestors = 418757779)

gafter

…it's not understood.

cston · 2020-05-01T23:26:07Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/NativeIntegerTypeDecoder.cs

                default:
                    Debug.Assert(type.TypeKind == TypeKind.Error);
+                    _decodeStatus = DecodeStatus.FailedToErrorType;
                    throw new ArgumentException();


Consider extracting a helper method with DecodeStatus argument.

cston · 2020-05-01T23:28:28Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/NativeIntegerTypeDecoder.cs

+        {
+            NotFailed,
+            FailedToErrorType,
+            FailedToBadMetadata,


Consider renaming fields, perhaps:

enum DecodeStatus { Succeeded, ErrorType, BadMetadata, }

cston · 2020-05-02T00:12:28Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/NativeIntegerTypeDecoder.cs


        private readonly ImmutableArray<bool> _transformFlags;
        private int _index;
+        private DecodeStatus _decodeStatus;


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?

cston · 2020-05-02T00:14:15Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/NativeIntegerTypeDecoder.cs

+            if (_transformFlags[index])
+            {
+                _decodeStatus = DecodeStatus.FailedToBadMetadata;
+                throw new ArgumentException();


throw new ArgumentException(); [](start = 16, length = 30)

Are we testing this case?

cston

LGTM. Thanks for fixing this.

333fred · 2020-05-02T07:36:40Z

@gafter there have been more changes since your review, probably needs another look.

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/NativeIntegerTypeDecoder.cs

AlekseyTs

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.

Obsolete

AlekseyTs · 2020-05-02T14:55:41Z

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

333fred · 2020-05-02T15:52:52Z

Actually, it looks like only nullable decoder ignores malformed attribute, which probably makes sense because annotations can cause only warnings.

Yeah. My concern with bad nint data is that it will affect codegen.

revoking review

gafter · 2020-05-05T23:31:42Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/NativeIntegerTypeDecoder.cs

+            {
+                return new UnsupportedMetadataTypeSymbol();
            }
            catch (ArgumentException)


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/

gafter

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.

ghost

Auto-approval

Fix a native integer decoding bug

45e0477

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.

333fred added the Area-Compilers label May 1, 2020

333fred requested review from a team and cston May 1, 2020 21:26

333fred commented May 1, 2020

View reviewed changes

cston reviewed May 1, 2020

View reviewed changes

gafter reviewed May 1, 2020

View reviewed changes

gafter previously approved these changes May 1, 2020

View reviewed changes

Be more explicit about bad type handling, fail to load metadata when …

7d8f794

…it's not understood.

cston reviewed May 1, 2020

View reviewed changes

cston reviewed May 2, 2020

View reviewed changes

Simplify error handling, add addtional test case

34de2a0

cston approved these changes May 2, 2020

View reviewed changes

333fred requested a review from gafter May 2, 2020 07:36

AlekseyTs reviewed May 2, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/NativeIntegerTypeDecoder.cs Show resolved Hide resolved

AlekseyTs reviewed May 2, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/NativeIntegerTypeDecoder.cs Show resolved Hide resolved

AlekseyTs previously requested changes May 2, 2020

View reviewed changes

gafter reviewed May 5, 2020

View reviewed changes

gafter approved these changes May 5, 2020

View reviewed changes

Use a private exception type for error types encountered instead of A…

53df7ca

…rgumentException.

333fred added the auto-merge label May 6, 2020

ghost approved these changes May 6, 2020

View reviewed changes

ghost merged commit cb1f6ee into dotnet:master May 6, 2020

333fred deleted the nint-decode-bug branch May 6, 2020 19:48

This pull request was closed.

		@@ -125,6 +128,12 @@ private int Increment()
		throw new ArgumentException();

Conversation

333fred commented May 1, 2020

Uh oh!

333fred May 1, 2020

Choose a reason for hiding this comment

Uh oh!

cston May 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 1, 2020

Choose a reason for hiding this comment

Uh oh!

gafter May 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred May 1, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 1, 2020

Choose a reason for hiding this comment

Uh oh!

gafter left a comment

Choose a reason for hiding this comment

Uh oh!

cston May 1, 2020

Choose a reason for hiding this comment

Uh oh!

cston May 1, 2020

Choose a reason for hiding this comment

Uh oh!

cston May 2, 2020

Choose a reason for hiding this comment

Uh oh!

cston May 2, 2020

Choose a reason for hiding this comment

Uh oh!

cston left a comment

Choose a reason for hiding this comment

Uh oh!

333fred commented May 2, 2020

Uh oh!

Uh oh!

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented May 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred commented May 2, 2020

Uh oh!

gafter May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cston May 1, 2020 •

edited

Loading

cston May 1, 2020 •

edited

Loading

gafter May 1, 2020 •

edited

Loading

333fred May 1, 2020 •

edited by gafter

Loading

AlekseyTs commented May 2, 2020 •

edited

Loading

gafter May 5, 2020 •

edited

Loading