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

fix: Stop patch when signing fails #1553

Merged
merged 1 commit into from Feb 1, 2024

Conversation

kitadai31
Copy link
Contributor

@kitadai31 kitadai31 commented Dec 5, 2023

fix #1253

Currently, even if an error occurred while signing apk, the patching process appears as if it is completed.
Therefore, the patched APK (out.apk) was not actually output, and the [Export APK] button causes a crash.

09-09 13:33:45.253 10011 10011 E AndroidRuntime: java.io.FileNotFoundException: /data/user/0/app.revanced.manager.flutter/cache/patcher/tmp-AXYSFX/out.apk: open failed: ENOENT (No such file or directory)

This issue (#1253) is caused by the double nested try block.
Any exceptions that occur while signing apk will be ignored by the try block.
Only a stack trace is output to logcat, but user will have never seen it.

--------- beginning of main
09-09 13:33:41.059 10011 12502 W System.err: java.security.UnrecoverableKeyException: no match
09-09 13:33:41.059 10011 12502 W System.err: 	at com.android.org.bouncycastle.jcajce.provider.keystore.bc.BcKeyStoreSpi$StoreEntry.getObject(BcKeyStoreSpi.java:325)
09-09 13:33:41.059 10011 12502 W System.err: 	at com.android.org.bouncycastle.jcajce.provider.keystore.bc.BcKeyStoreSpi.engineGetKey(BcKeyStoreSpi.java:620)
09-09 13:33:41.059 10011 12502 W System.err: 	at java.security.KeyStore.getKey(KeyStore.java:1062)
09-09 13:33:41.059 10011 12502 W System.err: 	at app.revanced.manager.flutter.utils.signing.Signer.signApk(Signer.kt:63)
09-09 13:33:41.059 10011 12502 W System.err: 	at app.revanced.manager.flutter.MainActivity.runPatcher$lambda$13(MainActivity.kt:279)
09-09 13:33:41.059 10011 12502 W System.err: 	at app.revanced.manager.flutter.MainActivity.$r8$lambda$6dokpIuPm1kqn7qLA9JtiVzSCi0(Unknown Source:0)
09-09 13:33:41.059 10011 12502 W System.err: 	at app.revanced.manager.flutter.MainActivity$$ExternalSyntheticLambda3.run(Unknown Source:24)
09-09 13:33:41.059 10011 12502 W System.err: 	at java.lang.Thread.run(Thread.java:929)

(quoted from #1253)

This PR removes this double try block to abort patching process properly.

Example

When user entered an incorrect keystore password, UnrecoverableKeyException occurs.
This PR prevents this exception from being ignored.

Screenshot

Screenshot_20231205-164905

print("Error signing APK: ${e.message}")
e.printStackTrace()
}
Signer("ReVanced", keystorePassword).signApk(patchedFile, outFile, keyStoreFile)
Copy link
Member

Choose a reason for hiding this comment

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

Why not instead simply return after the stack trace is printed? Also when would you expect an error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not instead simply return after the stack trace is printed?

But I think, if an exception is occurred, the exception should be handled at this catch block:

} catch (ex: Throwable) {
if (!cancel) {
val stack = ex.stackTraceToString()
updateProgress(
-100.0,
"Failed",
"An error occurred:\n$stack"
)
}
}

Looking at other parts of the method, I don't see any exception handling code.

Also when would you expect an error here?

If user entered an incorrect password

Screenshot

Screenshot_20231205-164905

If user imported a keystore generated by Android Studio

Screenshot

Screenshot_20231205-182526

(These screenshots are from this PR code)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently the stack trace is printed using print(), but this only outputs to stdout and does not appear on the screen.
So user can't see the error.
This problem can be seen in the screen recording at #1253.

@Ushie Ushie requested a review from oSumAtrIX December 10, 2023 20:25
@oSumAtrIX oSumAtrIX requested review from Axelen123 and removed request for oSumAtrIX December 23, 2023 21:33
@kitadai31
Copy link
Contributor Author

