From e6a7763e4e32e51efcfe219eaeed8e4ae1cc3972 Mon Sep 17 00:00:00 2001 From: Adeel Mujahid <3840695+am11@users.noreply.github.com> Date: Tue, 3 Nov 2020 22:28:21 +0200 Subject: [PATCH] Check catch and throw non-Exception derived types (#43969) Verifies: * `X` is derived from `Exception` in `throw X`. * `throw null` is valid. * `X` is derived from `Exception` in `catch(X)`. Contributes to #37390 --- .../src/tools/ILVerification/ILImporter.Verify.cs | 64 +++++++++++++--------- src/coreclr/src/tools/ILVerification/Strings.resx | 3 + src/coreclr/src/tools/ILVerification/Verifier.cs | 6 +- .../src/tools/ILVerification/VerifierError.cs | 3 +- src/coreclr/src/tools/ILVerify/Program.cs | 17 +++--- src/tests/ilverify/ILMethodTester.cs | 9 ++- src/tests/ilverify/ILTests/ExceptionRegionTests.il | 33 ++++++++++- src/tests/ilverify/ILTests/ThisStateTests.il | 2 +- src/tests/ilverify/ILTypeVerificationTester.cs | 6 +- 9 files changed, 101 insertions(+), 42 deletions(-) diff --git a/src/coreclr/src/tools/ILVerification/ILImporter.Verify.cs b/src/coreclr/src/tools/ILVerification/ILImporter.Verify.cs index 787afb5..6540aaf 100644 --- a/src/coreclr/src/tools/ILVerification/ILImporter.Verify.cs +++ b/src/coreclr/src/tools/ILVerification/ILImporter.Verify.cs @@ -201,11 +201,9 @@ namespace Internal.IL } } - public Action ReportVerificationError - { - set; - private get; - } + public Action ReportVerificationError { set; private get; } + + public bool SanityChecks { set; private get; } public void Verify() { @@ -509,9 +507,9 @@ again: if (_currentBasicBlock != null) _currentBasicBlock.IncrementErrorCount(); - var args = new ErrorArgument[] - { - new ErrorArgument("Offset", _currentInstructionOffset) + var args = new ErrorArgument[] + { + new ErrorArgument("Offset", _currentInstructionOffset) }; ReportVerificationError(args, error); } @@ -566,7 +564,7 @@ again: void CheckIsNumeric(StackValue value) { - if (!Check(StackValueKind.Int32 <= value.Kind && value.Kind <= StackValueKind.Float, + if (!Check(StackValueKind.Int32 <= value.Kind && value.Kind <= StackValueKind.Float, VerifierError.ExpectedNumericType, value)) { AbortBasicBlockVerification(); @@ -601,14 +599,14 @@ again: void CheckIsArray(StackValue value) { - Check((value.Kind == StackValueKind.ObjRef) && ((value.Type == null) || value.Type.IsSzArray), + Check((value.Kind == StackValueKind.ObjRef) && ((value.Type == null) || value.Type.IsSzArray), VerifierError.ExpectedArray /* , value */); } - void CheckIsAssignable(StackValue src, StackValue dst) + void CheckIsAssignable(StackValue src, StackValue dst, VerifierError error = VerifierError.StackUnexpected) { if (!IsAssignable(src, dst)) - VerificationError(VerifierError.StackUnexpected, src, dst); + VerificationError(error, src, dst); } private void CheckIsValidLeaveTarget(BasicBlock src, BasicBlock target) @@ -647,7 +645,7 @@ again: ref var targetRegion = ref _exceptionRegions[target.TryIndex.Value].ILRegion; // Target is not enclosing source - if (targetRegion.TryOffset > srcRegion.TryOffset || + if (targetRegion.TryOffset > srcRegion.TryOffset || src.StartOffset >= targetRegion.TryOffset + targetRegion.TryLength) { // Target is not first instruction @@ -687,8 +685,8 @@ again: ref var targetRegion = ref _exceptionRegions[target.TryIndex.Value].ILRegion; // If target is not associated try block, and not enclosing srcRegion - if (target.TryIndex != src.HandlerIndex && - (targetRegion.TryOffset > srcRegion.HandlerOffset || + if (target.TryIndex != src.HandlerIndex && + (targetRegion.TryOffset > srcRegion.HandlerOffset || targetRegion.TryOffset + targetRegion.TryLength < srcRegion.HandlerOffset)) { // If target is not first instruction of try, or not a direct sibling @@ -771,7 +769,7 @@ again: ref var srcRegion = ref _exceptionRegions[src.TryIndex.Value].ILRegion; ref var targetRegion = ref _exceptionRegions[target.TryIndex.Value].ILRegion; // If target is inside source region - if (srcRegion.TryOffset <= targetRegion.TryOffset && + if (srcRegion.TryOffset <= targetRegion.TryOffset && target.StartOffset < srcRegion.TryOffset + srcRegion.TryLength) { // Only branching to first instruction of try-block is valid @@ -919,7 +917,7 @@ again: } /// - /// Checks whether the given enclosed try block is a direct child try-region of + /// Checks whether the given enclosed try block is a direct child try-region of /// the given enclosing try block. /// /// The block enclosing the try block given by . @@ -1008,11 +1006,11 @@ again: case TypeFlags.Char: return "Char"; case TypeFlags.SByte: case TypeFlags.Byte: return "Byte"; - case TypeFlags.Int16: + case TypeFlags.Int16: case TypeFlags.UInt16: return "Short"; case TypeFlags.Int32: case TypeFlags.UInt32: return "Int32"; - case TypeFlags.Int64: + case TypeFlags.Int64: case TypeFlags.UInt64: return "Long"; case TypeFlags.Single: return "Single"; case TypeFlags.Double: return "Double"; @@ -1302,6 +1300,12 @@ again: var exceptionType = ResolveTypeToken(r.ILRegion.ClassToken); Check(!exceptionType.IsByRef, VerifierError.CatchByRef); basicBlock.EntryStack[0] = StackValue.CreateObjRef(exceptionType); + + if (SanityChecks && basicBlock.EntryStack[0] != StackValue.CreateObjRef(GetWellKnownType(WellKnownType.Object))) + { + CheckIsAssignable(basicBlock.EntryStack[0], StackValue.CreateObjRef(GetWellKnownType(WellKnownType.Exception)), + VerifierError.ThrowOrCatchOnlyExceptionType); + } } } else @@ -1606,21 +1610,21 @@ again: { // Rules for non-virtual call to a non-final virtual method (ECMA III.3.19: Verifiability of 'call'): - // Define: + // Define: // The "this" pointer is considered to be "possibly written" if // 1. Its address have been taken (LDARGA 0) anywhere in the method. // (or) // 2. It has been stored to (STARG.0) anywhere in the method. // A non-virtual call to a non-final virtual method is only allowed if - // 1. The this pointer passed to the callee is an instance of a boxed value type. + // 1. The this pointer passed to the callee is an instance of a boxed value type. // (or) // 2. The this pointer passed to the callee is the current method's this pointer. // (and) The current method's this pointer is not "possibly written". - // Thus the rule is that if you assign to this ANYWHERE you can't make "base" calls to - // virtual methods. (Luckily this does not affect .ctors, since they are not virtual). - // This is stronger than is strictly needed, but implementing a laxer rule is significantly + // Thus the rule is that if you assign to this ANYWHERE you can't make "base" calls to + // virtual methods. (Luckily this does not affect .ctors, since they are not virtual). + // This is stronger than is strictly needed, but implementing a laxer rule is significantly // harder and more error prone. if (method.IsVirtual && !method.IsFinal && !actualThis.IsBoxedValueType) { @@ -1636,7 +1640,7 @@ again: Check(!IsByRefLike(declaredThis), VerifierError.TailByRef, declaredThis); // Tail calls on constrained calls should be illegal too: - // when instantiated at a value type, a constrained call may pass the address of a stack allocated value + // when instantiated at a value type, a constrained call may pass the address of a stack allocated value Check(constrained == null, VerifierError.TailByRef); } } @@ -1832,7 +1836,7 @@ again: { if (next.IsThisInitialized && !_isThisInitialized) { - // Next block has 'this' initialized, but current state has not + // Next block has 'this' initialized, but current state has not // therefore next block must be reverified with 'this' uninitialized if (next.State == BasicBlock.ImportState.WasVerified && next.ErrorCount == 0) next.State = BasicBlock.ImportState.Unmarked; @@ -2218,6 +2222,12 @@ again: CheckIsObjRef(value); + if (SanityChecks && value != StackValue.CreateObjRef(GetWellKnownType(WellKnownType.Object))) + { + CheckIsAssignable(value, StackValue.CreateFromType(GetWellKnownType(WellKnownType.Exception)), + VerifierError.ThrowOrCatchOnlyExceptionType); + } + EmptyTheStack(); } @@ -2250,7 +2260,7 @@ again: Check(!IsByRefLike(targetType), VerifierError.BoxByRef, targetType); - Check(type.IsPrimitive || targetType.Kind == StackValueKind.ObjRef || + Check(type.IsPrimitive || targetType.Kind == StackValueKind.ObjRef || type.IsGenericParameter || type.IsValueType, VerifierError.ExpectedValClassObjRefVariable); Check(type.CheckConstraints(), VerifierError.UnsatisfiedBoxOperand); diff --git a/src/coreclr/src/tools/ILVerification/Strings.resx b/src/coreclr/src/tools/ILVerification/Strings.resx index 82f096b..726cf6d 100644 --- a/src/coreclr/src/tools/ILVerification/Strings.resx +++ b/src/coreclr/src/tools/ILVerification/Strings.resx @@ -171,6 +171,9 @@ ByRef not allowed as catch type. + + The type caught or thrown must be derived from System.Exception. + Code size is zero. diff --git a/src/coreclr/src/tools/ILVerification/Verifier.cs b/src/coreclr/src/tools/ILVerification/Verifier.cs index db15494..032c386 100644 --- a/src/coreclr/src/tools/ILVerification/Verifier.cs +++ b/src/coreclr/src/tools/ILVerification/Verifier.cs @@ -179,7 +179,10 @@ namespace ILVerify try { - var importer = new ILImporter(method, methodIL); + var importer = new ILImporter(method, methodIL) + { + SanityChecks = _verifierOptions.SanityChecks + }; importer.ReportVerificationError = (args, code) => { @@ -321,5 +324,6 @@ namespace ILVerify public class VerifierOptions { public bool IncludeMetadataTokensInErrorMessages { get; set; } + public bool SanityChecks { get; set; } } } diff --git a/src/coreclr/src/tools/ILVerification/VerifierError.cs b/src/coreclr/src/tools/ILVerification/VerifierError.cs index 331d0a8..4be09b1 100644 --- a/src/coreclr/src/tools/ILVerification/VerifierError.cs +++ b/src/coreclr/src/tools/ILVerification/VerifierError.cs @@ -145,6 +145,7 @@ namespace ILVerify DelegateCtorSigO, // Unrecognized delegate .ctor signature; expected Object. //E_RA_PTR_TO_STACK "Mkrefany on TypedReference, ArgHandle, or ArgIterator." CatchByRef, // ByRef not allowed as catch type. + ThrowOrCatchOnlyExceptionType, // The type caught or thrown must be derived from System.Exception. LdvirtftnOnStatic, // ldvirtftn on static. CallVirtOnStatic, // callvirt on static. InitLocals, // initlocals must be set for verifiable methods with one or more local variables. @@ -189,6 +190,6 @@ namespace ILVerify //IDS_E_GLOBAL "" //IDS_E_MDTOKEN "[mdToken=0x%x]" InterfaceImplHasDuplicate, // InterfaceImpl has a duplicate - InterfaceMethodNotImplemented // Class implements interface but not method + InterfaceMethodNotImplemented // Class implements interface but not method } } diff --git a/src/coreclr/src/tools/ILVerify/Program.cs b/src/coreclr/src/tools/ILVerify/Program.cs index b9fbf35..a5b3807 100644 --- a/src/coreclr/src/tools/ILVerify/Program.cs +++ b/src/coreclr/src/tools/ILVerify/Program.cs @@ -46,6 +46,7 @@ namespace ILVerify public string[] InputFilePath { get; set; } public string[] Reference { get; set; } public string SystemModule { get; set; } + public bool SanityChecks { get; set; } public string[] Include { get; set; } public FileInfo IncludeFile { get; set; } public string[] Exclude { get; set; } @@ -63,6 +64,7 @@ namespace ILVerify command.AddArgument(new Argument("input-file-path", "Input file(s)") { Arity = new ArgumentArity(1, Int32.MaxValue) }); command.AddOption(new Option(new[] { "--reference", "-r" }, "Reference metadata from the specified assembly")); command.AddOption(new Option(new[] { "--system-module", "-s" }, "System module name (default: mscorlib)")); + command.AddOption(new Option(new[] { "--sanity-checks", "-c" }, "Check for valid constructs that are likely mistakes")); command.AddOption(new Option(new[] { "--include", "-i" }, "Use only methods/types/namespaces, which match the given regular expression(s)")); command.AddOption(new Option(new[] { "--include-file" }, "Same as --include, but the regular expression(s) are declared line by line in the specified file.").ExistingOnly()); command.AddOption(new Option(new[] { "--exclude", "-e" }, "Skip methods/types/namespaces, which match the given regular expression(s)")); @@ -144,7 +146,11 @@ namespace ILVerify private int Run() { - _verifier = new Verifier(this, GetVerifierOptions()); + _verifier = new Verifier(this, new VerifierOptions + { + IncludeMetadataTokensInErrorMessages = _options.Tokens, + SanityChecks = _options.SanityChecks + }); _verifier.SetSystemModuleName(new AssemblyName(_options.SystemModule ?? "mscorlib")); int numErrors = 0; @@ -164,11 +170,6 @@ namespace ILVerify } } - private VerifierOptions GetVerifierOptions() - { - return new VerifierOptions { IncludeMetadataTokensInErrorMessages = _options.Tokens }; - } - private void PrintVerifyMethodsResult(VerificationResult result, EcmaModule module, string pathOrModuleName) { Write("[IL]: Error ["); @@ -257,7 +258,7 @@ namespace ILVerify } } } - catch + catch { Write("Error while getting method signature"); } @@ -409,7 +410,7 @@ namespace ILVerify /// /// This method returns the fully qualified method name by concatenating assembly, type and method name. - /// This method exists to avoid additional assembly resolving, which might be triggered by calling + /// This method exists to avoid additional assembly resolving, which might be triggered by calling /// MethodDesc.ToString(). /// private string GetQualifiedMethodName(MetadataReader metadataReader, MethodDefinitionHandle methodHandle) diff --git a/src/tests/ilverify/ILMethodTester.cs b/src/tests/ilverify/ILMethodTester.cs index b7891d6..cb5a316 100644 --- a/src/tests/ilverify/ILMethodTester.cs +++ b/src/tests/ilverify/ILMethodTester.cs @@ -28,7 +28,7 @@ namespace ILVerification.Tests void TestMethodsWithInvalidIL(InvalidILTestCase invalidIL) { IEnumerable results = null; - + try { results = Verify(invalidIL); @@ -58,7 +58,12 @@ namespace ILVerification.Tests EcmaModule module = TestDataLoader.GetModuleForTestAssembly(testCase.ModuleName); var methodHandle = (MethodDefinitionHandle) MetadataTokens.EntityHandle(testCase.MetadataToken); var method = (EcmaMethod)module.GetMethod(methodHandle); - var verifier = new Verifier((ILVerifyTypeSystemContext)method.Context, new VerifierOptions() { IncludeMetadataTokensInErrorMessages = true }); + var verifier = new Verifier((ILVerifyTypeSystemContext)method.Context, new VerifierOptions + { + IncludeMetadataTokensInErrorMessages = true, + SanityChecks = true + }); + return verifier.Verify(module.PEReader, methodHandle); } } diff --git a/src/tests/ilverify/ILTests/ExceptionRegionTests.il b/src/tests/ilverify/ILTests/ExceptionRegionTests.il index 7e066ee..874c420 100644 --- a/src/tests/ilverify/ILTests/ExceptionRegionTests.il +++ b/src/tests/ilverify/ILTests/ExceptionRegionTests.il @@ -12,6 +12,15 @@ .class public sequential ansi sealed beforefieldinit ExceptionRegionTests extends [System.Runtime]System.Object { + .method public hidebysig specialname rtspecialname instance void .ctor () cil managed + { + .maxstack 1 + + IL_0000: ldarg.0 + IL_0001: call instance void [System.Runtime]System.Object::.ctor() + IL_0006: ret + } + .method public instance void ExceptionRegion.NestedTryFinally_Valid() cil managed { .try @@ -732,5 +741,27 @@ MethodEnd: ret } -} + .method public hidebysig instance void Catch.NonExceptionDerivedType_Invalid_ThrowOrCatchOnlyExceptionType() cil managed + { + .maxstack 1 + + .try {} + catch ExceptionRegionTests + { + leave.s MethodEnd + } + + MethodEnd: + ret + } + + .method public hidebysig instance void Throw.NonExceptionDerivedType_Invalid_ThrowOrCatchOnlyExceptionType() cil managed + { + .maxstack 1 + + newobj instance void ExceptionRegionTests::.ctor() + throw + ret + } +} diff --git a/src/tests/ilverify/ILTests/ThisStateTests.il b/src/tests/ilverify/ILTests/ThisStateTests.il index 7b73893..47b96b5 100644 --- a/src/tests/ilverify/ILTests/ThisStateTests.il +++ b/src/tests/ilverify/ILTests/ThisStateTests.il @@ -83,7 +83,7 @@ .class public auto ansi beforefieldinit ThisStateTestsType extends [System.Runtime]System.Object { - .method public hidebysig instance void 'special.ThrowThisUninit..ctor_Invalid_UninitStack'() cil managed { ret } + .method public hidebysig instance void 'special.ThrowThisUninit..ctor_Invalid_UninitStack.ThrowOrCatchOnlyExceptionType'() cil managed { ret } .method public hidebysig specialname rtspecialname instance void .ctor() cil managed { ldarg.0 diff --git a/src/tests/ilverify/ILTypeVerificationTester.cs b/src/tests/ilverify/ILTypeVerificationTester.cs index 3f9959e..902e612 100644 --- a/src/tests/ilverify/ILTypeVerificationTester.cs +++ b/src/tests/ilverify/ILTypeVerificationTester.cs @@ -58,7 +58,11 @@ namespace ILVerification.Tests EcmaModule module = TestDataLoader.GetModuleForTestAssembly(testCase.ModuleName); var typeHandle = (TypeDefinitionHandle)MetadataTokens.EntityHandle(testCase.MetadataToken); var type = (EcmaType)module.GetType(typeHandle); - var verifier = new Verifier((ILVerifyTypeSystemContext)type.Context, new VerifierOptions() { IncludeMetadataTokensInErrorMessages = true }); + var verifier = new Verifier((ILVerifyTypeSystemContext)type.Context, new VerifierOptions + { + IncludeMetadataTokensInErrorMessages = true, + SanityChecks = true + }); return verifier.Verify(module.PEReader, typeHandle); } -- 2.7.4