Skip to content
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.

Sign up for GitHub

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

Add truncation checks for short forms of some IL instructions #393

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add truncation checks for short forms of some IL instructions #393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
497c339
Add truncation checks for short forms of some IL instructions
atykhyy May 5, 2017
cdbfac1
Don't check arguments of OperandType.ShortInlineI
atykhyy Nov 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions Mono.Cecil.Cil/CodeWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could do checked { .. } over whole switch statement

Copy link
Contributor Author

@atykhyy atykhyy May 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might not be a good idea for the long forms of the Ldc instructions. Truncating Ldc_i4_S 0x123 to Ldc_i4_S 0x23 is almost certainly a bug and deserves throwing an exception, but what about unsigned integer constants like Ldc_i4 0xffffffffu? That's probably not a bug, because there is no separate Ldc instruction for unsigned integers, but checked ((int) 0xffffffffu) will throw. On the other hand, truncating significant bits off a long for Ldc_i4 is just as bad as truncating significant digits off an int for Ldc_i4_S, not to mention silently casting a floating-point number to an integer, so there is a case for more thorough checking than I've added here. On the gripping hand, Instruction.Create overloads check for the correct instruction operand kind, and if somebody directly assigns Operand or OpCode to an incorrect type they ought to have known what they were doing. Anyway, the real point of this PR is the branch cast checks, because branch offsets can change through normal actions on the ILGenerator whereas the constants in Ldc_xxx instructions can't.

break;
}
case OperandType.InlineBrTarget: {
Expand All @@ -227,16 +227,16 @@ 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));
Expand Down
19 changes: 19 additions & 0 deletions Test/Mono.Cecil.Tests/BaseTestFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string> 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 {
Expand Down
59 changes: 59 additions & 0 deletions Test/Mono.Cecil.Tests/MethodBodyTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ()
{
Expand Down
Binary file modified Test/Resources/assemblies/libhello.dll
View file
Open in desktop
Binary file not shown.