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.

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

Conversation

atykhyy
Copy link
Contributor

@atykhyy atykhyy commented May 5, 2017

When inserting a lot of instructions, variables and method parameters into a method body that uses ldxxx.s and similar instructions to refer to existing instructions, variables and method parameters, eventually the offset or position of the item is silently truncated and no longer points to the intended item. Usually this just generates invalid or non-verifiable IL, but sometimes the result can be verifiable and yet incorrect, silently producing a difficult-to-discover bug. One of the two added tests demonstrates the latter problem. The PR changes the casts in CodeWriter.cs to checked, so that an OverflowException is thrown instead of writing a non-verifiable or erroneous module.

@jbevain jbevain self-requested a review May 5, 2017 17:56
@@ -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

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.

@atykhyy
Copy link
Contributor Author

atykhyy commented Nov 28, 2017

Removed truncation checks for OperandType.ShortInlineI because the relevant Instruction.Create overloads already validate operand range, and because modification of other instructions and/or method signature has no effect on the validity of operands of this type. @jbevain could you take a look at this, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants