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

Reflection Free Configuration and Proguard #146

Closed
philipbjorge opened this issue Aug 29, 2016 · 20 comments
Closed

Reflection Free Configuration and Proguard #146

philipbjorge opened this issue Aug 29, 2016 · 20 comments

Comments

@philipbjorge
Copy link

philipbjorge commented Aug 29, 2016

Is there any guidance on how to use the reflection free configuration with proguard and obfuscation?
Based on looking at the factories, it looks like I'll need to keep my class names at a minimum (possibly method names too).

-keepnames class com.myapp.**

Do you have any advice on how I can make this more specific?

@philipbjorge
Copy link
Author

Here's what I've got working on my project...

-keep class javax.inject.**

# Do not obfuscate annotation scoped classes
-keepnames @javax.inject.Singleton class *
# Add any custom defined @Scope (e.g. @Singleton) annotations here
# because proguard does not allow annotation inheritance rules

# Do not obfuscate classes with Injected Constructors
-keepclasseswithmembernames class * {
    @javax.inject.Inject <init>(...);
}

# Do not obfuscate classes with Injected Fields
-keepclasseswithmembernames class * {
    @javax.inject.Inject <fields>;
}

# Do not obfuscate classes with Injected Methods
-keepclasseswithmembernames class * {
    @javax.inject.Inject <methods>;
}

@dlemures
Copy link
Collaborator

dlemures commented Sep 7, 2016

Hi @philipbjorge ,

Thanks for the rules!

Do you need -keep class javax.inject.**?

We use them just at compile time, they don't need to be present at runtime. At groupon, we even use a gradle trick to use a version of thoses classes with a non-runtime retention policy, so they are definitely not part of our classes. (we do this to save space in the first dex.. Those annotations are available in TP for those who would like to use them too)

@stephanenicolas
Copy link
Owner

stephanenicolas commented Sep 7, 2016

As soon as @philipbjorge answers our question, we should add this to the wiki page about configurations:
https://github.com/stephanenicolas/toothpick/wiki/Configurations

@philipbjorge
Copy link
Author

philipbjorge commented Sep 7, 2016

@dlemures When I remove the -keep class javax.inject.**, I get the following error at runtime:

Unable to start activity ComponentInfo{com.xyz/com.xyz.activities.MainActivity}: e.c.e: No factory could be found for class com.xyz.a.a. Check that the class has either a @Inject annotated constructor or contains @Inject annotated members. If using Registries, check that they are properly setup with annotation processor arguments.
android.app.ActivityThread.performLaunchActivity

I've tried removing it and including both of the following and receive the same failures:

-keepnames class javax.inject.**
-keepattributes *Annotation*

Any ideas why or how to debug?

@philipbjorge
Copy link
Author

philipbjorge commented Sep 7, 2016

Looking at my stacktraces + mappings.txt makes me think something real funny is going on.
It looks like Toothpick is trying to get a factory for an interface that is never injected anywhere...
I'll keep digging and might be able to remove the javax.inject rule...

@philipbjorge
Copy link
Author

OK, so I had the following rule to keep the names of all the classes annotated with my custom scope annotations
-keepnames @com.xyz.support.di.scopes.* class *

Turns out that doesn't work and you need to list them all out manually

-keepnames @javax.inject.Singleton class *
-keepnames @com.xyz.support.di.scopes.ActivitySingleton class *
-keepnames @com.xyz.support.di.scopes.FragmentSingleton class *
-keepnames @com.xyz.support.di.scopes.ChildFragmentSingleton class *

Now I'm running into a new issue though. Will update shortly.

@philipbjorge
Copy link
Author

philipbjorge commented Sep 7, 2016

So the latest rules are...

# Do not obfuscate annotation scoped classes
-keepnames @javax.inject.Singleton class *
# Add any custom defined @Scope (e.g. @Singleton) annotations here
# because proguard does not allow annotation inheritance rules

# Do not obfuscate classes with Injected Constructors
-keepclasseswithmembernames class * {
    @javax.inject.Inject <init>(...);
}

# Do not obfuscate classes with Injected Fields
-keepclasseswithmembernames class * {
    @javax.inject.Inject <fields>;
}

# Do not obfuscate classes with Injected Methods
-keepclasseswithmembernames class * {
    @javax.inject.Inject <methods>;
}

