Adds support for multidex APKs, plus some optimizations

rovellipaolo / NinjaDroid

Ninja Reverse Engineering on Android APK packages

GNU General Public License v3.0

268 stars 49 forks source link

Adds support for multidex APKs, plus some optimizations #8

Closed iantruslove closed 5 years ago

iantruslove commented 5 years ago

I had a crack at fixing the multidex issue. Fixes #7.

Apologies for the meandering PR, I did some refactoring to learn about the code before digging in to the multidex issue itself.

ae11dd7 through 862db8a, plus the other commits not explicitly mentioned below are some refactorings and minor fixes. The UTC time stamps is probably the most notable difference.
6a5d5c9 adds a bunch of pieces to build NinjaDroid into a docker image. I found this really helpful for testing - it's a stable environment for all the other pieces of APK tooling.
3ffaf4c fixes multidex. The changes aren't too complex, but I did make a minor adjustment to the Dex interface.
387bccf is a minor change that makes the run time about a minute faster for me.

Happy to make any changes to this, whatever you see fit.

rovellipaolo commented 5 years ago

Thanks a lot for this pull request! :) I really appreciate it and encourage you to continue working on NinjaDroid in future.

Thus, I truly hope that you don't take it badly but, next time, please split each "new feature" or "standalone improvement" in a separate pull request. For example: one for the performance improvements, one for adding docker support, one for adding multidex support, ... It's easier and safer to review and merge X small pull requests rather than a single one X times bigger.

Anyway, I will review all the changes and let you know. ;)

iantruslove commented 5 years ago

Thus, I truly hope that you don't take it badly but, next time, please split each "new feature" or "standalone improvement" in a separate pull request.

Yeah, I know better than that... :) If you want I can split it up, it's probably easier for me to do that and you review the three or so MRs than the big monolith

rovellipaolo commented 5 years ago

Nah.. don't worry. I am already on it. :)

iantruslove commented 5 years ago

:D nice!

I was just trying it out again on some more APKs but I think there's a problem retrieving resources from the zip archive. Digging in...

iantruslove commented 5 years ago

@rovellipaolo thanks for merging! Couple of things:

1) why did you revert the memoization commit? It significantly sped up processing. 2) there were a couple of changes I made that you put back. There were valid reasons for those changes, albeit unclear. I will open issues for them.

rovellipaolo commented 5 years ago

About memoization, check my comment above. The change was actually really nice, but was breaking the unit tests and I didn't want to block the rest of the pull request (i.e. multidex support, docker support, ...) for that. Feel free to send the change again in a separate pull request but, of course, with the tests fixed. ;)