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

Modern versions of ILSpy do not handle aggressive branching very well. #2878

Closed
ElektroKill opened this issue Dec 31, 2022 · 1 comment
Closed
Labels
Bug Decompiler The decompiler engine itself

Comments

@ElektroKill
Copy link
Contributor

Input code

Binary:
KeyGenMe-SA2.zip
Method: KeyGenMe.MainForm.btnVerify_Click, Metadata token: 0x0600000B

Erroneous output produced by ILSpy 8.0-preview3

private void btnVerify_Click(object sender, EventArgs e)
{
	//IL_0054: Incompatible stack heights: 0 vs 1
	//IL_0057: Incompatible stack heights: 0 vs 2
	//IL_005e: Incompatible stack heights: 0 vs 1
	//IL_0061: Incompatible stack heights: 0 vs 1
	//IL_0068: Incompatible stack heights: 0 vs 1
	//IL_0072: Incompatible stack heights: 0 vs 1
	//IL_0079: Incompatible stack heights: 0 vs 1
	//IL_0080: Incompatible stack heights: 0 vs 1
	try
	{
		_ = ((MainForm)/*Error near IL_0007: Stack underflow*/).valid;
		_ = ((MainForm)/*Error near IL_000e: Stack underflow*/).txtUserName.Text;
		_ = ((MainForm)/*Error near IL_0017: Stack underflow*/).txtSerial.Text;
		((SerialValidator)/*Error near IL_006f: Stack underflow*/).ValidateSerial((string)/*Error near IL_006f: Stack underflow*/, (string)/*Error near IL_006f: Stack underflow*/);
		_ = ((MainForm)/*Error near IL_0022: Stack underflow*/).valid.SerialValid;
		if ((int)/*Error near IL_0026: Stack underflow*/ == 0)
		{
			MessageBox.Show(valid.GetError(), "KeyGenMe!", MessageBoxButtons.OK, MessageBoxIcon.Hand);
		}
		else
		{
			MessageBox.Show("Verification Successful!", "KeyGenMe!", MessageBoxButtons.OK, MessageBoxIcon.Asterisk);
		}
	}
	finally
	{
		valid.Reset();
	}
}

This problem exists in ILSpy versions all the way back to 3.0, the pre-rewrite 2.4 engine decompiles this method successfully.

ILSpy 2.4 decompilation:

private void btnVerify_Click(object sender, EventArgs e)
{
	try
	{
		this.valid.ValidateSerial(this.txtUserName.Text, this.txtSerial.Text);
		if (this.valid.SerialValid)
		{
			MessageBox.Show("Verification Successful!", "KeyGenMe!", MessageBoxButtons.OK, MessageBoxIcon.Asterisk);
		}
		else
		{
			MessageBox.Show(this.valid.GetError(), "KeyGenMe!", MessageBoxButtons.OK, MessageBoxIcon.Hand);
		}
	}
	finally
	{
		this.valid.Reset();
	}
}

IL code for btnVerify_Click

