jessikitty/jadx
1
0
Fork 0
Code Issues Pull Requests 1 Actions 1 Packages Projects Releases Wiki Activity

fix: check variables before merge in finally block (#1592)

Browse Source
This commit is contained in:
Skylot
2022-07-30 13:48:53 +01:00
parent 6802f6028e
commit 03e4afb12f
6 changed files with 229 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files
jadx-core/src/main/java/jadx/core/dex/visitors/finaly/FinallyExtractInfo.java
+26
View File
@@ -6,6 +6,7 @@ import java.util.List;
import java.util.Set;
import jadx.core.dex.nodes.BlockNode;
import jadx.core.dex.nodes.InsnNode;
import jadx.core.dex.nodes.MethodNode;
import jadx.core.dex.trycatch.ExceptionHandler;
import jadx.core.utils.Utils;
@@ -19,6 +20,10 @@ public class FinallyExtractInfo {
private final InsnsSlice finallyInsnsSlice = new InsnsSlice();
private final BlockNode startBlock;
private InsnsSlice curDupSlice;
private List<InsnNode> curDupInsns;
private int curDupInsnsOffset;
public FinallyExtractInfo(MethodNode mth, ExceptionHandler finallyHandler, BlockNode startBlock, List<BlockNode> allHandlerBlocks) {
this.mth = mth;
this.finallyHandler = finallyHandler;
@@ -54,6 +59,27 @@ public class FinallyExtractInfo {
return startBlock;
}
public InsnsSlice getCurDupSlice() {
return curDupSlice;
}
public void setCurDupSlice(InsnsSlice curDupSlice) {
this.curDupSlice = curDupSlice;
}
public List<InsnNode> getCurDupInsns() {
return curDupInsns;
}
public int getCurDupInsnsOffset() {
return curDupInsnsOffset;
}
public void setCurDupInsns(List<InsnNode> insns, int offset) {
this.curDupInsns = insns;
this.curDupInsnsOffset = offset;
}
@Override
public String toString() {
return "FinallyExtractInfo{"
jadx-core/src/main/java/jadx/core/dex/visitors/finaly/MarkFinallyVisitor.java
+83 -18
View File
@@ -12,6 +12,7 @@ import org.slf4j.LoggerFactory;
import jadx.core.Consts;
import jadx.core.dex.attributes.AFlag;
import jadx.core.dex.attributes.AType;
import jadx.core.dex.attributes.nodes.RegDebugInfoAttr;
import jadx.core.dex.instructions.InsnType;
import jadx.core.dex.instructions.args.InsnArg;
import jadx.core.dex.instructions.args.RegisterArg;
@@ -28,6 +29,7 @@ import jadx.core.dex.visitors.IDexTreeVisitor;
import jadx.core.dex.visitors.JadxVisitor;
import jadx.core.dex.visitors.ssa.SSATransform;
import jadx.core.utils.BlockUtils;
import jadx.core.utils.InsnList;
import jadx.core.utils.ListUtils;
import jadx.core.utils.Utils;
@@ -376,6 +378,7 @@ public class MarkFinallyVisitor extends AbstractVisitor {
* 'Finally' instructions can start in the middle of the first block.
*/
private static InsnsSlice isStartBlock(BlockNode dupBlock, BlockNode finallyBlock, FinallyExtractInfo extractInfo) {
extractInfo.setCurDupSlice(null);
List<InsnNode> dupInsns = dupBlock.getInstructions();
List<InsnNode> finallyInsns = finallyBlock.getInstructions();
int dupSize = dupInsns.size();
@@ -386,7 +389,7 @@ public class MarkFinallyVisitor extends AbstractVisitor {
int startPos;
int endPos = 0;
if (dupSize == finSize) {
if (!checkInsns(dupInsns, finallyInsns, 0)) {
if (!checkInsns(extractInfo, dupInsns, finallyInsns, 0)) {
return null;
}
startPos = 0;
@@ -394,11 +397,11 @@ public class MarkFinallyVisitor extends AbstractVisitor {
// dupSize > finSize
startPos = dupSize - finSize;
// fast check from end of block
if (!checkInsns(dupInsns, finallyInsns, startPos)) {
if (!checkInsns(extractInfo, dupInsns, finallyInsns, startPos)) {
// search start insn
boolean found = false;
for (int i = 1; i < startPos; i++) {
if (checkInsns(dupInsns, finallyInsns, i)) {
if (checkInsns(extractInfo, dupInsns, finallyInsns, i)) {
startPos = i;
endPos = finSize + i;
found = true;
@@ -414,6 +417,7 @@ public class MarkFinallyVisitor extends AbstractVisitor {
// put instructions into slices
boolean complete;
InsnsSlice slice = new InsnsSlice();
extractInfo.setCurDupSlice(slice);
int endIndex;
if (endPos != 0) {
endIndex = endPos + 1;
@@ -453,11 +457,12 @@ public class MarkFinallyVisitor extends AbstractVisitor {
return slice;
}
private static boolean checkInsns(List<InsnNode> remInsns, List<InsnNode> finallyInsns, int delta) {
private static boolean checkInsns(FinallyExtractInfo extractInfo, List<InsnNode> dupInsns, List<InsnNode> finallyInsns, int delta) {
extractInfo.setCurDupInsns(dupInsns, delta);
for (int i = finallyInsns.size() - 1; i >= 0; i--) {
InsnNode startInsn = finallyInsns.get(i);
InsnNode remInsn = remInsns.get(delta + i);
if (!sameInsns(remInsn, startInsn)) {
InsnNode dupInsn = dupInsns.get(delta + i);
if (!sameInsns(extractInfo, dupInsn, startInsn)) {
return false;
}
}
@@ -509,8 +514,9 @@ public class MarkFinallyVisitor extends AbstractVisitor {
if (dupInsnCount < finallyInsnCount) {
return false;
}
extractInfo.setCurDupInsns(dupInsns, 0);
for (int i = 0; i < finallyInsnCount; i++) {
if (!sameInsns(dupInsns.get(i), finallyInsns.get(i))) {
if (!sameInsns(extractInfo, dupInsns.get(i), finallyInsns.get(i))) {
return false;
}
}
@@ -524,26 +530,85 @@ public class MarkFinallyVisitor extends AbstractVisitor {
return true;
}
private static boolean sameInsns(InsnNode remInsn, InsnNode fInsn) {
if (!remInsn.isSame(fInsn)) {
private static boolean sameInsns(FinallyExtractInfo extractInfo, InsnNode dupInsn, InsnNode fInsn) {
if (!dupInsn.isSame(fInsn)) {
return false;
}
// TODO: check instance arg in ConstructorInsn
// TODO: compare literals
for (int i = 0; i < remInsn.getArgsCount(); i++) {
InsnArg remArg = remInsn.getArg(i);
for (int i = 0; i < dupInsn.getArgsCount(); i++) {
InsnArg dupArg = dupInsn.getArg(i);
InsnArg fArg = fInsn.getArg(i);
if (remArg.isRegister() != fArg.isRegister()) {
if (!isSameArgs(extractInfo, dupArg, fArg)) {
return false;
}
boolean remConst = remArg.isConst();
if (remConst != fArg.isConst()) {
return false;
}
if (remConst && !remArg.isSameConst(fArg)) {
}
return true;
}
@SuppressWarnings("RedundantIfStatement")
private static boolean isSameArgs(FinallyExtractInfo extractInfo, InsnArg dupArg, InsnArg fArg) {
boolean isReg = dupArg.isRegister();
if (isReg != fArg.isRegister()) {
return false;
}
if (isReg) {
RegisterArg dupReg = (RegisterArg) dupArg;
RegisterArg fReg = (RegisterArg) fArg;
if (!dupReg.sameCodeVar(fReg)
&& !sameDebugInfo(dupReg, fReg)
&& assignedOutsideHandler(extractInfo, dupReg, fReg)
&& assignInsnDifferent(dupReg, fReg)) {
return false;
}
}
boolean remConst = dupArg.isConst();
if (remConst != fArg.isConst()) {
return false;
}
if (remConst && !dupArg.isSameConst(fArg)) {
return false;
}
return true;
}
private static boolean sameDebugInfo(RegisterArg dupReg, RegisterArg fReg) {
RegDebugInfoAttr fDbgInfo = fReg.get(AType.REG_DEBUG_INFO);
RegDebugInfoAttr dupDbgInfo = dupReg.get(AType.REG_DEBUG_INFO);
if (fDbgInfo == null || dupDbgInfo == null) {
return false;
}
return dupDbgInfo.equals(fDbgInfo);
}
private static boolean assignInsnDifferent(RegisterArg dupReg, RegisterArg fReg) {
InsnNode assignInsn = fReg.getAssignInsn();
InsnNode dupAssign = dupReg.getAssignInsn();
if (assignInsn == null || dupAssign == null) {
return true;
}
if (!assignInsn.isSame(dupAssign)) {
return true;
}
if (assignInsn.isConstInsn() && dupAssign.isConstInsn()) {
return !assignInsn.isDeepEquals(dupAssign);
}
return false;
}
@SuppressWarnings("RedundantIfStatement")
private static boolean assignedOutsideHandler(FinallyExtractInfo extractInfo, RegisterArg dupReg, RegisterArg fReg) {
if (InsnList.contains(extractInfo.getFinallyInsnsSlice().getInsnsList(), fReg.getAssignInsn())) {
return false;
}
InsnNode dupAssign = dupReg.getAssignInsn();
InsnsSlice curDupSlice = extractInfo.getCurDupSlice();
if (curDupSlice != null && InsnList.contains(curDupSlice.getInsnsList(), dupAssign)) {
return false;
}
List<InsnNode> curDupInsns = extractInfo.getCurDupInsns();
if (Utils.notEmpty(curDupInsns) && InsnList.contains(curDupInsns, dupAssign, extractInfo.getCurDupInsnsOffset())) {
return false;
}
return true;
}
jadx-core/src/main/java/jadx/core/utils/InsnList.java
+13 -1
View File
@@ -29,8 +29,12 @@ public final class InsnList implements Iterable<InsnNode> {
}
public static int getIndex(List<InsnNode> list, InsnNode insn) {
return getIndex(list, insn, 0);
}
public static int getIndex(List<InsnNode> list, InsnNode insn, int startOffset) {
int size = list.size();
for (int i = 0; i < size; i++) {
for (int i = startOffset; i < size; i++) {
if (list.get(i) == insn) {
return i;
}
@@ -38,6 +42,14 @@ public final class InsnList implements Iterable<InsnNode> {
return -1;
}
public static boolean contains(List<InsnNode> list, InsnNode insn) {
return getIndex(list, insn, 0) != -1;
}
public static boolean contains(List<InsnNode> list, InsnNode insn, int startOffset) {
return getIndex(list, insn, startOffset) != -1;
}
public int getIndex(InsnNode insn) {
return getIndex(list, insn);
}
jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatchFinally15.java
+44
View File
@@ -0,0 +1,44 @@
package jadx.tests.integration.trycatch;
import org.junit.jupiter.api.Test;
import jadx.tests.api.SmaliTest;
import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat;
/**
* Negative test case for finally extract (issue 1592).
* Different registers incorrectly merged into one.
*/
@SuppressWarnings({ "CommentedOutCode", "GrazieInspection" })
public class TestTryCatchFinally15 extends SmaliTest {
// @formatter:off
/*
protected final Parcel test(int i, Parcel parcel) throws RemoteException {
Parcel obtain = Parcel.obtain();
try {
try {
this.zza.transact(i, parcel, obtain, 0);
obtain.readException();
return obtain;
} catch (RuntimeException e) {
obtain.recycle();
throw e;
}
} finally {
parcel.recycle();
}
}
*/
// @formatter:on
@Test
public void test() {
disableCompilation();
assertThat(getClassNodeFromSmali())
.code()
.doesNotContain("parcel = Parcel.obtain();")
.containsOne("this.zza.transact(i, parcel, obtain, 0);");
}
}
jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatchFinally6.java
+3 -10
View File
@@ -10,6 +10,7 @@ import jadx.core.dex.nodes.ClassNode;
import jadx.tests.api.IntegrationTest;
import static jadx.tests.api.utils.JadxMatchers.containsLines;
import static jadx.tests.api.utils.JadxMatchers.containsOne;
import static org.hamcrest.MatcherAssert.assertThat;
public class TestTryCatchFinally6 extends IntegrationTest {
@@ -54,15 +55,7 @@ public class TestTryCatchFinally6 extends IntegrationTest {
ClassNode cls = getClassNode(TestCls.class);
String code = cls.getCode().toString();
assertThat(code, containsLines(2,
"FileInputStream fileInputStream = null;",
"try {",
indent() + "call();",
indent() + "fileInputStream = new FileInputStream(\"1.txt\");",
"} finally {",
indent() + "if (fileInputStream != null) {",
indent() + indent() + "fileInputStream.close();",
indent() + '}',
"}"));
// impossible to proof that variables should be merged, so can't restore finally block here
assertThat(code, containsOne("if (0 != 0) {"));
}
}
jadx-core/src/test/smali/trycatch/TestTryCatchFinally15.smali
+60
View File
@@ -0,0 +1,60 @@
.class public Ltrycatch/TestTryCatchFinally15;
.super Ljava/lang/Object;
.implements Landroid/os/IInterface;
.field private final zza:Landroid/os/IBinder;
.field private final zzb:Ljava/lang/String;
.method protected final test(ILandroid/os/Parcel;)Landroid/os/Parcel;
.registers 6
.annotation system Ldalvik/annotation/Throws;
value = {
Landroid/os/RemoteException;
}
.end annotation
.line 1
invoke-static {}, Landroid/os/Parcel;->obtain()Landroid/os/Parcel;
move-result-object v0
:try_start_4
iget-object v1, p0, Ltrycatch/TestTryCatchFinally15;->zza:Landroid/os/IBinder;
const/4 v2, 0x0
.line 2
invoke-interface {v1, p1, p2, v0, v2}, Landroid/os/IBinder;->transact(ILandroid/os/Parcel;Landroid/os/Parcel;I)Z
.line 3
invoke-virtual {v0}, Landroid/os/Parcel;->readException()V
:try_end_d
.catch Ljava/lang/RuntimeException; {:try_start_4 .. :try_end_d} :catch_13
.catchall {:try_start_4 .. :try_end_d} :catchall_11
.line 6
invoke-virtual {p2}, Landroid/os/Parcel;->recycle()V
return-object v0
:catchall_11
move-exception p1
goto :goto_18
.line 5
:catch_13
move-exception p1
.line 4
:try_start_14
invoke-virtual {v0}, Landroid/os/Parcel;->recycle()V
.line 5
throw p1
:try_end_18
.catchall {:try_start_14 .. :try_end_18} :catchall_11
.line 6
:goto_18
invoke-virtual {p2}, Landroid/os/Parcel;->recycle()V
.line 7
throw p1
.end method