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.

Sign up for GitHub

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

Jump to bottom

#32: Remove apktool dependency #33

Merged
merged 1 commit into from
Jan 10, 2019
Merged

#32: Remove apktool dependency #33

merged 1 commit into from
Jan 10, 2019

Conversation

victorrattis
Copy link
Collaborator

Remove apktool dependency and use baksmali to decode an APK file.

  • Remove apktoolkit dependency;
  • Use baksmali lib to decode the APK;
  • Add and config Ivy Apache to download the dependencies;
  • Implement smali decoder to decode an APK in smali code;

@alexzaitsev alexzaitsev self-assigned this Jan 4, 2019
@alexzaitsev
Copy link
Owner

Wow! Amazing work. Give me some time to play around with your changes.

@victorrattis
Copy link
Collaborator Author

Thanks, take your time...

I analyzed better my submitted code and there is an improvement to do:

In build.xml:
I included all the dependencies hardcoded:

<include name="${lib.dir}/baksmali-2.2.5.jar" />
<include name="${lib.dir}/dexlib2-2.2.5.jar" />
<include name="${lib.dir}/guava-18.0.jar" />
<include name="${lib.dir}/util-2.2.5.jar" />
<include name="${lib.dir}/jcommander-1.64.jar" />

And the above code can be improvement for the following one:
<include name="${lib.dir}/**/*.jar" />

I am going submit another commit with this fix.

alexzaitsev
alexzaitsev requested changes Jan 8, 2019
View reviewed changes
src/code/decode/ApkSmaliDecoder.java Outdated Show resolved Hide resolved
src/code/decode/ApkSmaliDecoder.java Outdated
}
}

private int getNumerOfAvailableProcessors() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo, should be getNumberOfAvailableProcessors

src/code/decode/ApkSmaliDecoder.java Outdated
void decode() throws IOException {
File apkFile = new File(this.apkFilePath);
if (!apkFile.exists()) {
throw new IOException("Apk file not found!");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo, should be "Apk file is not found!"

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this string should be constant (private static final...)

src/code/io/ArgumentReader.java Outdated Show resolved Hide resolved
@victorrattis
Copy link
Collaborator Author

Done, I sent a new commit with all the changes suggested by you. For this new commit, I squashed the changes in just one commit... I think it's better, but what do you think about in joining two commits into one commit only? or is it better to keep two or more commits for each change request in code review?

@alexzaitsev
Copy link
Owner

@victorrattis I think that squashing in most cases is the preferable way for PRs. Thanks for the quick changes! I'm going to approve and merge it to the master. How do you feel about this PR - have you tested it well? Should something in readme be changed as well?

@victorrattis
Copy link
Collaborator Author

Hi @alexzaitsev, I analyzed and tested better this patch... I stopped for a while to analyse each code checking if everything is okay.

My dev analysis:

I found two points that caught my attention:

1. Android API version: To decode the APK using baksmali jar is necessary to pass as parameter the API version. Using the value 28 as default because in my analysis it is the latest version. Theoretically and in my tests it decodes well the dex file of the last and previous versions without any problem. The baksmali uses this info to make the mapping of API that will be used to decode each dex code (for more information look this at link).

private static final int DEFAULT_ANDROID_VERSION = 28;

About this point, I have tested APK between the 15 and 28 versions and it works well but I am going to add a // TODO comment there explaining the next step to change the default version to the current version of the APK so it can be more dynamic.

2. Get the dex files inside the APK: I implemented a solution to filter and return all files that contains "classes" in the file name but I think it is better to change it to filter all the files with the extension ".dex" so it checks if contains "classes".

return ZipFileUtils.filterByContainsName(apkFilePath, "classes");

The current patch is working well but I am going to submit a new commit with these improvements.
I have tested in a few Android versions and projects. So then I can integrate it =)

@victorrattis
Copy link
Collaborator Author

victorrattis commented Jan 10, 2019 •
edited
Loading

Should something in readme be changed as well?

I think that so because in current README says about apktool, but do you think better do that in another issue? or in this pull request?

@victorrattis
Copy link
Collaborator Author

Ready, I submitted the last version for this Pull Request, I believe it is last. I tested it using APKs with or without multi-dex, few Android SDK versions and Android App Bundle and it is working well in my tests. I didn't test using an instant app. Now you can integrate it =)

@alexzaitsev
Copy link
Owner

alexzaitsev commented Jan 10, 2019 •
edited
Loading

Ready, I submitted the last version for this Pull Request, I believe it is last. I tested it using APKs with or without multi-dex, few Android SDK versions and Android App Bundle and it is working well in my tests. I didn't test using an instant app. Now you can integrate it =)

Thank you for your efforts! You've done a huge piece of work here.

@alexzaitsev
Copy link
Owner

Should something in readme be changed as well?

I think that so because in current README says about apktool, but do you think better do that in another issue? or in this pull request?

I think that it's ok to have readme changes here as well because they reflect code changes. Thanks!

@victorrattis
Remove apktoolkit dependency
acfca21 
 - Remove apktoolkit dependency;
 - Use baksmali lib to decode the apk;
 - Add and config Ivy Apache to download the depedencies;
 - Implement Smali decoder to decode an APK in smali code;

 alexzaitsev#32
@victorrattis
Copy link
Collaborator Author

Done, I submitted as changes in the README.md file, I think it is okay... I removed the following sections:

  • "Long way" - This section says to run apktool and so run the apk-dependency-graph jar and I think it is not more necessary;
  • "Troubleshooting" - I tested an instant app APK and in my tests, it is working well. Due to that, I removed this section.

@alexzaitsev alexzaitsev merged commit 2a2ef99 into alexzaitsev:master Jan 10, 2019
@alexzaitsev
Copy link
Owner

Hooray! Thank you @victorrattis ! Amazing work!

@victorrattis victorrattis deleted the 32-remove-apktoolkit branch January 11, 2019 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexzaitsev alexzaitsev alexzaitsev approved these changes

Assignees

@alexzaitsev alexzaitsev

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@victorrattis @alexzaitsev