1

Closed

New decompiler may corrupt variable initialization

description

When using the new decompiler, I've run into a case where a variable initialization expression is assigned to a newly-introduced variable (which is never used anywhere else), and the variable that should have been initialized is left uninitialized.
 
The bug can be seen when running the PeToPeViaCodeModel sample, after updating it to use the new decompiler. I've provided a DLL to use as input that illustrates the problem.
 
In the attached ZIP file, I've also got IL source files showing the disassembly of the Pe2Pe output, as well as a hand-modified version to show where the error is when the files are diff'd.
 
Please see the readme.txt file in the attached ZIP, for more details on reproducing the bug.
 
I will attempt to reduce the DLL to a minimal repro, but it may take some time and thought I might as well submit what I have now in case it isn't necessary to minimize.

file attachments

Closed Jan 5, 2013 at 1:06 AM by hermanv

comments

BadCorporateLogo wrote Apr 6, 2012 at 4:46 PM

Okay, I've isolated the problem in a much smaller repro, and have attached them.

The source file is BadVariableInit.cs, and the problem occurs for variable b. I've also included the disassembled IL for the small repro - the variable V_1 is introduced and is initialized instead of b.

When this source file is compiled into a DLL and given to PeToPeViaCodeModel (using the new decompiler), the rewritten PE contains the IL as shown in the attached badinit.rewritten.il file.

BadCorporateLogo wrote Apr 6, 2012 at 4:49 PM

I've isolated the problem down to a few lines: see minimal repro in BadVariableInit.zip.

The source file is BadVariableInit.cs, and the problem occurs for variable b. I've also included the disassembled IL for the small repro - the variable V_1 is introduced and is initialized instead of b.

When this source file is compiled into a DLL and given to PeToPeViaCodeModel (using the new decompiler), the rewritten PE contains the IL as shown in the attached badinit.rewritten.il file.

BadCorporateLogo wrote Apr 6, 2012 at 10:20 PM

Sorry for the double comment earlier - my browser hung the first time so I posted again.

I believe the problem is in the DeclarationUnifier. The TraverseChildren(IBlockStatement) function appears to be trying to move the LocalDeclarationStatement to the location of the first assignment. The loop is only looking for explicit assignments, and doesn't recognize an address reference as a possible assignment. In cases where the variable is initialized with a value type constructor, or as an out param in a function call, this function moves the variable declaration past its first use.

After that, I'm guessing the IL generator declares a new local to fill in for the actual first use, where the reference is to an undeclared local.

hermanv wrote Apr 18, 2012 at 10:10 PM

Thanks for looking into this and getting me a nice repro. Change set 68818 contains (among other things) a fix for this problem.

Sorry about taking so long to get to this (I was on vacation).

BadCorporateLogo wrote Apr 20, 2012 at 1:37 AM

Hi Herman,
Change 68818 works for the repro that I attached earlier, but not all cases affected by the bug. There are two additional scenarios for initialization: out params and explicit initialization of value types.

The patch that I uploaded addresses these, but I admit there is probably a more elegant way to write it. I apologize for not uploading the additional scenarios I used to test my patch.

I'm attaching an expanded repro that includes scenarios that remain unfixed. If you search for "V_" in the badinit.rewritten.il file, you'll see the remaining issues.

New attachment is badinit_more_repro_scenarios.zip. Thanks!

hermanv wrote Apr 20, 2012 at 10:14 PM

The additional cases are now fixed as well. See revision: 68846.

BadCorporateLogo wrote Apr 20, 2012 at 11:23 PM

I attached the wrong source file for the additional scenarios. Please see BadVariableInit.cs.