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

Streamline decoding AndroidManifest.xml #3171

Merged
merged 2 commits into from Jul 23, 2023

Conversation

sv99
Copy link
Contributor

@sv99 sv99 commented Jul 19, 2023

Streamline decoding AndroidManifest.xml.

Decoding AndroidManifest.xml without using ResStreamDecoderContainer.

Both methods: decodeManifest decodeManifestWithResources in fact initializes decoder in the same way.

@iBotPeaches
Copy link
Owner

Failure looks odd. Looks related to some changes I did yesterday here: 3fdc06a, but they passed?

@iBotPeaches
Copy link
Owner

Sorry appears all builds, even old ones are busted at the moment. I've opened #3174 and will have to fix that before any new PRs.

@sv99
Copy link
Contributor Author

sv99 commented Jul 19, 2023

Sorry appears all builds, even old ones are busted at the moment. I've opened #3174 and will have to fix that before any new PRs.

No problem

Copy link
Owner

@iBotPeaches iBotPeaches left a comment

Choose a reason for hiding this comment

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

Both methods: decodeManifest decodeManifestWithResources in fact initializes decoder in the same way.

I don't think this is true. decodeManifest calls getManifestFileDecoder(false) and decodeManifestWithResources calls getManifestFileDecoder(true)

Gotta find the ticket that led to the introduction of this, so people could force decode the manifest without parsing the resource table.

@sv99
Copy link
Contributor Author

sv99 commented Jul 20, 2023

At first glance its true but in reality initialization the same.

decodeManifest(...)

    // Duo<ResFileDecoder, AXmlResourceParser> duo = getManifestFileDecoder(false);
    ResStreamDecoderContainer decoders = new ResStreamDecoderContainer();
    AXmlResourceParser axmlParser = new AndroidManifestResourceParser();
    decoders.setDecoder("xml", new XmlPullStreamDecoder(axmlParser, getResXmlSerializer()));
    Duo<ResFileDecoder, AXmlResourceParser> duo = new Duo<>(new ResFileDecoder(decoders), axmlParser);

    ResFileDecoder fileDecoder = duo.m1;
    // Set ResAttrDecoder
    duo.m2.setAttrDecoder(new ResAttrDecoder());
    ResAttrDecoder attrDecoder = duo.m2.getAttrDecoder();
    attrDecoder.setResTable(resTable);

    ...

// Set ResAttrDecoder - in the function decodeManifest

decodeManifestWithResources(...)

    // Duo<ResFileDecoder, AXmlResourceParser> duo = getManifestFileDecoder(true);
    ResStreamDecoderContainer decoders = new ResStreamDecoderContainer();
    AXmlResourceParser axmlParser = new AndroidManifestResourceParser();
    // Set ResAttrDecoder
    axmlParser.setAttrDecoder(new ResAttrDecoder());
    decoders.setDecoder("xml", new XmlPullStreamDecoder(axmlParser, getResXmlSerializer()));
    Duo<ResFileDecoder, AXmlResourceParser> duo =  new Duo<>(new ResFileDecoder(decoders), axmlParser);

    ResFileDecoder fileDecoder = duo.m1;
    ResAttrDecoder attrDecoder = duo.m2.getAttrDecoder();
    attrDecoder.setResTable(resTable);

    ...

// Set ResAttrDecoder - in the function getManifestFileDecoder(true)

All tests worked.

If resources dont parsed we have empty resource table initialized in the function getResourceTable().

@sv99 sv99 force-pushed the streamline_decode_manifest branch from 049c49e to 0a8a463 Compare July 20, 2023 19:46
@sv99
Copy link
Contributor Author

sv99 commented Jul 20, 2023

PR rebased

@iBotPeaches
Copy link
Owner

All tests worked.

I've been learning the past few days that the test suite wasn't as robust as I hoped :)

Thanks for the update, I'll take another look.

@sv99 sv99 force-pushed the streamline_decode_manifest branch 2 times, most recently from 82155c8 to 55e2977 Compare July 21, 2023 15:35
@sv99
Copy link
Contributor Author

sv99 commented Jul 21, 2023

Rebased and removed local variable.

@iBotPeaches
Copy link
Owner

Thanks for update. I understand now that the main difference is having a blank/empty resource table if we force it. Just going to find an old ticket that added this feature and hope the sample app still exists. Want to be sure that isn't regressed.

@sv99
Copy link
Contributor Author

sv99 commented Jul 21, 2023

This is independent from decoding process.

I think need first decode resources - they are not dependent from manifest.
And only then decode manifest. Now resources implicitly decoded in the getResTable() .

Copy link
Owner

@iBotPeaches iBotPeaches left a comment

Choose a reason for hiding this comment

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

Looks good, but holding on merge till after I cut 2.8.1. Which should be Sat/Sun.

Copy link
Owner

@iBotPeaches iBotPeaches left a comment

Choose a reason for hiding this comment

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

I ran some tests today and I noticed a regression that we always print

➜  Optimized apktool d -r --force-manifest app-debug.apk -f
I: Using Apktool 2.8.1 on app-debug.apk
I: Loading resource table...
I: Decoding AndroidManifest.xml with only framework resources...
➜  Optimized apktool d app-debug.apk -f    
I: Using Apktool 2.8.1 on app-debug.apk
I: Loading resource table...
I: Decoding AndroidManifest.xml with only framework resources...
➜  Optimized apktool d -r app-debug.apk -f
I: Using Apktool 2.8.1 on app-debug.apk
I: Copying raw manifest...
I: Copying raw resources...

As you can see it doesn't matter what mode we do. It still prints "decoding AndroidManifest.xml with only framework resources..."

@sv99 sv99 force-pushed the streamline_decode_manifest branch from ef9dabe to 6d21ecc Compare July 23, 2023 08:13
@sv99
Copy link
Contributor Author

sv99 commented Jul 23, 2023

Different logger message added

@sv99 sv99 force-pushed the streamline_decode_manifest branch from 6d21ecc to 3549650 Compare July 23, 2023 08:23
@sv99 sv99 force-pushed the streamline_decode_manifest branch from 3549650 to 7564bc4 Compare July 23, 2023 19:09
@sv99
Copy link
Contributor Author

sv99 commented Jul 23, 2023

rebased

Copy link
Owner

@iBotPeaches iBotPeaches left a comment

Choose a reason for hiding this comment

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

Going to fix a typo post merge, but otherwise looks good now.

@iBotPeaches iBotPeaches changed the title Sreamline decoding AndroidManifest.xml Streamline decoding AndroidManifest.xml Jul 23, 2023
@iBotPeaches iBotPeaches merged commit 3ba9838 into iBotPeaches:master Jul 23, 2023
31 checks passed
iBotPeaches added a commit that referenced this pull request Jul 23, 2023
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.

None yet

2 participants