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

refactor: ApkDecoder & ApkBuilder overhaul #3699

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

IgorEisberg
Copy link
Contributor

@IgorEisberg IgorEisberg commented Sep 20, 2024

A major rewrite of ApkDecoder and ApkBuilder classes to make them manageable. Removed many instances of redundancy and improved syntax and indentation.

Modifying the stock Apktool source to our needs have become too difficult, so I'm pushing the general (not specific to our needs) changes upstream.

I'd change a lot more, but I wanted to make sure all tests pass as expected, despite some of them being weird, outdated or unnecessary.

This also fixes certain files in META-INF being lost during recompile when the -c/--copy-original option isn't used.
Also, fixes the expected behavior for -j/--jobs option, e.g. for "-j 1" it's expected to do everything on the main thread, so for j > 1 it's actually j-1 for smali/baksmali, and the last one is for the main thread (for aapt).
Also, when smali fails (e.g. due to method limit exceeded), it'll now correctly show which of the smali dirs failed to smali.

This has been tweaked and tested for several days and I vouch for its stability.

A major rewrite of ApkDecoder and ApkBuilder classes to make them managable.
Removed many instances of redundancy and improved syntaxed and indentation.

Modifying the stock Apktool source to our needs have become too difficult,
so I'm pushing the general (not specific to our needs) changes upstream.

I'd change a lot more, but I wanted to make sure all tests pass as expected,
despite some of them being wierd, outdated or unnecessary.

This also fixes certain files in META-INF being lost during recompile
when the -c/--copy-original option isn't used.

This has been tweaked and tested for several days and I vouch for its stablity.
@iBotPeaches
Copy link
Owner

Left me some reviewing to do.

@iBotPeaches
Copy link
Owner

Couple skim comments

  • All the equals from equalsIgnoreCase - a few of those cases are from weird obfuscated apks that had like cOloR or something.
  • The e to ex is just preference?

@IgorEisberg
Copy link
Contributor Author

IgorEisberg commented Sep 21, 2024

Couple skim comments

  • All the equals from equalsIgnoreCase - a few of those cases are from weird obfuscated apks that had like cOloR or something.
  • The e to ex is just preference?
  • Oh I see, I never stumbled upon such APKs. Want me to revert that? Just hard to believe that those APKs are that common or even accepted by Android framework...
  • Just for consistency I searched the whole source and picked the one that appears more often. "ex" appeared a lot more often.

@IgorEisberg
Copy link
Contributor Author

By the way, this:

        boolean cmdFound = false;
        for (String opt : commandLine.getArgs()) {
            if (opt.equalsIgnoreCase("d") || opt.equalsIgnoreCase("decode")) {
                cmdDecode(commandLine, config);
                cmdFound = true;
            } else if (opt.equalsIgnoreCase("b") || opt.equalsIgnoreCase("build")) {
                cmdBuild(commandLine, config);
                cmdFound = true;
            } else if (opt.equalsIgnoreCase("if") || opt.equalsIgnoreCase("install-framework")) {
                cmdInstallFramework(commandLine, config);
                cmdFound = true;
            } else if (opt.equalsIgnoreCase("empty-framework-dir")) {
                cmdEmptyFrameworkDirectory(commandLine, config);
                cmdFound = true;
            } else if (opt.equalsIgnoreCase("list-frameworks")) {
                cmdListFrameworks(commandLine, config);
                cmdFound = true;
            } else if (opt.equalsIgnoreCase("publicize-resources")) {
                cmdPublicizeResources(commandLine, config);
                cmdFound = true;
            }
        }

There's no reason to use "equalsIgnoreCase" here either because the options are case-sensitive anyway.

@IgorEisberg
Copy link
Contributor Author

Here's my take - let's keep the "equals" checks and if those broken APKs appear we can revert what's relevant.
It's just that as I'm looking on where those "equalsIgnoreCase" were used - it's mainly arbitrary. Like attr_name.equalsIgnoreCase("versionCode"), there's no way it's both "vErSiOnCoDe" and a valid APK at the same time, it's better to be strict as much as possible.

@IgorEisberg
Copy link
Contributor Author

IgorEisberg commented Sep 21, 2024

I have a question for you too: Do we really need libs (not lib), kotlin, META-INF/services to be kept in main APK folder? You wrote:

                    // If the original APK contains the folder META-INF/services folder
                    // that is used for service locators (like coroutines on android),
                    // copy it to the destination folder, so it does not get dropped.

However, with my changes, everything in META-INF (except for *.(RSA|SF|MF) that are kept in original) as well as those other dirs will not be lost in the rebuilt APK anyway, so not sure what the purpose of that is, since AAPT doesn't do anything with them.

@iBotPeaches
Copy link
Owner

I have a question for you too: Do we really need libs (not lib), kotlin, META-INF/services to be kept in main APK folder?

In my opinion there is a clear distinction in Apktool of unknown files (we know nothing) and known Android files of the ones we process the ones we know are supposed to be there and passed along (libs, lib, kotlin, meta-inf, etc)

@IgorEisberg
Copy link
Contributor Author

I have a question for you too: Do we really need libs (not lib), kotlin, META-INF/services to be kept in main APK folder?

In my opinion there is a clear distinction in Apktool of unknown files (we know nothing) and known Android files of the ones we process the ones we know are supposed to be there and passed along (libs, lib, kotlin, meta-inf, etc)