.method private hidebysig 
	instance void btnVerify_Click (
		object sender,
		class [mscorlib]System.EventArgs e
	) cil managed 
{
	// Method begins at RVA 0x255c
	// Header size: 12
	// Code size: 143 (0x8f)
	.maxstack 4

	.try
	{
		IL_0000: br.s IL_0053

		IL_0002: ldfld class KeyGenMe.SerialValidator KeyGenMe.MainForm::valid
		IL_0007: br.s IL_0056

		IL_0009: ldfld class [System.Windows.Forms]System.Windows.Forms.TextBox KeyGenMe.MainForm::txtUserName
		IL_000e: br.s IL_0059

		IL_0010: br.s IL_0060

		IL_0012: ldfld class [System.Windows.Forms]System.Windows.Forms.TextBox KeyGenMe.MainForm::txtSerial
		IL_0017: br.s IL_0063

		IL_0019: br.s IL_006a

		IL_001b: br.s IL_0071

		IL_001d: ldfld class KeyGenMe.SerialValidator KeyGenMe.MainForm::valid
		IL_0022: br.s IL_0074

		IL_0024: brfalse.s IL_0038

		IL_0026: ldstr "Verification Successful!"
		IL_002b: ldstr "KeyGenMe!"
		IL_0030: ldc.i4.0
		IL_0031: ldc.i4.s 64
		IL_0033: br.s IL_007b

		IL_0035: pop
		IL_0036: leave.s IL_008e

		IL_0038: ldarg.0
		IL_0039: ldfld class KeyGenMe.SerialValidator KeyGenMe.MainForm::valid
		IL_003e: callvirt instance string KeyGenMe.SerialValidator::GetError()
		IL_0043: ldstr "KeyGenMe!"
		IL_0048: ldc.i4.0
		IL_0049: ldc.i4.s 16
		IL_004b: call valuetype [System.Windows.Forms]System.Windows.Forms.DialogResult [System.Windows.Forms]System.Windows.Forms.MessageBox::Show(string, string, valuetype [System.Windows.Forms]System.Windows.Forms.MessageBoxButtons, valuetype [System.Windows.Forms]System.Windows.Forms.MessageBoxIcon)
		IL_0050: pop
		IL_0051: leave.s IL_008e

		IL_0053: ldarg.0
		IL_0054: br.s IL_0002

		IL_0056: ldarg.0
		IL_0057: br.s IL_0009

		IL_0059: callvirt instance string [System.Windows.Forms]System.Windows.Forms.Control::get_Text()
		IL_005e: br.s IL_0010

		IL_0060: ldarg.0
		IL_0061: br.s IL_0012

		IL_0063: callvirt instance string [System.Windows.Forms]System.Windows.Forms.Control::get_Text()
		IL_0068: br.s IL_0019

		IL_006a: callvirt instance void KeyGenMe.SerialValidator::ValidateSerial(string, string)
		IL_006f: br.s IL_001b

		IL_0071: ldarg.0
		IL_0072: br.s IL_001d

		IL_0074: callvirt instance bool KeyGenMe.SerialValidator::get_SerialValid()
		IL_0079: br.s IL_0024

		IL_007b: call valuetype [System.Windows.Forms]System.Windows.Forms.DialogResult [System.Windows.Forms]System.Windows.Forms.MessageBox::Show(string, string, valuetype [System.Windows.Forms]System.Windows.Forms.MessageBoxButtons, valuetype [System.Windows.Forms]System.Windows.Forms.MessageBoxIcon)
		IL_0080: br.s IL_0035
	} // end .try
	finally
	{
		IL_0082: ldarg.0
		IL_0083: ldfld class KeyGenMe.SerialValidator KeyGenMe.MainForm::valid
		IL_0088: callvirt instance void KeyGenMe.SerialValidator::Reset()
		IL_008d: endfinally
	} // end handler

	IL_008e: ret
} // end of method MainForm::btnVerify_Click

This is rather exotic IL code but it does execute without issues on the CLR.

This difference seems to come down to the approach used to build the initial ILAst model. The current decompiler processes instructions in the order in which they appear in the body. However, the older engine used a different approach as can be seen here:
https://github.com/icsharpcode/ILSpy/blob/v2.4/ICSharpCode.Decompiler/ILAst/ILAstBuilder.cs#L313
The older decompiler processes instructions in an order more reminiscent of that of the actual execution flow.

Details

  • Product in use: ILSpy
  • Version in use: 8.0-preview4 and 2.4
@ElektroKill ElektroKill added Bug Decompiler The decompiler engine itself labels Dec 31, 2022
@dgrunwald
Copy link
Member

That IL code is invalid according to the ECMA spec. ILSpy currently uses a simple linear single pass to determine the stack sizes and types.
The .NET runtime uses a significantly more complex process where individual blocks are potentially even "reimported" multiple times to gradually improve the runtime's knowledge of the stack types. See also #901 (comment)

dgrunwald added a commit that referenced this issue Jun 1, 2023
…orts

This makes our logic more similar to that used by the dotnet runtime. This lets us infer correct stack types in edge cases such as #2401. It also improves support for obfuscated control flow such as #2878.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Decompiler The decompiler engine itself
Projects
None yet
Development

No branches or pull requests

2 participants