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

redex removes required classes. #30

Closed
VenomVendor opened this issue Apr 13, 2016 · 15 comments
Closed

redex removes required classes. #30

VenomVendor opened this issue Apr 13, 2016 · 15 comments
Assignees

Comments

@VenomVendor
Copy link
Contributor

VenomVendor commented Apr 13, 2016

App crashes on launch.

java.lang.IllegalAccessError: Illegal class access: 'okhttp3.internal.http.RequestLine' attempting to access 'io.fabric.sdk.android.services.common.ExecutorUtils$1' (declaration of 'okhttp3.internal.http.RequestLine' appears in /data/app/com.packageName-1/base.apk)
    at okhttp3.internal.http.RequestLine.getNamedThreadFactory(streamModelLoader:68)
    at com.crashlytics.android.core.CrashlyticsCore.<init>(transition:42)
    at com.crashlytics.android.core.CrashlyticsCore.<init>(transition:200)
    at com.crashlytics.android.Crashlytics.<init>(treeUri:29)

Decompiling redexed apk does not have io.fabric.sdk.android.services.common.ExecutorUtils.class where as in default one has ExecutorUtils.class

Minimal project to try.

Redex.zip
decompiled-jars.zip

run ./gradlew clean build

Find 2 apks in root dir.

  • app-debug.apk works
  • app-redex-signed.apk fails
@rekire
Copy link

rekire commented Apr 13, 2016

Try using the -m and -P arguments to apply your proguard settings. See also in your terminal redex -h

@ghost
Copy link

ghost commented Apr 13, 2016

I guess Fabric is using reflection there... How can we specify rules to exclude some files from guarding?

@VenomVendor
Copy link
Contributor Author

I am not using any proguard settings.

@ghost
Copy link

ghost commented Apr 13, 2016

We are talking about redex here, not about ProGuard. Official facebook post says that we can specify rules in *.json file to ReDex. But i can't find any docs about that. That's the question

@ghost
Copy link

ghost commented Apr 13, 2016

basically i'm observing same crash, as @VenomVendor
here is full crash log

@stephanenicolas
Copy link

using proguard config (-P) and seeds.txt (-m) did not work out of the box :
In proguard file, a few lines were causing redex to file :

  • this block had to be removed
-keepclassmembers class * implements java.io.Serializable {
    static final long serialVersionUID;
    private static final java.io.ObjectStreamField[] serialPersistentFields;
    !static !transient <fields>;
    !private <fields>;
    !private <methods>;
    private void writeObject(java.io.ObjectOutputStream);
    private void readObject(java.io.ObjectInputStream);
    java.lang.Object writeReplace();
    java.lang.Object readResolve();
}
  • this line had to be commented :#-ignorewarnings
  • this line had to be commented : #-dontskipnonpubliclibraryclassmembers
  • this line had to be commented #-keepclassmembers,includedescriptorclasses class * { native <methods>; }

And then, redex failed to compile our app, proguard fails during build if I restart it with the commented lines...

@bertmaher
Copy link
Contributor

Thanks for the detailed repro steps, I'll take a look in the morning.

@bertmaher bertmaher self-assigned this Apr 15, 2016
@ghost
Copy link

ghost commented Apr 15, 2016

I apologize. I'm in the process of deleting my github account. I have sent an issue ticket to the github help desk. I appreciate everyone who worked on this. Have a great day tomorrow. ;~)

@valintepes
Copy link
Contributor

valintepes commented Apr 16, 2016

I also have the same sort of problem. (log here)
This is a nearly empty project that just calls Fabric.with(this, new Crashlytics()); onCreate of its only activity. Without that call, the redex'd apk works, which makes sense.
The only thing I have to add is, I tried what @VenomVendor did in his original post and looked at the redex'd dex file. It gave me a little false hope, since I DID find ExecutorUtils$1 in my classes.dex file (using jadx. UrlUtils was also present. Sorry, you'll have to trust me as I don't know how to link an image here), so I tried it on a device and still, got this same crash.

EDIT:
The original Fabric io.fabric.sdk.android.services.common.ExecutorUtils is broken into io.fabric.sdk.android.services.common.ExecutorUtils$1 and io.fabric.sdk.android.services.common.ExecutorUtils$2 in the redex'd file. I'm giving away how clueless I am but I'm thinking this matters. I've heard that a common cause of illegal class access errors are changes to the called class that is not reflected in the final build/calling class is not aware. Also, this is the only file that seems to be so drastically changed. (unlike, say, io.fabric.sdk.android.services.common.CommonUtils)

EDIT2:
NVM I am an idiot. @VenomVendor's first post is correct. I simply did not understand, and jadx is a confusing tool. With dexmanager, I can see that the redex'd file has all the ExecutorUtils$... but not ExecutorUtils.

@valintepes
Copy link
Contributor

So, I used dex manager to put the missing ExecutorUtils file into the redex'd apk, resigned it, (debug, I haven't tried release) ... and it loads! huzzah! This is a dummy app, so not much of a test, but a work around is a work around. I will try it with my actual project some time soon. Sleep now.

@valintepes
Copy link
Contributor

Ok, I still don't really understand what's going on, but I have a more realistic work around than messing with packages manually, and some observations.

First, seems most of the errors reported here are accessing ExecutorUtils$1, though its ExecutorUtils that's missing. Also, the line numbers in the stack trace doesn't correspond to anything meaningful. one of mine was an import statement, and another was a null check for a parameter passed to a method. That one, (line 68) is the same line number regardless of what was the calling class. It IS always getNamedThreadFactory though. Some kind of naming problem/collision? I will also add, originally when I had this problem, I included a library I wasn't even using. And ITS classes was what was illegally accessing ExecutorUtil$1. Trace output shows all methods of ExecutorUtils were relocated or removed, so it makes sense it would be deleted, but no clue why it is still causing a problem. Anyhoo, onto work around, sort of.

I played with the --config option one pass at a time until ExecutorUtils was deleted. This occurred during StaticReloPass.

I tried various config jsons to see if I can get "keep_packages" flag set for io.fabric.sdk.android.services.common.ExecutorUtils so it won't be messed with, but couldn't figure it out. (the reference to pass options being documents next to each pass in config.md doesn't really tell me anything. is it yet to come?)

I tried setting a --keep file with a single line that's the class name, and a -P proguard file again with one line, -keep classing the troublesome class at hand. I DID get this in trace output:

Loading ProGuard configuration from ./config/proguard.conf
Reading seed classes from /Users/arcaditepes/keepme
Read 1 seed classes in 0.0 seconds
adding pattern Lio/fabric/sdk/android/services/common/ExecutorUtils 
matched cls Lio/fabric/sdk/android/services/common/ExecutorUtils$1; against pattern Lio/fabric/sdk/android/services/common/ExecutorUtils 
matched cls Lio/fabric/sdk/android/services/common/ExecutorUtils; against pattern Lio/fabric/sdk/android/services/common/ExecutorUtils 
matched cls Lio/fabric/sdk/android/services/common/ExecutorUtils$1$1; against pattern Lio/fabric/sdk/android/services/common/ExecutorUtils 
matched cls Lio/fabric/sdk/android/services/common/ExecutorUtils$2; against pattern Lio/fabric/sdk/android/services/common/ExecutorUtils 
matched on 4 classes with CLASS KEEP proguard rules 

but it still deleted ExectuorUtils.smali in the final dex.

So, looking at StaticRelo.cpp, deletable classes are determined during build_mutations. But, unless I have it wrong, that method only determines not to delete if a) the class is the relocation target or b) there is no relocation target (or rather the method collides with methods in the relocation target, same diff). nowhere else is the can_delete_class flag negated. This means, short of collision, all candidate classes are deletable.

build_candidates then, uses a whole bunch of matching things to determine deletability. However, it does not mention is_seed. So, nothing I'm --keeping matters at this point. So, I added

     && m::can_delete<DexClass>()
+    && !m::is_seed<DexClass>()

to StaticRelo.cpp::build_candidates, and

+/** Match which checks is_seed helper for DexMembers */
+template<typename T>
+match_t<T, std::tuple<> > is_seed() {
+  return {
+    [](const T* t) {
+      return is_seed(t);
+    }
+  };
+}

to Match.h's namespace m. rebuilt and ran with the same -P and --keep, and now I have a working apk post redex and signing (default configs)!

Now, I may have misinterpreted a bunch of stuff and ended up doing all this for nothing, but I don't see how --keep would matter (maybe later passes, but not here) if there's no check against it here. And a quick search in the code shows is_seed only being called in ReachableClasses::reportReachableClasses, which not only simply outputs things to file, its also never called anywhere.

All this and its still not a fix on the original problem, that ExecutorUtils is deleted. Anyways, sorry for the wall of text, I didn't want to forget anything in case its either useful or a huge misconception on my part I need to fix.

@bertmaher
Copy link
Contributor

Thanks for digging into this, @ArcadiTepes. You're totally right, the keep_* options aren't being respected by StaticRelo. Unfortunately the config is a bit of a disaster right now -- we have multiple parallel sets of options controlling what isn't removed. This isn't really an acceptable situation so I'm working on cleaning it up as priority #1.

Anyways, the keep_packages option is used by the do_not_strip family of functions in the code, so that's another alternative to the seed option. Seeds and proguard-config are a partially implemented feature right now (as you've found out, we ingest them but don't actually use them yet). I need to update the help to indicate that these options are developer only.

@huyongli
Copy link

I happen the same problem, but I don't solve it.
Do you solve it ? @ArcadiTepes @VenomVendor

@valintepes
Copy link
Contributor

@huyongli
No. Assuming the issues is truly similiar, that your ReDex'd apk is missing files in the .dex file(s), there's a workaround. It requires minor changes to redex code. If you take a look at your exception output, you can determine what class is the problem. Then, create a file with that class's name as the content, like so in my case:

keepme.txt contents:

io.fabric.sdk.android.services.common.ExecutorUtils

Take a look at my post above or #107. Make those same changes to your redex code, then rebuilt.

Now run your redex command with the --keep option and pass that file in.

Try the new apk. Hopefully that'll do it for you.

@bertmaher
Copy link
Contributor

The --keep option is one workaround (and StaticRelo should support it), but there's another fundamental issue with StaticRelo in that it doesn't fix up visibility when it moves methods. I've patched this bug and @VenomVendor's test APK works with my patch. I'll land the fix as soon as it gets reviewed.

@ghost ghost closed this as completed in 3383c9a Apr 20, 2016
ghost pushed a commit that referenced this issue Apr 22, 2016
Summary:ReDex does not respect `--keep`/`--seed` options.  It processes the input but does not check them in `StaticReloPass`.  This adds the check so problem classes such as in #30 can be worked around.
Closes #107

Reviewed By: dariorussi

Differential Revision: D3198539

Pulled By: bertmaher

fb-gh-sync-id: 3abee295742d42e2ca207f2f762cc081970e0415
fbshipit-source-id: 3abee295742d42e2ca207f2f762cc081970e0415
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants