Skip to content

Commit ea78c3a

Browse files
nklapstenklapste
committed
fix: always pivot through constructors in findStatementInsn to prevent lost comments
The previous canPivotThroughConstructor guard blocked the pivot when any InvokeNode appeared between <init> and its post-ctor user, intending to detect cases where JADX would keep the variable as an explicit declaration. The heuristic was wrong: JADX can still inline the constructor even with a blocking invoke present (e.g. Object.getClass()), because that invoke gets itself inlined into the consumer first, clearing the path. Removing the guard means comments always walk past <init> to the enclosing statement. This fixes b64: getPackagesForUid disappearing entirely — the comment was landing on the result-less invoke-direct <init> node and then being lost when ConstructorVisitor merged new-instance + init. Also adds the SYNTHETIC-instruction stop that was already documented but missing from this branch of the walk, and bumps jadx to 1.5.5. New test fixtures (b64_reflect_trycatch, b64_cached_reflection, b64_in_trycatch, b64_conditional_trycatch) cover the various try-catch + conditional branching patterns that triggered the regression.
1 parent ba23bc3 commit ea78c3a

7 files changed

Lines changed: 653 additions & 14 deletions

File tree

‎build.gradle.kts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ plugins {
1313
}
1414

1515
dependencies {
16-
val jadxVersion = "1.5.1"
16+
val jadxVersion = "1.5.5"
1717
val isJadxSnapshot = jadxVersion.endsWith("-SNAPSHOT")
1818

1919
// use compile only scope to exclude jadx-core and its dependencies from result jar

‎src/main/java/jadx/plugins/stringdecoder/B64DeobfuscatePass.java‎

Lines changed: 32 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import jadx.api.plugins.pass.impl.OrderedJadxPassInfo;
1919
import jadx.api.plugins.pass.types.JadxDecompilePass;
2020
import jadx.core.codegen.utils.CodeComment;
21+
import jadx.core.dex.attributes.AFlag;
2122
import jadx.core.dex.attributes.AType;
2223
import jadx.core.dex.instructions.ConstStringNode;
2324
import jadx.core.dex.instructions.FilledNewArrayNode;
@@ -46,6 +47,7 @@ public JadxPassInfo getInfo() {
4647
"B64Deobfuscate",
4748
"Detect and decode likely Base64-encoded string constants")
4849
.after("SSATransform")
50+
.after("MarkFinallyVisitor")
4951
.before("ConstInlineVisitor");
5052
}
5153

@@ -172,6 +174,12 @@ public void visit(MethodNode mth) {
172174
* Attaching CODE_COMMENTS there — using the typed, list-merging addAttr — avoids the
173175
* replace-on-copy problem in inheritMetadata when multiple decoded strings live in the
174176
* same chained expression.
177+
*
178+
* Constructor pivot: when the chain enters an {@code invoke-direct <init>}, we pivot
179+
* through to the post-constructor user (the single non-init user of the new-instance
180+
* register) and continue walking from there. This ensures the comment always lands on
181+
* the enclosing statement rather than on the result-less {@code <init>} node, which
182+
* would be lost when ConstructorVisitor later merges new-instance + init.
175183
*/
176184
private static InsnNode findStatementInsn(ConstStringNode csn) {
177185
RegisterArg result = csn.getResult();
@@ -194,8 +202,9 @@ private static InsnNode findStatementInsn(ConstStringNode csn) {
194202
}
195203
RegisterArg useResult = useInsn.getResult();
196204
if (useResult == null) {
197-
// Constructor call (invoke-direct on <init>): the constructed object flows through
198-
// arg[0] rather than a result register — pivot to its SSAVar and continue tracing.
205+
// No result register: terminal statement (void call, constructor, sput, return, etc.).
206+
// For constructors, pivot through to the post-constructor user so the comment lands
207+
// on the enclosing statement rather than the result-less <init> node.
199208
if (useInsn instanceof InvokeNode && ((InvokeNode) useInsn).getCallMth().isConstructor()) {
200209
InsnArg arg0 = useInsn.getArg(0);
201210
if (arg0 instanceof RegisterArg) {
@@ -224,24 +233,36 @@ private static InsnNode findStatementInsn(ConstStringNode csn) {
224233
if (useVar == null || useVar.getUseList().isEmpty()) {
225234
return useInsn;
226235
}
236+
// Don't walk into SYNTHETIC instructions (control-flow merge points that JADX generates
237+
// but doesn't render directly). When useInsn's result is only consumed by a SYNTHETIC
238+
// instruction, the current instruction is the last one that produces visible code.
239+
if (useVar.getUseList().size() == 1) {
240+
InsnNode soleConsumer = useVar.getUseList().get(0).getParentInsn();
241+
if (soleConsumer != null && soleConsumer.contains(AFlag.SYNTHETIC)) {
242+
break;
243+
}
244+
}
227245
current = useInsn;
228246
var = useVar;
229247
}
230248
return current;
231249
}
232250

233-
/** Returns the single use of {@code ctorVar} that is not the constructor call itself, or null. */
234-
private static InsnNode singlePostCtorUser(SSAVar ctorVar, InsnNode ctorInsn) {
251+
/**
252+
* Returns the single use of {@code ctorVar} that is not {@code ctor} itself, or null if
253+
* there are zero or more than one such uses.
254+
*/
255+
private static InsnNode singlePostCtorUser(SSAVar ctorVar, InsnNode ctor) {
256+
List<RegisterArg> uses = ctorVar.getUseList();
235257
InsnNode found = null;
236-
for (RegisterArg use : ctorVar.getUseList()) {
258+
for (RegisterArg use : uses) {
237259
InsnNode parent = use.getParentInsn();
238-
if (parent == null || parent == ctorInsn) {
239-
continue;
240-
}
241-
if (found != null) {
242-
return null;
260+
if (parent != null && parent != ctor) {
261+
if (found != null) {
262+
return null;
263+
}
264+
found = parent;
243265
}
244-
found = parent;
245266
}
246267
return found;
247268
}

‎src/test/java/jadx/plugins/stringdecoder/JadxStringDecoderPluginTest.java‎

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -420,10 +420,10 @@ public void pascalCaseSkipDisabledTest() throws Exception {
420420

421421
@Test
422422
public void multiB64InvokeChainTest() throws Exception {
423-
// Two const-strings both used as direct Base64.decode args in the same method chain:
423+
// Two const-strings both used as direct Base64.decode args in the same fully-inlined chain:
424424
// Class.forName(new String(Base64.decode("...", 0)))
425425
// .getMethod(new String(Base64.decode("...", 0)), ...)
426-
// Both should get indexed b64 comments in left-to-right source order
426+
// Both are grouped under the top-level statement and emitted as indexed comments.
427427
String code = decompileSmali("b64/multi_b64_invoke.smali");
428428
System.out.println(code);
429429
assertThat(code).contains("b64[0]: android.app.ActivityThread");
@@ -440,6 +440,45 @@ public void base64EncoderInnerClassTest() throws Exception {
440440
assertThat(code).contains("bytes: \"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_\"");
441441
}
442442

443+
@Test
444+
public void b64ReflectTryCatchTest() throws Exception {
445+
// Multiple Base64.decode strings inside try-catch blocks with conditional branching.
446+
// ActivityThread+getPackageManager are in a fully-inlined chain → grouped as indexed comments.
447+
// getPackageInfo has blocking invokes between its constructor and use → kept as an explicit
448+
// variable str2, so the comment lands on str2 = new String(...) rather than on getMethod.
449+
String code = decompileSmali("b64/b64_reflect_trycatch.smali");
450+
System.out.println(code);
451+
assertThat(code).contains("b64: android.os.UserHandle");
452+
assertThat(code).contains("b64: getUserId");
453+
assertThat(code).contains("b64[0]: android.app.ActivityThread");
454+
assertThat(code).contains("b64[1]: getPackageManager");
455+
assertThat(code).contains("b64: getPackageInfo");
456+
}
457+
458+
459+
@Test
460+
public void b64CachedReflectionTest() throws Exception {
461+
// Three Base64.decode strings in a cached-reflection pattern with try-catch + conditionals.
462+
// ActivityThread+getPackageManager are in a fully-inlined chain → grouped as indexed comments.
463+
// getPackagesForUid has a blocking Object.getClass() between its constructor and getMethod,
464+
// but JADX still inlines the constructor, so the comment lands on the getMethod statement.
465+
String code = decompileSmali("b64/b64_cached_reflection.smali");
466+
System.out.println(code);
467+
assertThat(code).contains("b64[0]: android.app.ActivityThread");
468+
assertThat(code).contains("b64[1]: getPackageManager");
469+
assertThat(code).contains("b64: getPackagesForUid");
470+
}
471+
472+
@Test
473+
public void b64InTryCatchTest() throws Exception {
474+
// new String(Base64.decode("Z2V0UGFja2FnZUluZm8=", 0)) inside a .catchall block.
475+
// "getPackageInfo" is camelCase and would normally be filtered, but the explicit
476+
// Base64.decode call triggers decodeForced() which bypasses heuristics.
477+
String code = decompileSmali("b64/b64_in_trycatch.smali");
478+
System.out.println(code);
479+
assertThat(code).contains("b64: getPackageInfo");
480+
}
481+
443482
@Test
444483
public void byteArrayStringPassDisabledTest() throws Exception {
445484
// with enableByteArrayStringPass=false the bytes: comment must not appear
@@ -459,6 +498,7 @@ private static String opt(String key) {
459498

460499
private String decompileSmali(String fileName, Map<String, String> pluginOptions) throws Exception {
461500
JadxArgs args = new JadxArgs();
501+
args.isShowInconsistentCode();
462502
args.getInputFiles().add(getSampleFile(fileName));
463503
args.setPluginOptions(pluginOptions);
464504
try (JadxDecompiler jadx = new JadxDecompiler(args)) {

‎src/test/resources/samples/b64/b64_cached_reflection.smali‎

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
# Tests Base64-obfuscated reflection strings in a real-world caching pattern.
2+
#
3+
# Method getPackages(int):
4+
# Conditional cached lookup structure with three Base64.decode args:
5+
#
6+
# First block (if cachedPm == null):
7+
# "YW5kcm9pZC5hcHAuQWN0aXZpdHlUaHJlYWQ=" -> "android.app.ActivityThread"
8+
# "Z2V0UGFja2FnZU1hbmFnZXI=" -> "getPackageManager"
9+
# Both are in a fully-inlined chain (no intervening invokes between each
10+
# <init> and its post-ctor user) -> grouped as indexed b64[0]/b64[1] comments.
11+
#
12+
# Second block (if cachedGetPkgs == null):
13+
# "Z2V0UGFja2FnZXNGb3JVaWQ=" -> "getPackagesForUid"
14+
# Has a blocking Object.getClass() invoke between <init> and getMethod,
15+
# but JADX inlines the constructor anyway (getClass is itself inlined first),
16+
# so the comment must land on the enclosing getMethod statement.
17+
18+
.class public final Lb64/B64CachedReflect;
19+
.super Ljava/lang/Object;
20+
21+
22+
# static fields
23+
.field private static flagA:I = -0x80000000
24+
25+
.field private static cachedPm:Ljava/lang/Object;
26+
27+
.field private static flagC:Ljava/lang/reflect/Method;
28+
29+
.field private static cachedGetPkgs:Ljava/lang/reflect/Method;
30+
31+
32+
.method public static getPackages(I)Landroid/content/pm/PackageInfo;
33+
.registers 8
34+
35+
.line 2
36+
const/4 v0, 0x0
37+
38+
:try_start_1
39+
sget-object v1, Lb64/B64CachedReflect;->cachedPm:Ljava/lang/Object;
40+
41+
const/4 v2, 0x0
42+
43+
if-nez v1, :cond_2e
44+
45+
new-instance v1, Ljava/lang/String;
46+
47+
const-string v3, "YW5kcm9pZC5hcHAuQWN0aXZpdHlUaHJlYWQ="
48+
49+
invoke-static {v3, v2}, Landroid/util/Base64;->decode(Ljava/lang/String;I)[B
50+
51+
move-result-object v3
52+
53+
invoke-direct {v1, v3}, Ljava/lang/String;-><init>([B)V
54+
55+
invoke-static {v1}, Ljava/lang/Class;->forName(Ljava/lang/String;)Ljava/lang/Class;
56+
57+
move-result-object v1
58+
59+
new-instance v3, Ljava/lang/String;
60+
61+
const-string v4, "Z2V0UGFja2FnZU1hbmFnZXI="
62+
63+
invoke-static {v4, v2}, Landroid/util/Base64;->decode(Ljava/lang/String;I)[B
64+
65+
move-result-object v4
66+
67+
invoke-direct {v3, v4}, Ljava/lang/String;-><init>([B)V
68+
69+
new-array v4, v2, [Ljava/lang/Class;
70+
71+
invoke-virtual {v1, v3, v4}, Ljava/lang/Class;->getMethod(Ljava/lang/String;[Ljava/lang/Class;)Ljava/lang/reflect/Method;
72+
73+
move-result-object v1
74+
75+
new-array v3, v2, [Ljava/lang/Object;
76+
77+
invoke-virtual {v1, v0, v3}, Ljava/lang/reflect/Method;->invoke(Ljava/lang/Object;[Ljava/lang/Object;)Ljava/lang/Object;
78+
79+
move-result-object v1
80+
81+
sput-object v1, Lb64/B64CachedReflect;->cachedPm:Ljava/lang/Object;
82+
83+
:cond_2e
84+
sget-object v1, Lb64/B64CachedReflect;->cachedGetPkgs:Ljava/lang/reflect/Method;
85+
86+
const/4 v3, 0x1
87+
88+
if-nez v1, :cond_50
89+
90+
new-instance v1, Ljava/lang/String;
91+
92+
const-string v4, "Z2V0UGFja2FnZXNGb3JVaWQ="
93+
94+
invoke-static {v4, v2}, Landroid/util/Base64;->decode(Ljava/lang/String;I)[B
95+
96+
move-result-object v4
97+
98+
invoke-direct {v1, v4}, Ljava/lang/String;-><init>([B)V
99+
100+
sget-object v4, Lb64/B64CachedReflect;->cachedPm:Ljava/lang/Object;
101+
102+
invoke-virtual {v4}, Ljava/lang/Object;->getClass()Ljava/lang/Class;
103+
104+
move-result-object v4
105+
106+
new-array v5, v3, [Ljava/lang/Class;
107+
108+
sget-object v6, Ljava/lang/Integer;->TYPE:Ljava/lang/Class;
109+
110+
aput-object v6, v5, v2
111+
112+
invoke-virtual {v4, v1, v5}, Ljava/lang/Class;->getMethod(Ljava/lang/String;[Ljava/lang/Class;)Ljava/lang/reflect/Method;
113+
114+
move-result-object v1
115+
116+
sput-object v1, Lb64/B64CachedReflect;->cachedGetPkgs:Ljava/lang/reflect/Method;
117+
118+
:cond_50
119+
sget-object v1, Lb64/B64CachedReflect;->cachedGetPkgs:Ljava/lang/reflect/Method;
120+
121+
sget-object v4, Lb64/B64CachedReflect;->cachedPm:Ljava/lang/Object;
122+
123+
new-array v5, v3, [Ljava/lang/Object;
124+
125+
invoke-static {p0}, Ljava/lang/Integer;->valueOf(I)Ljava/lang/Integer;
126+
127+
move-result-object p0
128+
129+
aput-object p0, v5, v2
130+
131+
invoke-virtual {v1, v4, v5}, Ljava/lang/reflect/Method;->invoke(Ljava/lang/Object;[Ljava/lang/Object;)Ljava/lang/Object;
132+
133+
move-result-object p0
134+
135+
check-cast p0, [Ljava/lang/String;
136+
137+
if-eqz p0, :cond_73
138+
139+
array-length v1, p0
140+
141+
if-ne v1, v3, :cond_73
142+
143+
aget-object p0, p0, v2
144+
145+
invoke-static {p0, v2}, Lb64/B64CachedReflect;->getPackageByName(Ljava/lang/String;I)Landroid/content/pm/PackageInfo;
146+
147+
move-result-object p0
148+
:try_end_6d
149+
.catchall {:try_start_1 .. :try_end_6d} :catchall_6f
150+
151+
move-object v0, p0
152+
153+
goto :goto_73
154+
155+
:catchall_6f
156+
move-exception p0
157+
158+
invoke-static {p0}, Lhelper/ErrUtil;->log(Ljava/lang/Throwable;)V
159+
160+
:cond_73
161+
:goto_73
162+
return-object v0
163+
.end method

0 commit comments

Comments
 (0)