kitadai31 commented Dec 27, 2023

I made the PR description more specific.
Please refer to it whenever you review.

@oSumAtrIX oSumAtrIX changed the title fix: Exceptions while signing apk are ignored fix: Stop patch when signing fails Feb 1, 2024
@oSumAtrIX oSumAtrIX merged commit 5b2c551 into ReVanced:dev Feb 1, 2024
1 check passed
github-actions bot pushed a commit that referenced this pull request Feb 2, 2024
## Bug Fixes
* Stop patch when signing fails ([#1553](#1553)) ([kitadai31](5b2c551))
github-actions bot pushed a commit that referenced this pull request Mar 5, 2024
# [1.19.0](v1.18.0...v1.19.0) (2024-03-05)

### Bug Fixes

* added a trailing comma ([975180b](975180b))
* adjust padding ([3559477](3559477))
* Allow mounting without Magisk ([3f96608](3f96608))
* Bump dependencies to support BCS keystore ([6ec6546](6ec6546))
* Do not delete files from post-fs-data.d ([70a1086](70a1086))
* Fix patched APKs exports after installation ([1200360](1200360))
* fix redundant buttons on dialog ([079c0de](079c0de))
* Incorrect strings and logics ([#1619](#1619)) ([4f22e88](4f22e88))
* **Keystore Password:** textfield title display ([8e52abd](8e52abd))
* Mount script causes build to fail ([#1613](#1613)) ([f3c78c2](f3c78c2))
* **Patch Option:** Set text colour on dropdown menu ([acb1e24](acb1e24))
* **PopScope:** User able to exit patch screen when the installer is still running ([#1663](#1663)) ([eb6d3cd](eb6d3cd))
* **Release CI:** truncate the "v" from version ([8595099](8595099))
* Show version label correctly ([c72d10a](c72d10a))
* Specify that dark theme is dark ([#1699](#1699)) ([d4b15ae](d4b15ae))
* Stop patch when signing fails ([#1553](#1553)) ([5b2c551](5b2c551))
* **Update Confirmation Sheet:** Add top padding ([9aeb156](9aeb156))
* Use correct title size for bottom sheet ([#1687](#1687)) ([3436523](3436523))
* Use correct version code & name ([#1647](#1647)) ([d933997](d933997))
* use lowercase repo names ([#1626](#1626)) ([edd8602](edd8602))

### Features

* Add a toggle for alternative sources ([#1686](#1686)) ([f89c742](f89c742))
* Add API migration code ([#1615](#1615)) ([28ae276](28ae276))
* add haptic feedback ([#1459](#1459)) ([7911459](7911459))
* Allow changing languages ([#1488](#1488)) ([f82c439](f82c439))
* Display current app language at the top of the list ([aa0575a](aa0575a))
* Hide the Install button during installation ([#1633](#1633)) ([3e696d6](3e696d6))
* Improve app selector and patcher UI ([#1616](#1616)) ([efb2d5e](efb2d5e))
* Improve consistency on language selector ([b2119ce](b2119ce))
* Improve language distinguishness and resolve language-specific issues ([#1706](#1706)) ([6d866d4](6d866d4))
* Improve Split APK warning readability ([#1625](#1625)) ([6fd740f](6fd740f))
* Show a dialog when an update is available ([#1654](#1654)) ([c7d975e](c7d975e))
* Simplify settings strings ([#1618](#1618)) ([0d45fe4](0d45fe4))
* Use more appropriate progress values ([2d7026a](2d7026a))
* use native TextField ([9ed43ef](9ed43ef))

### Performance Improvements

* Load patched apps as soon as possible ([c94eb7a](c94eb7a))

### Reverts

* WillPopScope migration ([3b0fed5](3b0fed5))
@kitadai31 kitadai31 deleted the kitadai31-patch-1 branch March 25, 2024 07:09
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