The problem I'm running into now is the following...

public class Base {
  @Inject Foo foo;
}

public class Child extends Base {
}

We need both to be non-obfuscated and I'm not sure if there's a way to select for that with proguard.

It seems like the easiest way forward might be to have Toothpick generate a proguard rules file that keeps class names for classes that will be looked up in the registry? Alternatively, just recommend all class names within your project be non-obfuscated.

@stephanenicolas
Copy link
Owner

stephanenicolas commented Sep 20, 2016

Yes, probably the best here would be to recommend not to obfuscate any project class...
Obfuscation barely works with annotation processing.

@stephanenicolas stephanenicolas modified the milestone: RC10 Sep 21, 2016
@stephanenicolas
Copy link
Owner

solved by https://github.com/stephanenicolas/toothpick/wiki/Proguard-&-TP

@markchristopherng
Copy link

markchristopherng commented Mar 3, 2017

we recently migrated to toothpick from roboguice and these proguard rules worked on our project

-keepattributes Annotation

-keepnames class * {
@javax.inject.Inject (...);
@javax.inject.Inject ;
@javax.annotation.Nullable ;
}

-keepnames @javax.inject.Singleton class *
-dontwarn javax.inject.**

-keep class **$$Factory{ *; }
-keep class **$$MemberInjector{ *; }

@L7ColWinters
Copy link

wait, so the solution to this is not to obfuscate the code?! Most production applications require some form of obfuscation for a semblance of security. This seems like a major dealbreaker and i'm only saying that because I'm having trouble with the above proguard rules. It would have been nice to have a solution that included a recommended (Tested and true) way of using proguard with the library.

@stephanenicolas
Copy link
Owner

stephanenicolas commented Apr 3, 2017 via email

@terrakok
Copy link
Contributor

@stephanenicolas
We need setup ProGuard with
https://www.guardsquare.com/en/proguard/manual/usage#adaptclassstrings
For changing class names in FactoryRegistry

@stephanenicolas
Copy link
Owner

Thx for hint @terrakok. If anyone feels like adding a proguard configuration page to TP wiki, feel welcome to do so.

@pshmakov
Copy link
Collaborator

pshmakov commented Jan 3, 2018

@stephanenicolas
@terrakok
adaptclassstrings almost works (with registries).
The only problem is that switch statement doesn't work for equal strings that have undergone this proguards "adaptation". Don't ask me why.
So this code:
switch("com.smth.Smth") { case "com.smth.Smth": return true; } return false;
returns true before obfuscation and false after obfuscation with -adaptclassstrings.

Whatever the reason, the solution is simple: just generate if-else chain instead of switch. I checked that it works.

@stephanenicolas
Copy link
Owner

@pshmakov the problem is that it would be slower, that's the main reason why we have the switch.

Can you give an example of the code after it is obfuscated so we can fine tune this ?

@pshmakov
Copy link
Collaborator

pshmakov commented Jan 8, 2018

@stephanenicolas
Indeed, switch is faster, and the optimization that makes it faster is the one that causes problems with obfuscation.
I'm not sure what you mean by "example of the code after it is obfuscated". If I understand it correctly, obfuscation is applied to the bytecode, not the source code. Pretty much any Toothpick sample would exhibit the problem with obfuscation; I use this sample for experimenting: https://bitbucket.org/Con_Fuoco/toothpick-demo/overview

@pshmakov
Copy link
Collaborator

@stephanenicolas
I updated my patch, so that a HashMap is generated instead of switches or ifs. The lookup time should be roughly the same as before, but there is some extra memory usage, which I think shouldn't be critical: those factories are lightweight, they don't have any fields at all.
The change only affects the case when "support_obfuscation" annotation processing parameter was set to "true". Otherwise it will generate switches as before.

I'd like to point out that this issue seems to be a "blocker" for using ToothPick in a large portion of commercial projects, because using obfuscation there is absolutely non-negotiable. I work on one of such projects, and would really like to give Toothpick a shot, because it's awesome. But this issue needs to be fixed first. Otherwise we'll have to go with Dagger 2, unfortunately (which doesn't have problems with obfuscation as far as I know).

@terrakok
Copy link
Contributor

@stephanenicolas
What do you think about PR?

@stephanenicolas
Copy link
Owner

This is adressed in TP 1.1.3, released today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants