From 497c3392829abc1f2fd0f665cd691b0da339ed1e Mon Sep 17 00:00:00 2001 From: Anton Tykhyy Date: Fri, 5 May 2017 14:20:19 +0300 Subject: [PATCH 1/2] Add truncation checks for short forms of some IL instructions --- Mono.Cecil.Cil/CodeWriter.cs | 14 +++--- Test/Mono.Cecil.Tests/BaseTestFixture.cs | 19 ++++++++ Test/Mono.Cecil.Tests/MethodBodyTests.cs | 59 +++++++++++++++++++++++ Test/Resources/assemblies/libhello.dll | Bin 3072 -> 3072 bytes 4 files changed, 85 insertions(+), 7 deletions(-) diff --git a/Mono.Cecil.Cil/CodeWriter.cs b/Mono.Cecil.Cil/CodeWriter.cs index 2003f104f..c154d6046 100644 --- a/Mono.Cecil.Cil/CodeWriter.cs +++ b/Mono.Cecil.Cil/CodeWriter.cs @@ -217,7 +217,7 @@ void WriteOperand (Instruction instruction) case OperandType.ShortInlineBrTarget: { var target = (Instruction) operand; var offset = target != null ? GetTargetOffset (target) : body.code_size; - WriteSByte ((sbyte) (offset - (instruction.Offset + opcode.Size + 1))); + WriteSByte (checked ((sbyte) (offset - (instruction.Offset + opcode.Size + 1)))); break; } case OperandType.InlineBrTarget: { @@ -227,25 +227,25 @@ void WriteOperand (Instruction instruction) break; } case OperandType.ShortInlineVar: - WriteByte ((byte) GetVariableIndex ((VariableDefinition) operand)); + WriteByte (checked ((byte) GetVariableIndex ((VariableDefinition) operand))); break; case OperandType.ShortInlineArg: - WriteByte ((byte) GetParameterIndex ((ParameterDefinition) operand)); + WriteByte (checked ((byte) GetParameterIndex ((ParameterDefinition) operand))); break; case OperandType.InlineVar: - WriteInt16 ((short) GetVariableIndex ((VariableDefinition) operand)); + WriteInt16 (checked ((short) GetVariableIndex ((VariableDefinition) operand))); break; case OperandType.InlineArg: - WriteInt16 ((short) GetParameterIndex ((ParameterDefinition) operand)); + WriteInt16 (checked ((short) GetParameterIndex ((ParameterDefinition) operand))); break; case OperandType.InlineSig: WriteMetadataToken (GetStandAloneSignature ((CallSite) operand)); break; case OperandType.ShortInlineI: if (opcode == OpCodes.Ldc_I4_S) - WriteSByte ((sbyte) operand); + WriteSByte (checked ((sbyte) operand)); else - WriteByte ((byte) operand); + WriteByte (checked ((byte) operand)); break; case OperandType.InlineI: WriteInt32 ((int) operand); diff --git a/Test/Mono.Cecil.Tests/BaseTestFixture.cs b/Test/Mono.Cecil.Tests/BaseTestFixture.cs index 0112725f1..e80cd710b 100644 --- a/Test/Mono.Cecil.Tests/BaseTestFixture.cs +++ b/Test/Mono.Cecil.Tests/BaseTestFixture.cs @@ -124,6 +124,25 @@ static void Run (TestCase testCase) using (var runner = new TestRunner (testCase, TestCaseType.WriteFromImmediate)) runner.RunTest (); } + + public static void SaveModuleAndRun (ModuleDefinition module, Action action) + { + var filename = Path.GetTempFileName (); + var domain = AppDomain.CreateDomain ("test", null, AppDomain.CurrentDomain.SetupInformation); + try + { + using (var stream = File.Create (filename)) + module.Write (stream); + + domain.DoCallBack ((CrossAppDomainDelegate) Delegate.CreateDelegate (typeof (CrossAppDomainDelegate), + filename, action.Method)); + } + finally + { + AppDomain.Unload (domain); + File.Delete (filename); + } + } } abstract class TestCase { diff --git a/Test/Mono.Cecil.Tests/MethodBodyTests.cs b/Test/Mono.Cecil.Tests/MethodBodyTests.cs index 15fd5fb42..a2c6df647 100644 --- a/Test/Mono.Cecil.Tests/MethodBodyTests.cs +++ b/Test/Mono.Cecil.Tests/MethodBodyTests.cs @@ -76,6 +76,65 @@ .locals init (System.String V_0) }); } + [Test] + [ExpectedException (typeof (OverflowException))] + public void BranchOffsetTruncation () + { + TestModule ("libhello.dll", module => { + var lib = module.GetType ("Library"); + Assert.IsNotNull (lib); + + var method = lib.GetMethod ("GetHelloString"); + Assert.IsNotNull (method); + + // insert some valid IL + // truncated offset will branch between Ldnull and Ret + var insn = method.Body.Instructions.First (_ => _.OpCode == OpCodes.Ldloc_0); + var il = method.Body.GetILProcessor (); + for (int i = 0 ; i < 128 ; ++i) + { + il.InsertBefore (insn, il.Create (OpCodes.Ldnull)); + il.InsertBefore (insn, il.Create (OpCodes.Ret)); + } + + il.InsertBefore (insn, il.Create (OpCodes.Nop)); + }); + } + + [Test] + [ExpectedException (typeof (OverflowException))] + public void BranchOffsetTruncation2 () + { + TestModule ("libhello.dll", module => { + var lib = module.GetType ("Library"); + Assert.IsNotNull (lib); + + var method = lib.GetMethod ("GetHelloString"); + Assert.IsNotNull (method); + + // insert some valid IL + // truncated offset will branch before Ldnull + var insn = method.Body.Instructions.First (_ => _.OpCode == OpCodes.Ldloc_0); + var il = method.Body.GetILProcessor (); + for (int i = 0 ; i < 128 ; ++i) + { + il.InsertBefore (insn, il.Create (OpCodes.Ldnull)); + il.InsertBefore (insn, il.Create (OpCodes.Ret)); + } + + SaveModuleAndRun (module, BranchOffsetTruncation2Delegate); + }); + } + + static void BranchOffsetTruncation2Delegate (string filename) + { + var obj = Activator.CreateInstanceFrom (filename, "Library").Unwrap (); + + // should be equal because inserted instructions never execute, + // but is null without the truncation check + Assert.AreEqual ("hello world of tomorrow", obj.GetType ().GetMethod ("GetHelloString").Invoke (obj, null)); + } + [Test] public void Switch () { diff --git a/Test/Resources/assemblies/libhello.dll b/Test/Resources/assemblies/libhello.dll index 7b43e0114e71e9368eb7ac60be18927434362417..154560cb302da02796b32fbd6effab893c54868d 100644 GIT binary patch delta 524 zcmY*WOG_J37(L&enK(urOoBB*)EOc^qKKlXAcE2s6-0%QLP0AL2MvUv$%r6Qr%**V zVz>%oLA&TecS083_yYt%u=EFH-AyTOM7r>sm@fLkIddM&eayIFH+;BfolaN3M|p}6 zRu$=h%7O1CJj-7W&`!Q+8dFhV#tTP*3z4@%(xL_WCO^@3#O2PL35I}%&n9pq0Fp^< zP|k|_Mud6@yfoJcpgHF6gfG@4+IcH~Os^ zh$GFq*3;vO$v6DmVm~GCMO&SBY&K9B|Fn=bO5^rA5`3ZK5?`a8dp=v431rryo}I=S zVb=7DT@1j_Zl&zr!eF=c;=(fsvJ3F{c!daa5LSsROWchrMA3~9y3l}Ts6m%k{BH{K zT=`HrZaZU{+=iW%f0ayK+|H(w>C|R&F>Q}IeEG$7$CfE|u>3~Xss};^T z)il$bhG~5C0*f-t0@vKJLc(GzY)%}pQZ#9|Udt*D4VxxV;Q$kNT1FIj*we%N6~H?H zQQ@)C7kXzO0+S55mpr?>y}D7{A;dRjDVfdBB1s*7$rJNaIl8mtbK;fvwNn}W)H;96 zgr#(s)-X-WpOea=vZgX3Kf=Ts5ktY0`4!Uek}EddNhA-0TmX3l5hXR?X=(yKbRvNe zV$`9Q@o%c+A|7;)@Nz&ZT#U;JGe>v!_e#dOAFCd Date: Tue, 28 Nov 2017 16:24:46 +0200 Subject: [PATCH 2/2] Don't check arguments of OperandType.ShortInlineI --- Mono.Cecil.Cil/CodeWriter.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Mono.Cecil.Cil/CodeWriter.cs b/Mono.Cecil.Cil/CodeWriter.cs index c154d6046..919001831 100644 --- a/Mono.Cecil.Cil/CodeWriter.cs +++ b/Mono.Cecil.Cil/CodeWriter.cs @@ -243,9 +243,9 @@ void WriteOperand (Instruction instruction) break; case OperandType.ShortInlineI: if (opcode == OpCodes.Ldc_I4_S) - WriteSByte (checked ((sbyte) operand)); + WriteSByte ((sbyte) operand); else - WriteByte (checked ((byte) operand)); + WriteByte ((byte) operand); break; case OperandType.InlineI: WriteInt32 ((int) operand);