Hmm well then there are more folders that match the description, like kotlinx, the whole META-INF (rather than just META-INF/services, minus *.(RSA|SF|MF)), also google (usually for protobuf files), okhttp3, and many more.
They really serve no purpose being in the main APK dir, but up to you.

@iBotPeaches
Copy link
Owner

Hmm well then there are more folders that match the description, like kotlinx, the whole META-INF (rather than just META-INF/services, minus *.(RSA|SF|MF)), also google (usually for protobuf files), okhttp3, and many more.

Perhaps what you are leading to is the idea of splitting unknown and files we track (originals, etc) is moot. I could see that aspect as tracking those individually makes no sense when they are both just packed into the application. The split stems from some files having to be tacked onto the aapt/aapt2 process while others are just "zipped" in post build.

@IgorEisberg
Copy link
Contributor Author

IgorEisberg commented Sep 21, 2024

Hmm well then there are more folders that match the description, like kotlinx, the whole META-INF (rather than just META-INF/services, minus *.(RSA|SF|MF)), also google (usually for protobuf files), okhttp3, and many more.

Perhaps what you are leading to is the idea of splitting unknown and files we track (originals, etc) is moot. I could see that aspect as tracking those individually makes no sense when they are both just packed into the application. The split stems from some files having to be tacked onto the aapt/aapt2 process while others are just "zipped" in post build.

Yes, there is no limit to how many "known" but non-AAPT related folders or files can appear in the future but being "known" means nothing in terms of editing the app, e.g. DebugProbesKt.bin also goes to unknown but come on, every app built with Kotlin has it. So "unknown" is a misleading name, it's as simple as untracked or misc or others - files that are usually left as-is and we want them repacked - and we're separating them for the sole purpose of keeping the working dir easy on the eyes to work with.

EDIT: With that said, using the name unknown isn't all that bad, because said files are indeed unknown to both smali and AAPT, even if they are known to the developer.

1) We take advantage of the fact that doNotCompress already tracks uncompressed files,
   including those separated into "unknown".
   With this change the "unknownFiles" is simply ignored, so it's backward-compatible
   with existing decoded APK dirs.
   Tweaked a few tests to match the removal of "unknownFiles".

2) Passing doNotCompress to AAPT is redundant, Apktool extracts the temp APK packed by
   AAPT to build/apk and then repackages it anyway, so it serves no purpose.
@IgorEisberg
Copy link
Contributor Author

For now, I didn't touch RAW_DIRNAMES, as it'll require removing some kotlin/coroutines tests (which really mean nothing to the one editing the APK).
But ideally this:

public final static String[] RAW_DIRNAMES = new String[] { "assets", "lib", "libs", "kotlin", "META-INF/services" };

Should be as simple as:

public final static String[] RAW_DIRNAMES = new String[] { "assets", "lib" };

And it works great. The rest is later repacked to the final APK. That's what I'm using in our custom Apktool, it simply avoids the whole "what is known?" question.

@iBotPeaches
Copy link
Owner

Only on my phone right now, but a bit worried with all the compress changes. We have years of bugs about files losing intended compression, older apks compressing arsc files, newer apks not doing so for random access.

Those changes will take some reviewing.

@IgorEisberg
Copy link
Contributor Author

IgorEisberg commented Sep 21, 2024

Only on my phone right now, but a bit worried with all the compress changes. We have years of bugs about files losing intended compression, older apks compressing arsc files, newer apks not doing so for random access.

Those changes will take some reviewing.

There is no compression change for resources.arsc. It's listed in doNotCompress if it's stored as before.
I only tweaked the PNG test because those are never compressed in original APKs (AAPT rules).
Also *.dex and *.so are either all compressed or all stored, so the dex|so addition is alright. Also useful when adding a new classes#.dex file is necessary because the existing ones already hit the dex method limit so can't add any code (I had cases like that), so you'd like the new dex file to inherit the same compression as the existing ones.

@iBotPeaches
Copy link
Owner

Screenshot 2024-09-23 at 9 32 46 AM

I blocked off some time on weekend for this. I'll either merge it or have some comments.

* Regression: minSdkVersion inferred from baksmali was not stored properly.

* The arsc extension can be generalized for simplicity as seen in AOSP source.
https://cs.android.com/android/platform/superproject/main/+/main:external/deqp/scripts/android/build_apk.py;l=644?q=apk%20pack&ss=android%2Fplatform%2Fsuperproject%2Fmain:external%2F
  Note:
    NO_COMPRESS_EXT_PATTERN only collapses paths to a common extension.
    It does NOT force these extensions to be always uncompressed.
    doNotCompress is the one determining files/extensions that should be uncompressed.
  (no funcionality was changed)

* resourcesAreCompressed in apkInfo is redundant. It was only used in invokeAapt,
  but not ApkBuilder. Its value is also never set by Apktool, only read.
  Like with doNotCompress, passing any kind of compression rules to AAPT is pointless,
  since we don't use the temp APK packed by AAPT directly - it's extracted and repacked
  by ApkBuilder, where doNotCompress already determines whether resources.arsc should
  or should not be compressed in the final APK.
  (no funcionality was changed)
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

Successfully merging this pull request may close these issues.

2 participants