-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Nothing installed in Lineage /system/addon.d/ via Magisk app Direct Install + fix_env #3820
Comments
It's supposed to install at some point via the app Direct Install too: Magisk/app/src/main/java/com/topjohnwu/magisk/core/tasks/MagiskInstaller.kt Lines 133 to 134 in cd96454
There was a plan to remove the unneeded installer code duplication and have everything run and maintained in one place via the shell scripts, but I'm not sure where that's at now. |
Yep... all the bits are there. This probably needs just a one- or two-line fix Half of the lines of code in Magisk/scripts/flash_script.sh Lines 78 to 85 in b6643b7
|
So looks like this was intentional. John said "I do not want to involve system rw at anytime during boot. This just opens up a whole can of worms..." I won't close this as #wontfix since he may still change his mind. It would just need to use Magisk's own system mirror and rw it if the addon.d directory exists. 🤞 |
I'm sorry if I'm missing something, but I just don't understand that comment in the context of this bug report, which is about install-time not boot-time. The essential core of this issue is:
That means that some devices out there are getting How does "system rw at anytime during boot" come into this? |
(To rephrase again: logically, either |
He means "while booted", hopefully that clears up the context for you. |
Ohhhhhhh! Yes, that clears it up a lot! Thank you! I now understand why it is the way it is right now. It would be really nice if this difference between the two installation methods was mentioned in the installation documentation (https://topjohnwu.github.io/Magisk/install.html), because it is a pretty significant difference in behavior for a ROM that supports (In any case, at least the next confused schmuck like me that is trying to figure out what is going on will have this bug report to stumble upon. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
addon.d-v2 is only guaranteed to work from a booted OTA. Lineage intentionally didn't want it working in recovery and TWRP only has it working in the _9 branch with some hacks I suggested currently. |
This comment has been minimized.
This comment has been minimized.
For Magisk addon.d script to install it must currently be installed from recovery, then if you have an A/B device (i.e. addon.d-v2) then the only way to be sure the addon.d script runs is to do the OTA through the system settings updater app. |
This comment has been minimized.
This comment has been minimized.
Hmm I believe either recovery should work. It can still install the addon.d script to /system in Lineage Recovery last time I checked. But the Magisk apk must be flashed in recovery for the addon.d script to be installed. If it's not working after that then there's something else weird going on, separate from this issue. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Not sure why regular addon.d (i.e. A-only devices) wouldn't work in LOS Recovery, but, again, that's a different issue from this. Edit: Oh wait, I know why, LOS Recovery doesn't support decrypting /data. 😛 Well known if you check the logs: So yeah probably TWRP only for A-only too. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
That said, Magisk still makes system rw on custom ROMs to remove any existing system su (see remove_system_su), so unless you have a way to systemlessly do that as part of injecting Magisk's su wherever it needs to go, then system rw is still a necessary "evil" and so the addon.d script might as well also remain part of the package. |
remove_system_su only uses on recovery mode, so not "while booted": 17efdff Further, module that adds addon support for magisk: https://github.com/yujincheng08/Magisk-Addon. This also adds support if magisk is installed via boot patch. |
You're incorrect. remove_system_su is run booted as part of addon.d-v2 as well. |
But we are now removing addon support. So when addon support removes, no more remount,rw |
Then I guess you should add remove_system_su to your module. |
Since magisk has one (tho for recovery only), we can reuse it. If magisk removes this function, we can add that too. I am just proving the possibility of the existence of such a module. So the implementation is somehow trivial. Of course, we can add more features. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
It looks like LineageOS won't pass CTS and I'm surprised by the news. LineageOS, being the most popular custom ROM, should avoid splitting the Android community and make the ROM compliant with CDD to reduce the pressure on developers to adapt it. |
I saw the previous link (programminghoch10/Lygisk#20), this feature has never worked on FBE devices. #3037 This feature requires writing to system partitions and unencrypted data partitions, which would be unthinkable in 2022. |
That is a pretty heated conversation here. Very technical too, but I haven't seen anyone consider the problem from the user perspective. It excessively simple: I don't want to be required to reinstall Magisk any time I do a Lineage update. This is a fairly legitimate request, I think. I don't care how ugly It works. And that's all a user asks for. So, until a better solution is found, there's no good reason to deny a feature parity based on that one. |
@christophehenry I absolutely know users' requests. And my point is to avoid magisk doing such a dirty job (since addon.d itself is quite a dirty solution). Instead, a better solution is, no doubt, to build a dedicated magisk module for it, and we have basically developed a very initial version of such a module: https://github.com/yujincheng08/Magisk-Addon. Moreover, other developers are now attracted to making this module not depend on an unencrypted data partition. |
This comment was marked as off-topic.
This comment was marked as off-topic.
@yujincheng08 Sounds promising. Thank you all for your work. |
Hi, it's me, 🤩 @mdoggydog, the original poster here at good ol' #3820 --- "the thread that keeps on giving". 🎅🎁 As a conscientious but naive user, my perspective is:
As a security-conscious software engineer, my perspective is:
Happy Holidays to all the residents of #3820! 😷 🥳 🎉 🍾 🍿 |
Okay, so the best way to handle this would be that addon.d script becomes opt-in only as a button in the app settings, similar to Systemless Hosts, but only visible on a custom ROM with /system/addon.d. That way it can be used regardless of Magisk install method, and a custom ROM user can choose not to use it too. |
I just found out about this issue while doing yet another Magisk reinstall after a LineageOS-based /e/OS update. Is there any roadmap to implement @osm0sis’ proposed solution? @mdoggydog : best user feedback/POV I’ve ever read!!! :D |
Looking on my device in So assuming I have Magisk 26.3 installed on my device, if I (i.e. manually, with open magisk manager before starting lineage os updater (important), minimize magisk manager and run lineage os updater do not reboot when prompted, go back to magisk manager click install magisk and click install after OTA and reboot when prompted (magisk should be present along with your modules and settings) and just let the normal post-OTA-update reboot happen as usual and have Magisk survive the OTA update? My device is A/B if that makes any difference. |
Found a module that works around the issue: https://github.com/mModule/mSurvival |
This one supplies it's own script for
This one installs the |
Just my assumption: if this module only puts the same files in the same place as the original Magisk installation via recovery, then why shouldn't Magisk update these files like its own? |
This comment was marked as off-topic.
This comment was marked as off-topic.
I think It's ok. |
Hi, I noticed that some apps (tk.de) checks if magisk.sh is in addon.d and does not let you use warning that the phone is rooted. |
simply try:
and reboot and try the TK app again |
I installed Magisk by using the "patch boot.img and flash with fastboot" method, since the installation instructions say that using the installer zip via a custom recovery is no longer recommended. On the next LineageOS OTA update, Magisk was not preserved, and I had to manually patch the boot.img and flash with fastboot again.
I discovered that
/system/addon.d/99-magisk.sh
was never installed, thus the OTA update would have no way to preserve Magisk.Digging through the code, it appears that
/system/addon.d/99-magisk.h
is only installed when a Magisk installer zip is used via a custom recovery.A related problem: even if Magisk is initially installed via installer-zip/custom-recovery, it appears that updating Magisk via the MagiskManager app will never update an existing
/system/addon.d/99-magisk.sh
either.This issue is basically a duplicate of #3782, which I think got prematurely closed because I did not explain the problem well enough the first time. Please see my last comment in #3782 for more analysis of this problem.
The text was updated successfully, but these errors were encountered: