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

[core] Code restructure failed for a small method #845

Closed
bagipro opened this issue Feb 4, 2020 · 5 comments
Closed

[core] Code restructure failed for a small method #845

bagipro opened this issue Feb 4, 2020 · 5 comments
Labels
bug Core Issues in jadx-core module

Comments

@bagipro
Copy link
Collaborator

bagipro commented Feb 4, 2020

The failing code with a missing assignment in class com.antivirus.o.aaf

    /* JADX WARNING: Code restructure failed: missing block: B:2:0x0006, code lost:
        r0 = r1.f3523c;
     */
    /* renamed from: h */
    public boolean mo5178h() {
        com.antivirus.p001o.aae aae;
        return super.mo5178h() || (aae != null && aae.mo5178h());
    }

its smali

.method public h()Z
    .registers 2

    .line 172
    invoke-super {p0}, Lcom/antivirus/o/aac;->h()Z

    move-result v0

    if-nez v0, :cond_13

    iget-object v0, p0, Lcom/antivirus/o/aaf;->c:Lcom/antivirus/o/aae;

    if-eqz v0, :cond_11

    invoke-virtual {v0}, Lcom/antivirus/o/aae;->h()Z

    move-result v0

    if-eqz v0, :cond_11

    goto :goto_13

    :cond_11
    const/4 v0, 0x0

    goto :goto_14

    :cond_13
    :goto_13
    const/4 v0, 0x1

    :goto_14
    return v0
.end method

Hope it can help to find a root issue much faster

APK: https://drive.google.com/file/d/159bPvATZpnVkOKOm0AiK9lHy1rFVdI1s/view?usp=sharing

@bagipro bagipro added bug Core Issues in jadx-core module labels Feb 4, 2020
@huku-
Copy link

huku- commented Feb 7, 2020

I'll look into it. The problem seems to be in IfRegionVisitor. If you edit jadx-core/src/main/java/jadx/core/Jadx.java and comment out the following line, the test-case will decompile successfully.

passes.add(new IfRegionVisitor());

I've narrowed down the problem to this line in IfRegionVisitor.java:

DepthRegionTraversal.traverseIterative(mth, TERNARY_VISITOR);

@huku-
Copy link

huku- commented Feb 7, 2020

So, here's the flow graph of the decompiled method.

method

In TernaryMod.java, in method clearConditionBlocks(), the instructions from all if-condition blocks are removed:

private static void clearConditionBlocks(List<BlockNode> conditionBlocks, BlockNode header) {
    for (BlockNode block : conditionBlocks) {
        if (block != header) {
            block.getInstructions().clear();
            block.add(AFlag.REMOVE);
        }   
    }   
}

Variable conditionBlocks contains blocks 0x0004, 0x0008 and 0x000e, which I believe are the correct blocks. However, the following line is what causes the problem:

block.getInstructions().clear();

If you comment it out, the method decompiles successfully. I can't understand why removing the instructions of the if-conditions, in this case, makes block 0x0006 disappear. Maybe @skylot can shed some light on this.

@huku-
Copy link

huku- commented Feb 8, 2020

So, turns out this issue has the same root cause as #820. The problem is that, when looking for successor if clauses, certain instructions may end up in the, so called, forced inline instructions of the successor if. This is the case with the lost code in this example. It is inserted in the forced-inline instructions of 0x0008. However, forced-inline instructions are only taken into account in the next CodeShrinkVisitor pass, which is too late, as certain optimizations taking place in intermediate passes might make such instructions disappear.

Now, regarding the fix. I guess @skylot will decide how to handle this. One solution I can see is, in clearConditionBlocks(), skipping blocks that have forced-inline instructions.

However, to solve both this and #820, I did the following. In IfMakerHelper.java, I modified the following code:

if (assignInlineNeeded) {
    for (BlockNode mergedBlock : currentIf.getMergedBlocks()) {
        if (mergedBlock.contains(AFlag.LOOP_START)) {
            // don't inline assigns into loop condition
            return currentIf;
        }   
    }   
} 

To look like this:

if (assignInlineNeeded) {
    return currentIf;
} 

This will skip merging of if nodes when there are forced-inline instructions. However, this cancels all the actions that have already taken place for detecting such instructions, but since forced-inline instruction handling looks (IMO) incomplete, it's not a very bad thing to do :)

@skylot
Copy link
Owner

skylot commented Feb 9, 2020

@huku- thanks for analysis!
Do you suggest disabling inner assignment? (it is what your code does).
I don't want to do that, I think fixing failing cases is a better way to move forward and improve jadx :)
Anyway, I made a fix to inline inner assignment before TernaryMod. And also, I made fix for #820 to allow inline inside the same block. So now both issues are fixed.

@bagipro
Copy link
Collaborator Author

bagipro commented Feb 9, 2020

@skylot
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Core Issues in jadx-core module
Projects
None yet
Development

No branches or pull requests

3 participants