Storyyeller / Krakatau

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

I don't know what to call this but it's bad #70

Closed samczsun closed 8 years ago

samczsun commented 8 years ago
.method public static main : ([Ljava/lang/String;)V 
    .code stack 10 locals 10
        .catch java/lang/Throwable from L1 to L2 using L2
        .catch java/lang/Throwable from L0 to L2 using Levil
L0:
        getstatic java/lang/System oop Ljava/io/PrintStream;
L1:     invokevirtual java/io/PrintStream println ()V
        return
L2:
        return
Levil:
        getstatic java/lang/System out Ljava/io/PrintStream;
        ldc "Insert cryptolocker code here"
        invokevirtual java/io/PrintStream println (Ljava/lang/Object;)V
        return
    .end code 
.end method
    public static void main(String[] a)
    {
        try
        {
            System.oop.println();
            return;
        }
        catch(NoSuchFieldError ignoredException)
        {
            return;
        }
    }

Also, change NoSuchFieldError to Throwable to see Krakatau throw in some extra exceptions for fun

    public static void main(String[] a)
    {
        IllegalMonitorStateException a0 = null;
        try
        {
            System.oop.println();
            return;
        }
        catch(Throwable ignoredException)
        {
            try
            {
                return;
            }
            catch(IllegalMonitorStateException a1)
            {
                a0 = a1;
            }
        }
        throw a0;
    }

(Actually, upon posting I remembered that return can throw IMSE. I suppose it can be excused but it's also possible to optimize this case away by considering that there are no monitorenter instructions so an IMSE is impossible)

Does this make up for my absence yesterday? :P

Storyyeller commented 8 years ago

In the first case, I set Krakatau to assume no linkage errors occur, so the exception handler gets optimized away. It's a difficult tradeoff between being faster and outputting cleaner code in the normal case and being technically correct in weird edge cases. I've never seen code that deliberately causes a linkage error so I didn't think it would be an issue. I suppose I can revisit that if you want.

As for the second case, it's a known issue. The problem is that changing it requires a complete rewrite of the structuring code, which has long been my top priority for Krakatau (pretty much ever since I finished the current structuring code two and a half years ago in fact). The problem is that it's not exactly easy to come up with a better algorithm. It's something I've thought about a great deal from time to time, but I still haven't come up with a complete replacement algorithm.

And before you ask about the obvious option of simply removing IMSEs when there are no monitorenter instructions, I already tried that and it makes the output worse in most cases. In fact, you can see the signs of the failed implementation in the code. Look at blockmaker line 351.

samczsun commented 8 years ago

I think the it would definitely be beneficial to revisit it for the first case. My opinion (and you can feel free to take it with a grain of salt if you like) when it comes to what to things like this in particular is that output should always be accurate (and if it can't be then make sure it's clear with a comment or something else). A great example would be CFR which if it encounters backwards jumps using exceptions will throw a comment in saying that "the resulting code may differ from actual behaviour".

Now granted, I know that decompilers aren't perfect and I remember you said something around the lines of "always disassemble when reviewing malicious code". However, I do tend to heavily promote Krakatau over other decompilers just because of how much better it does, and I would hate for something like this to tarnish its reputation (especially considering all the other decompilers produce accurate, if not legal, code).

The second case doesn't really bother me as much, just something I thought I'd mention. Like I said I value accuracy over decompiled code being pretty and I don't think one or two extra exceptions will obfuscate the source code too much so leaving them low priority I can totally understand.

samczsun commented 8 years ago

Maybe you could do something like Enjarify, where there's a flag which can enable/disable certain optimizations if it really does take up too much performance. If you do decide to do that I would recommend making it clear that by disabling the flag the user could get inaccurate code.

Storyyeller commented 8 years ago

As far as linkage errors go, perhaps I could make it a command line option? There's also the question of how to handle the ability of legacy thread methods to throw arbitrary exceptions at any point. If you want to replicate that faithfully, it is impossible to optimize exceptions at all. Though I'm not sure if that is even supported in recent JVM versions.

Note that Krakatau also assumes a statically known class hierarchy, and that there isn't any funny buisness with classloaders. It also doesn't attempt to preserve reflection or debugging related metadata, because that is impossible in general. It's an endless rabbit hole.

Warning for code that won't be accurately represented in Java is definitely something on my wishlist (mostly for typecast issues). That being said, it is impossible to ever be 100% faithful to the behavior of the JVM because there are bugs in the JVM.

samczsun commented 8 years ago

That is definitely true. I suppose a command line option would be the best solution as this kind of code is an edge case at the end of the day.

samczsun commented 8 years ago

With regards to your edit, I think a good general guideline is to try and handle the bytecode itself correctly and ignore anything that may dynamically alter (eg classloader). As the instructions remain static it should be infinitely easier than worrying about things like unsafe's constant pool patches or things like machine codes ability to jump to arbitrary locations and execute the memory as code

Storyyeller commented 8 years ago

I added an experimental option which will treat all non-jump instructions as if they can throw arbitrary exceptions. You can try it out by passing -xmagicthrow. Note that this will make Krakatau much slower.