Storyyeller / Krakatau

Java decompiler, assembler, and disassembler
GNU General Public License v3.0
1.95k stars 219 forks source link

Decompiler computes incorrect local liveness. #204

Closed notiska closed 2 months ago

notiska commented 2 months ago

Not too sure if this is relevant anymore as it's related to the old Python version, but the decompiler does not compute liveness correctly. This doesn't matter most of the time, but some carefully crafted examples can cause issues.

The problematic part is when propagating liveness through exception handlers, found here in the code: https://github.com/Storyyeller/Krakatau/blob/master/Krakatau/ssa/blockmaker.py#L47 Short answer fix: do the for loop twice. Longer answer: it's caused by exception handlers not updating the liveness of the regions they cover properly when they have their liveness updated on a later iteration of the same loop.

POC: crash.zip I can also attach the file I used to generate this if you want.

Storyyeller commented 2 months ago

Thanks for providing such a detailed report. I'll try to get to it this evening.

notiska commented 2 months ago

Excellent, thanks. I've made a POC in Java, which may be easier for you to manage compared to a class file:

public class KrakCrash {
    public static void main(String[] args) {
        int x = 0;
        try {
            if (x != 0) {} 
            System.out.println("t");
            return;
        } catch (Throwable ignored) {
            try {
                ; 
            } catch (Throwable ignored2) {
            }
        }
        System.out.println(x);
    }
}
Storyyeller commented 2 months ago

Interesting. I see a crash when decompiling your crash.zip, but if I compile the Java code you provided myself and decompile that, it works fine.

notiska commented 2 months ago

Oh right that's my bad, I must've been testing with the wrong file. Yeah, it looks like it may be harder to get javac to generate the required bytecode than I thought.

notiska commented 2 months ago

On closer inspection it seems that javac won't generate back-to-back exception handlers. The above code also has its issues, this would be the corrected version:

public class KrakCrash {
    public static void main(String[] args) {
        int x = 0;
        try {
            if (x != 0) {}
            System.out.println("t");
            return;
        } catch (Throwable ignored) {
            try {
                System.out.println("t");
            } catch (Throwable ignored2) {
                System.out.println(x);
            }
        }
    }
}

To cause the crash you'll need to adjust the second exception handler manually to cover the astore instruction of the first exception handler.

Really sorry about that, I should've tested more properly.