-
-
Notifications
You must be signed in to change notification settings - Fork 27
feat: Magisk module install #127
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
base: dev
Are you sure you want to change the base?
Changes from 31 commits
85dd550
9ba4e3e
a283be1
b180aea
6299737
647e7d6
bba8eb3
3e9dbc9
1d972f1
f6e0783
fa4e883
b026668
c2d57b7
35ab7dc
2c4a8d6
7b86534
08ac5dd
ed0c7a0
dda483b
e022803
09b39f8
277e368
754bddf
2e64ffd
e4c06cb
c229f11
3280af0
d27169d
3119114
fe6f102
650e952
e276095
d0f432e
041519a
84738bb
6fded1d
3d020fa
55f752c
b9d0a1b
995d155
b65883f
ce3e0e6
91c4b87
86887b2
dc438d2
547764d
d1f1076
2d19f45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,305 @@ | ||
| package app.revanced.library | ||
|
|
||
| import app.revanced.library.installation.installer.Constants | ||
| import app.revanced.library.installation.installer.Constants.invoke | ||
| import com.topjohnwu.superuser.Shell | ||
| import com.topjohnwu.superuser.nio.FileSystemManager | ||
| import java.io.File | ||
| import java.util.zip.ZipFile | ||
|
|
||
| object MagiskUtils { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can be internal no?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually looks like these are APIs for callsite? Why are they needed?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I got an error while testing patching with youtube, it failed to load since there were missing native libraries, so this basically includes the necessary libs
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And MagiskUtils is used directly by the manager, so it can't be internal
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. APIs in library should only exist in library if they can/should be shared with cli too, but i cant see how this would apply for a couple of apis introduced here.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
MagiskUtils is called directly from the manager's RootInstaller for live mount & unmount operations in a coroutine scope, so it must stay public
Understandable but for android specific APIs like prepareMagiskModule & extractNativeLibraries don't make sense for CLI imo, but we could always move it into manager as a specialized internal helper if you want
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Manager should not require any live mount/unmount operations at all, this is business logic of the library installer that manager should use and not reimplement. So The APIs here should be designed to reflect that.
Why not? CLI can prepare a magisk module via ADB too? |
||
| const val MODULES_PATH = "/data/adb/modules" | ||
|
|
||
| /* | ||
| Shell.isAppGrantedRoot() queries libsu's internal state. | ||
| It returns false until a root shell has actually been built and verified by libsu. | ||
| */ | ||
| fun hasRootAccess() = Shell.isAppGrantedRoot() ?: false | ||
|
|
||
| /* | ||
| Checks whether su exists in PATH. Pure filesystem check, no shell needed. | ||
| Returns true as soon as APatch makes su available, regardless of whether your app was granted root. | ||
| */ | ||
| fun isDeviceRooted() = | ||
| System.getenv("PATH")?.split(":")?.any { path -> File(path, "su").canExecute() } ?: false | ||
|
|
||
| /* | ||
| Shell.getShell().isRoot forces a shell to be built (or returns the cached one) and checks if it came up as root. | ||
| This is the only reliable "root is usable right now" check. | ||
| */ | ||
| fun isMagiskInstalled() = Shell.getShell().isRoot | ||
|
|
||
| /* | ||
| Returns true if root was granted, false if denied. | ||
| Must be called on a background thread. | ||
| */ | ||
| fun requestRoot(): Boolean { | ||
| Shell.getCachedShell()?.takeIf { !it.isRoot }?.close() | ||
| return Shell.getShell().isRoot | ||
| } | ||
|
|
||
| fun isInstalled(packageName: String, remoteFS: FileSystemManager) = | ||
| remoteFS.getFile("$MODULES_PATH/$packageName-revanced").exists() | ||
|
|
||
| fun isInstalledAsMagiskModule(packageName: String, remoteFS: FileSystemManager) = | ||
| remoteFS.getFile("$MODULES_PATH/revanced_${packageName.replace('.', '_')}").exists() | ||
|
|
||
| /* | ||
| Bind-mounts the patched APK over the stock APK path using the Magisk mirror when | ||
| available, ensuring the mount is visible across all process namespaces (required on | ||
| Zygisk/MIUI). | ||
| */ | ||
| fun mount(packageName: String, sourceDir: String) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this function needed? Installers already implement it
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like all these utils functions below
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are called from the manager's own RootInstaller for live mounts outside the library's install lifecycle (re-mounting on app launch without reinstalling). The library installers handle their own mounting on the installation flow
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I dont think manager should have this feature, in which case the utils function should be removable no?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry but I dont get what you meant here, I guess you want to get rid of the mount & unmount from the manager and directly call those 2 functions on library?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Basically, manager should not provide in-place mount & unmount options. Installation and uninstallation over mount is handled by installers which manager is supposed to call. |
||
| if (Shell.getShell().newJob().add("mount | grep -q \"$sourceDir\"").exec().isSuccess) { | ||
| unmount(sourceDir) | ||
| } | ||
|
|
||
| val patchedApkPath = Constants.MOUNTED_APK_PATH(packageName) | ||
| val fallbackPath = "$MODULES_PATH/$packageName-revanced/$packageName.apk" | ||
|
|
||
| val patchedApk = when { | ||
| Shell.getShell().newJob().add("[ -f \"$patchedApkPath\" ]").exec().isSuccess -> patchedApkPath | ||
| Shell.getShell().newJob().add("[ -f \"$fallbackPath\" ]").exec().isSuccess -> fallbackPath | ||
| else -> throw ShellCommandException("Patched APK not found for $packageName", -1, emptyList(), emptyList()) | ||
| } | ||
|
|
||
| Shell.getShell().newJob().add($$""" | ||
| MIRROR="" | ||
| if command -v magisk >/dev/null 2>&1; then | ||
| if ! MAGISKTMP=$(magisk --path 2>/dev/null); then MAGISKTMP=/sbin; fi | ||
| MIRROR=${MAGISKTMP}/.magisk/mirror | ||
| [ -d "${MIRROR}" ] || MIRROR="" | ||
| fi | ||
| mount -o bind "${MIRROR}$$patchedApk" "$$sourceDir" | ||
| """.trimIndent()).exec().assertSuccess("Failed to mount APK") | ||
| } | ||
|
|
||
| fun unmount(sourceDir: String) { | ||
| // Skip if not mounted | ||
| val checkMount = Shell.getShell().newJob().add("mount | grep -q \"$sourceDir\"").exec() | ||
| if (!checkMount.isSuccess) return | ||
|
|
||
| Shell.getShell().newJob() | ||
| .add("umount -l \"$sourceDir\"") | ||
| .exec() | ||
| .assertSuccess("Failed to unmount APK") | ||
| } | ||
|
|
||
| fun uninstall(packageName: String, remoteFS: FileSystemManager) { | ||
| val unifiedPath = Constants.MOUNTED_APK_PATH(packageName).substringBeforeLast("/") | ||
| remoteFS.getFile(unifiedPath).deleteRecursively() | ||
|
|
||
| remoteFS.getFile("$MODULES_PATH/$packageName-revanced").deleteRecursively() | ||
| .also { if (!it) throw Exception("Failed to delete files") } | ||
| } | ||
|
|
||
| fun uninstallMagiskModule(packageName: String, patchedPackageName: String, remoteFS: FileSystemManager) { | ||
| val unifiedPath = Constants.MOUNTED_APK_PATH(packageName).substringBeforeLast("/") | ||
| remoteFS.getFile(unifiedPath).deleteRecursively() | ||
|
|
||
| val formattedPackageName = packageName.replace('.', '_') | ||
| val handleDisabledScriptPath = Constants.HANDLE_DISABLED_SCRIPT_PATH(formattedPackageName) | ||
|
|
||
| Shell.getShell().newJob() | ||
| .add("pm uninstall --user 0 \"$patchedPackageName\"") | ||
| .add("rm -f \"$handleDisabledScriptPath\"") | ||
| .exec() | ||
|
|
||
| remoteFS.getFile("$MODULES_PATH/revanced_$formattedPackageName").deleteRecursively() | ||
| .also { if (!it) throw Exception("Failed to delete Magisk module files") } | ||
| } | ||
|
|
||
| fun extractNativeLibraries(apkFile: File, systemAppPath: String, remoteFS: FileSystemManager) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its needed since it can raise an exception when installing/mounting the app, I've mentioned this exact issue somewhere else on the comments
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I cant find the comment, how/why does it raise an error?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
#127 (comment)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is that thrown when mounting? And how does extractNativeLibraries fix it
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Correct for libs that exist in both the unpatched/stock and patched APKs. The issue is when the patched APK introduces new .so file(s) that were never in the stock APK (i.e libelements.so in YT), PM never extracted those, so dlopen can't find them - only case extractNativeLibraries handles
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For system apps yes but revanced targets user-installed apps (
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Understandable, however i dont think youre exhausting everything that needs mounting no? Is there anything else besides the base apk and the libs we would have to handle in mounting? Granted this including the extractNativeLibraries function are not in scope of this PR, but if you want to introduce this feature here, this functionality shouldve been implemented in RootInstaller class on install/uninstall and not just MagiskUtils.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Actually we dont constrain any target. Mounting installation follows this:
Where the location is is not in our responsibility. It should work anywhere where the app is installed. I could create a ROM which ships e.g. yt as a system app. So, if we patch and mount it, it should work correctly.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Correct, the mount path already does this, the manager reads applicationInfo.sourceDir at runtime, so it naturally follows wherever the app is installed, whether its /data/app or /system/app The system-app caveat I mentioned was specifically about extractNativeLibraries() writing to the lib directory on a read-only system partition. But not longer an "issue" since 754bddf the module path uses pm install, so PM handles lib extraction automatically, and the function has been removed in b9d0a1b |
||
| val libPath = "$systemAppPath/lib" | ||
| remoteFS.getFile(libPath).apply { | ||
| if (exists()) deleteRecursively() | ||
| mkdirs() | ||
| } | ||
|
|
||
| ZipFile(apkFile).use { zip -> | ||
| zip.entries().asSequence() | ||
| .filter { it.name.startsWith("lib/") && it.name.endsWith(".so") } | ||
| .forEach { entry -> | ||
| val parts = entry.name.split("/") | ||
| if (parts.size < 3) return@forEach | ||
|
|
||
| val apkAbi = parts[1] | ||
| val libName = parts.last() | ||
| val systemAbi = when (apkAbi) { | ||
| "arm64-v8a" -> "arm64" | ||
| "armeabi-v7a" -> "arm" | ||
| "x86_64" -> "x86_64" | ||
| "x86" -> "x86" | ||
| else -> apkAbi | ||
| } | ||
|
|
||
| val targetDir = "$libPath/$systemAbi" | ||
| remoteFS.getFile(targetDir).apply { if (!exists()) mkdirs() } | ||
|
|
||
| val targetFile = "$targetDir/$libName" | ||
| zip.getInputStream(entry).use { inputStream -> | ||
| remoteFS.getFile(targetFile).newOutputStream().use { outputStream -> | ||
| inputStream.copyTo(outputStream) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fun installApk(apkPath: String) = | ||
| Shell.getShell().newJob() | ||
| .add("pm install -r -d --user 0 \"$apkPath\"") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've implemented that same user id fetching method on my Shizuku implementation here secp192k1@d8a979b#diff-88608bd83b41bf6ff10291c36c0cb227e766cb43d0a8599885cce7a7cd79dac0R16 @oSumAtrIX what do you think? Keep or replace to dynamic userid?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to specify the user when installing though? Doesnt PM inherit the user that launched the ADB shell/process automatically?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If you have multiple users (private space, work profile, etc) this gives issues big time, since pm install tries to install to all users, gotten this "bug" multiple times, this is a race condition that affects production where signatures mismatch OR says app is already installed - yes on main one it isnt, but its still on private space Plus id say its good practice to define this argument since it would be inherited from where the manager is being ran (
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good, in this case use Process.myUid(). This way it will only install for the current user that runs manager. However, then, this should be implemented in the installer constants too. Why do we need an installApk utility function for manager inside library? The only place where manager does installations is for its own updates or for patched apps. For patched apps it should use the installer classes, for manager updates the functionality should be implemented inside manager repo and not with a shell in library.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
👍 995d155
👍 b65883f |
||
| .exec() | ||
| .assertSuccess("Failed to install APK: $apkPath") | ||
|
|
||
| fun uninstallKeepData(packageName: String) = | ||
| Shell.getShell().newJob() | ||
| .add("pm uninstall -k --user 0 \"$packageName\"") | ||
| .exec() | ||
|
|
||
| fun provisionMagiskModule( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. stage/prepare/provision decide haha (use prepare, simpler)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The --user 0 should also be removed from that command above it, ReVanced/revanced-manager#3288
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| remoteFS: FileSystemManager, | ||
| packageName: String, | ||
| patchedPackageName: String, | ||
| patchedApk: File | ||
| ) { | ||
| val formattedPackageName = packageName.replace('.', '_') | ||
| val modulePath = "$MODULES_PATH/revanced_$formattedPackageName" | ||
| val unifiedApkPath = Constants.MOUNTED_APK_PATH(packageName) | ||
| val handleDisabledScriptPath = Constants.HANDLE_DISABLED_SCRIPT_PATH(formattedPackageName) | ||
|
|
||
| // Ensure directories exist | ||
| val unifiedDir = unifiedApkPath.substringBeforeLast("/") | ||
| Shell.getShell().newJob() | ||
| .add("mkdir -p \"$modulePath\"") | ||
| .add("mkdir -p \"$unifiedDir\"") | ||
| .exec() | ||
| .assertSuccess("Failed to create module directories") | ||
|
|
||
| writeModuleFiles(remoteFS, modulePath, packageName, patchedPackageName) | ||
|
|
||
| // Handle-disabled script: uninstalls the patched app when the module is disabled or removed. | ||
| val handleDisabledSh = Constants.HANDLE_DISABLED_SCRIPT | ||
| .replace("__PATCHED_PKG__", patchedPackageName) | ||
| .replace("__FORMATTED_PKG__", formattedPackageName) | ||
| remoteFS.getFile(handleDisabledScriptPath).newOutputStream().use { it.write(handleDisabledSh.toByteArray()) } | ||
|
|
||
| // Source of truth APK | ||
| copyApk(remoteFS, patchedApk, unifiedApkPath) | ||
|
|
||
| // Set permissions | ||
| Shell.getShell().newJob() | ||
| .add("chmod 644 \"$unifiedApkPath\"") | ||
| .add("chown system:system \"$unifiedApkPath\"") | ||
| .add("chcon u:object_r:apk_data_file:s0 \"$unifiedApkPath\"") | ||
| .add("chmod +x \"$modulePath/service.sh\"") | ||
| .add("chmod +x \"$modulePath/uninstall.sh\"") | ||
| .add("chmod +x \"$handleDisabledScriptPath\"") | ||
| .exec() | ||
| .assertSuccess("Failed to set file permissions") | ||
| } | ||
|
|
||
| fun provisionRootFolder( | ||
| remoteFS: FileSystemManager, | ||
| packageName: String, | ||
| patchedApk: File | ||
| ) { | ||
| val modulePath = "$MODULES_PATH/$packageName-revanced" | ||
| val unifiedApkPath = Constants.MOUNTED_APK_PATH(packageName) | ||
|
|
||
| // Ensure directories exist | ||
| val unifiedDir = unifiedApkPath.substringBeforeLast("/") | ||
| Shell.getShell().newJob() | ||
| .add("mkdir -p \"$modulePath\"") | ||
| .add("mkdir -p \"$unifiedDir\"") | ||
| .exec() | ||
| .assertSuccess("Failed to create module directories") | ||
|
|
||
| // MOUNT type: patched package name == original package name (bind-mount, no rename) | ||
| writeModuleFiles(remoteFS, modulePath, packageName, packageName) | ||
|
|
||
| // Source of truth APK | ||
| copyApk(remoteFS, patchedApk, unifiedApkPath) | ||
|
|
||
| Shell.getShell().newJob() | ||
| .add("chmod 644 \"$unifiedApkPath\"") | ||
| .add("chown system:system \"$unifiedApkPath\"") | ||
| .add("chcon u:object_r:apk_data_file:s0 \"$unifiedApkPath\"") | ||
| .add("chmod +x \"$modulePath/service.sh\"") | ||
| .add("chmod +x \"$modulePath/uninstall.sh\"") | ||
| .exec() | ||
| .assertSuccess("Failed to set file permissions") | ||
| } | ||
|
|
||
| private fun writeModuleFiles( | ||
| remoteFS: FileSystemManager, | ||
| modulePath: String, | ||
| packageName: String, | ||
| patchedPackageName: String, | ||
| ) { | ||
| val formattedPackageName = packageName.replace('.', '_') | ||
|
|
||
| val moduleProp = Constants.MAGISK_MODULE_PROP | ||
| .replace("__FORMATTED_PKG__", formattedPackageName) | ||
| .replace("__PKG_NAME__", packageName) | ||
| remoteFS.getFile("$modulePath/module.prop").newOutputStream().use { it.write(moduleProp.toByteArray()) } | ||
|
|
||
| val serviceSh = Constants.MODULE_SERVICE_SCRIPT | ||
| .replace("__PKG_NAME__", packageName) | ||
| .replace("__PATCHED_PKG__", patchedPackageName) | ||
| remoteFS.getFile("$modulePath/service.sh").newOutputStream().use { it.write(serviceSh.toByteArray()) } | ||
|
|
||
| val uninstallSh = Constants.MAGISK_UNINSTALL_SCRIPT | ||
| .replace("__PKG_NAME__", packageName) | ||
| .replace("__PATCHED_PKG__", patchedPackageName) | ||
| .replace("__FORMATTED_PKG__", formattedPackageName) | ||
| remoteFS.getFile("$modulePath/uninstall.sh").newOutputStream().use { it.write(uninstallSh.toByteArray()) } | ||
| } | ||
|
|
||
| private fun copyApk(remoteFS: FileSystemManager, source: File, destination: String) { | ||
| remoteFS.getFile(source.absolutePath) | ||
| .also { if (!it.exists()) throw Exception("Source APK file doesn't exist: ${source.absolutePath}") } | ||
| .newInputStream().use { inputStream -> | ||
| remoteFS.getFile(destination).newOutputStream().use { outputStream -> | ||
| inputStream.copyTo(outputStream) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private fun Shell.Result.assertSuccess(errorMessage: String) { | ||
| if (!isSuccess) { | ||
| throw ShellCommandException(errorMessage, code, out, err) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| class ShellCommandException( | ||
| val userMessage: String, | ||
| val exitCode: Int, | ||
| val stdout: List<String>, | ||
| val stderr: List<String> | ||
| ) : Exception(format(userMessage, exitCode, stdout, stderr)) { | ||
| companion object { | ||
| private fun format( | ||
| message: String, | ||
| exitCode: Int, | ||
| stdout: List<String>, | ||
| stderr: List<String> | ||
| ): String = buildString { | ||
| appendLine(message) | ||
| appendLine("Exit code: $exitCode") | ||
|
|
||
| val output = stdout.filter { it.isNotBlank() } | ||
| val errors = stderr.filter { it.isNotBlank() } | ||
|
|
||
| if (output.isNotEmpty()) { | ||
| appendLine("stdout:") | ||
| output.forEach(::appendLine) | ||
| } | ||
| if (errors.isNotEmpty()) { | ||
| appendLine("stderr:") | ||
| errors.forEach(::appendLine) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| package app.revanced.library.installation.installer | ||
|
|
||
| import android.content.Context | ||
| import app.revanced.library.installation.command.LocalShellCommandRunner | ||
| import com.topjohnwu.superuser.ipc.RootService | ||
| import java.io.Closeable | ||
|
|
||
| /** | ||
| * [LocalMagiskInstaller] for installing and uninstalling [Apk] files locally with root permissions via Magisk modules. | ||
| * | ||
| * @param context The [Context] to use for binding to the [RootService]. | ||
| * @param onReady A callback to be invoked when [LocalMagiskInstaller] is ready to be used. | ||
| * | ||
| * @throws NoRootPermissionException If the device does not have root permission. | ||
| * | ||
| * @see Installer | ||
| * @see LocalShellCommandRunner | ||
| */ | ||
| @Suppress("unused") | ||
| class LocalMagiskInstaller private constructor( | ||
| context: Context, | ||
| onReady: LocalMagiskInstaller.() -> Unit, | ||
| private val readyHook: Array<(() -> Unit)?>, | ||
| ) : MagiskInstaller( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, Is there any reason why this class does not extend LocalRootInstaller? MagiskRootInstaller : RootInstaller No?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically from here #127 (comment) |
||
| { LocalShellCommandRunner(context) { readyHook[0]?.invoke() } }, | ||
| ), | ||
| Closeable { | ||
|
|
||
| constructor( | ||
| context: Context, | ||
| onReady: LocalMagiskInstaller.() -> Unit = {}, | ||
| ) : this(context, onReady, arrayOfNulls(1)) | ||
|
|
||
| init { | ||
| // The supplier passed to [MagiskInstaller] runs during super-init, before `this` | ||
| // exists as a subclass reference, so the ready callback cannot capture it directly. | ||
| // Instead we route through [readyHook], which is populated here — safe because | ||
| // [LocalShellCommandRunner.onServiceConnected] fires asynchronously after IPC bind. | ||
| readyHook[0] = { onReady() } | ||
| } | ||
|
|
||
| override fun close() = (shellCommandRunner as LocalShellCommandRunner).close() | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,16 +17,27 @@ import java.io.Closeable | |
| * @see LocalShellCommandRunner | ||
| */ | ||
| @Suppress("unused") | ||
| class LocalRootInstaller( | ||
| class LocalRootInstaller private constructor( | ||
| context: Context, | ||
| onReady: LocalRootInstaller.() -> Unit = {}, | ||
| onReady: LocalRootInstaller.() -> Unit, | ||
| private val readyHook: Array<(() -> Unit)?>, | ||
| ) : RootInstaller( | ||
| { installer -> | ||
| LocalShellCommandRunner(context) { | ||
| (installer as LocalRootInstaller).onReady() | ||
| } | ||
| }, | ||
| { LocalShellCommandRunner(context) { readyHook[0]?.invoke() } }, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whats the reason this is a lambda instead of: RootInstaller(LocalShellCommandRunner(context) { readyHook[0]?.invoke() })
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Defers its creation until the installer is first used. Forcing it to initialize as soon as its seen would trigger the root shell initialization while its still in construction, before the user has had a chance to check or request root (which is important in the case of whitelisting root privileges later down the line)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The ctor evalutes the lambda on init: Its not really deferring here.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Refactored this at secp192k1@3d020fa |
||
| ), | ||
| Closeable { | ||
|
|
||
| constructor( | ||
| context: Context, | ||
| onReady: LocalRootInstaller.() -> Unit = {}, | ||
| ) : this(context, onReady, arrayOfNulls(1)) | ||
|
|
||
| init { | ||
| // The supplier passed to [RootInstaller] runs during super-init, before `this` | ||
| // exists as a subclass reference, so the ready callback cannot capture it directly. | ||
| // Instead we route through [readyHook], which is populated here — safe because | ||
| // [LocalShellCommandRunner.onServiceConnected] fires asynchronously after IPC bind. | ||
| readyHook[0] = { onReady() } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to understand, you cant just do: class LocalRootInstaller private constructor( Because onReady is called during super init? Since there is no additional subclass constructor technically speaking the runner is ready at super init, as long as this ready call is inserted at the end of super init, but thats understandably difficult to guarantee. I think a latch based approach waiting the thread until ready simplifes the code a lot, sparing us from this weird situation. If libsu is ready on another thread this shouldnt deadlock
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The latch approach would deadlock if LocalRootInstaller is constructed on the main thread, since onServiceConnected also dispatches on the main thread. |
||
| } | ||
|
|
||
| override fun close() = (shellCommandRunner as LocalShellCommandRunner).close() | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.