mdoggydog commented 3 years ago

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.

yujincheng08 commented 1 year ago

A writable system is never the goal of Magisk.

For feature parity, I am going to remove support for addon.d on custom recovery.

osm0sis commented 1 year ago

For feature parity, I am going to remove support for addon.d on custom recovery.

That's a pretty fucked up thing to say.

It works. Leave it alone.

vvb2060 commented 1 year ago

especially considering the installation instructions list recovery installs as deprecated

addon.d script is part of the recovery installation. Yes, addon.d script is also deprecated.

so writing to system should not be concerning.

We've had issue with not having enough space to write.

addon.d script have various compatibility issues. And sepolicy compromises: https://github.com/topjohnwu/Magisk/blob/4d876f01458e38790eab315a32444f2efece1571/native/src/sepolicy/rules.cpp#L190-L191

osm0sis commented 1 year ago

Lineage runs update_engine permissive. We merely brought that to other custom ROMs to ensure it runs correctly for all. See the blame for an explanation in the commit which originally added that.

Not enough system space to write means the custom ROM is built incorrectly or the user filled it up completely with GApps. Not really Magisk's fault nor concern.

To say addon.d is deprecated really just shows you don't use or care at all about custom ROMs, but Magisk absolutely should continue to care about custom ROMs, which is why I will continue to disagree with you there and maintain Magisk's addon.d support as I have all this time.

vvb2060 commented 1 year ago

Magisk has given up hiding, so I can point out an elephant in the room: checking /system/addon.d confirms this is a rooted device. This is the reason why I want to delete this dir.

yujincheng08 commented 1 year ago

It works. Leave it alone.

Then don't expect feature parity. Magisk is never gonna remount the system as rw while booted.

FMPOV, addon.d is not even a standard (tho some custom ROMs embrace it), and it's quite ugly (telling all apps this is a customized ROM, disallowing using ro filesystem for the system for better compression, etc.).

I still expect to remove native support for addon.d. A third-party Magisk module should be enough for this feature.

osm0sis commented 1 year ago

If Omni and Lineage both have a feature then it's a standard. They're the base for most of the custom ROM community.

It reveals it's a custom ROM, we're only talking about custom ROMs here, which already allow system rw without issue. Leave attempting to hide that to the custom hiding module developers.

This all still comes down to a difference of opinion only.

A compromise could be to never install the addon.d script by default through any method, and have a button in the Magisk app settings (a la Systemless Hosts) which is only visible on custom ROMs with /system/addon.d and will only install the addon.d script when tapped; an opt in only feature.

yujincheng08 commented 1 year ago

If Omni and Lineage both have a feature then it's a standard.

No.

Leave attempting to hide that to the custom hiding module developers.

I don't think we were talking about hiding. My point is that addon.d is bad, and we can definitely have better solutions (say, putting scripts into writable partitions like metadata, persist, data/unencrypted, etc.). I don't think Magisk should natively support such a bad and non-standardized workaround.

I still cannot see any reason why not to leave this support to a third-party module (which is an opt-in, apparently). By this means, users can get any kind of OTA survival support (even not addon.d) by installing an appropriate module.

vvb2060 commented 1 year ago

If Omni and Lineage both have a feature then it's a standard. They're the base for most of the custom ROM community.

This feature is not standard and even differs between Omni and Lineage, only Lineage's update_engine is permissive domain.

It reveals it's a custom ROM, we're only talking about custom ROMs here, which already allow system rw without issue.

Only a small number of users are using custom ROMs. In order to be compatible with this small number of users, the sepolicy of stock ROM has been affected.

Leave attempting to hide that to the custom hiding module developers.

This is technically impossible, at least we (shamiko developers) cannot.

osm0sis commented 1 year ago

Cool well a lot of custom ROM developers have put work into addon.d over the years, so it's unfortunate that you're calling their work ugly and bad. It's a key component of GApps installation. This whole conversation on your side has been pretty callous and loaded, with blatant disregard or dismissiveness about the custom ROM community. Sad to see the core developers of Magisk adopt this attitude.

vvb2060 commented 1 year ago

Have any of the custom ROMs ever run CTS(Compatibility Test Suite)? Do they guarantee they meet the CDD(Android Compatibility Definition Document ) requirements?

If not, they can't even be called Android.

osm0sis commented 1 year ago

Have any of the custom ROMs ever run CTS(Compatibility Test Suite)? Do they guarantee they meet the CDD(Android Compatibility Definition Document ) requirements?

If not, they can't even be called Android.

And there it is. Let's let Lineage know you brazenly don't consider their ROM Android. Maybe you should check in with @topjohnwu before you alienate the entire Android customization community.

yujincheng08 commented 1 year ago

addon.d is bad and ugly no matter how many people were on it. Those people have no choice but to adopt this implementation. Note that you have to stay critical of customized ROMs, even for OEM ROMs. I admit the starting point of addon.d. It was good, and the implementation may be good in the past. but it cannot be good all the time.

addon.d is indeed bad and ugly. at least for now. it deserves better implementations. this fact won't change no matter how many people have worked on it.

moreover, the whole argument was meaningless. you keep arguing about whether addon.d is good or not. but my point is whether magisk should natively support it or leave it to third-party modules. I raised the point that addon.d is bad because I am positive to the custom ROM community that as long as Magisk hands out addon.d to third parties they can come up with better implementations together with corresponding modules.

osm0sis commented 1 year ago

All GApps packages rely on addon.d. It's not going anywhere. Removing it from Magisk just makes life on a custom ROM harder. That shouldn't be the goal here.

yujincheng08 commented 1 year ago

Removing it from Magisk just makes life on a custom ROM harder

A magisk module can relieve them, so why not.

osm0sis commented 1 year ago

A hypothetical module that doesn't exist. addon.d does exist and will continue to exist because it's a staple of custom ROMs for GApps and other add-ons.

Howard20181 commented 1 year ago

Since Android started using full disk encryption or file-based encryption, /data/adb is inaccessible in many cases (Magisk's OTA survival strongly depends on the scripts and files in this location) in Recovery (even LineageOS Recovery does not mount /data), so this feature has been unavailable for a long time.

yujincheng08 commented 1 year ago

A module that doesn't exist.

It will exist as soon as magisk removes addon.d support. Just like root hide modules.

osm0sis commented 1 year ago

It will exist as soon as magisk removes addon.d support. Just like root hide modules.

Hypothetically. There won't be the same drive as there was for root hiding.

yujincheng08 commented 1 year ago

Hypothetically.

what if it comes true? Will you support removing addon.d support?

osm0sis commented 1 year ago

Sure, fuck it, Merry Christmas.

osm0sis commented 1 year ago

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.

yujincheng08 commented 1 year ago

remove_system_su only uses on recovery mode, so not "while booted": https://github.com/topjohnwu/Magisk/commit/17efdff134f4e1788df812dabe4d0b21f63e9c39

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.

osm0sis commented 1 year ago

You're incorrect. remove_system_su is run booted as part of addon.d-v2 as well.

yujincheng08 commented 1 year ago

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

osm0sis commented 1 year ago

Then I guess you should add remove_system_su to your module.

yujincheng08 commented 1 year ago

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.

osm0sis commented 1 year ago

All of your responses have generally ranged from passive aggressive and petty, to outright open contempt throughout this debate. I'm glad @topjohnwu is still the only one who gets to press merge.

yujincheng08 commented 1 year ago

@osm0sis You should stop personally attacking.

osm0sis commented 1 year ago

That's not an attack.

yujincheng08 commented 1 year ago

Well, whether it's or not is not defined by you. But the fact is that you are the only one who says dirty words during the debate :).

vvb2060 commented 1 year ago

@osm0sis

And there it is. Let's let Lineage know you brazenly don't consider their ROM Android. Maybe you should check in with topjohnwu before you alienate the entire Android customization community.

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.

vvb2060 commented 1 year ago

https://github.com/programminghoch10/Lygisk/blob/ci-management/README.md?plain=1#L9-L11

Lygisk fixes the needed /data access in addon.d, which fails to reinstall Magisk during OTA if the device does not support FBE decryption in recovery.

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.

christophehenry commented 1 year ago

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 addons.d is or how hypothetically it may break things in the future.

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.

yujincheng08 commented 1 year ago

@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.

osm0sis commented 1 year ago

My responses have been quite cordial by comparison despite my using of the occasional swear. 🤔

I'll hide this one myself. 😉

christophehenry commented 1 year ago

@yujincheng08 Sounds promising.

Thank you all for your work.

mdoggydog commented 1 year ago

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:

I don't follow Magisk conversations all day on XDA/Twitter/Reddit/etc, but I do my best to RTFM (read the documentation) before I go pestering anyone.
- The Magisk README literally says
  
  "Github is the only source where you can get official Magisk information and downloads."
  
  so I don't think I should even try to look anywhere else.
"Surviving OTA updates" is an essential feature for Magisk, and should be supported on any system where it is possible.
- I am using a ROM that features OTA updates, and I chose that ROM in part because I want those easy OTA updates; I bothered to install Magisk, because I need Magisk; and thus, if Magisk does not survive an OTA update (especially if it could and it used to), I will be frustrated.
"Via .zip sideload in Recovery" is the best way to install Magisk, and I have no idea why it would be deprecated.
- Why the best? Because it is so simple! Look:
```
$ adb reboot recovery
$ adb sideload Magisk-vXXX.zip
```
  That's it! And if I just installed a ROM via Recovery, then I only need the second line! No craziness with extracting a boot.img from somewhere, transfer to phone, patch it, transfer it back, boot to bootloader, flash via fastboot... Who wants to do all that?
- Seriously, "Install via .zip in Recovery" is awesome.
The documentation says nothing about what happens to Magisk after a system update (OTA or not), so if Magisk disappears after an update, I become worried; and if Magisk had survived earlier updates and then disappears, I am confused; and when I finally discover an answer by digging through two-year-old bug reports in github, and I realize that I am just one of many users suffering the same experience, I am frustrated and angry.
- Yes, there is that docs/ota.md page, but it is not linked by any of the documentation index pages. I only find it after desperately rooting around in the Magisk source tree. (Even I keep forgetting it exists, even though this is my favorite topic on my favorite piece of software of all time.) Also, it doesn't mention anything about the addon.d mechanism.
- To this day, the documentation still does not mention, anywhere, that "Install via .zip in Recovery" is the magic that provides the addon.d installation.
I know nothing about Modules and the Magisk documentation does not help me. I do not know where to find modules, how to install them, or (very important) how to assess the trustworthiness of any modules I do find.

As a security-conscious software engineer, my perspective is:

Moving OTA-survival into a module seems reasonable because:
1. As I understand it, OTA-survival will never work automagically on StockROM's, because stock Android has no native mechanism for "specify extra stuff to do at the end of an OTA update", so it cannot be a "universal" feature anyway.
2. Implementation as a module would make it easier to support different/new/additional OTA-survival mechanisms in the future.
3. Set up via installing a module makes it easier to have a single user-flow for enabling OTA-survival, when it is desired and available, no matter how Magisk was installed. (I.e., "Finally, if you want OTA-survival, install one of these modules...")
However, I am concerned that if an essential feature is moved into a module, it will become more fragile because Magisk-core will take the attitude of "not my problem anymore!"
- The basic machinery for OTA-survival needs to be part of the core, and needs to have a stable API. (I assume that there is a lot of overlap between "bits needed to install Magisk" and "bits needed to survive OTA".)
- The focus of survival modules should be on how/when that core machinery is activated.
- The Magisk documentation should, at the very least, inform users that these modules exist and where to find them.
I would not want to see an essential feature, OTA-survival, removed from Magisk-core until there was a well-tested, widely-available, well-documented alternative available.
- I appreciate that @yujincheng08 has a proof-of-concept module already, but I wouldn't expunge addon.d support from Magisk-core yet without the testing, documenting, and informing users.
"Deprecation" means (1) informing other people, through documentation, that a change will be happening; (2) providing a timeline for that change; (3) providing the alternative that should be used to replace the deprecated element. Deprecation is a process to ensure feature-continuity without surprises.
- Mentioning that something is deprecated in a sentence buried in 87 comments in a bug report is, at best, "wishful thinking", at worst, "preparing for failure".

Happy Holidays to all the residents of #3820!

😷 🥳 🎉 🍾 🍿

osm0sis commented 1 year ago

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.

breversa commented 11 months ago

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

brianjmurrell commented 11 months ago

Looking on my device in /system/addon.d/ I don't see any 99-magisk.sh.

So assuming I have Magisk 26.3 installed on my device, if I (i.e. manually, with adb) copy https://github.com/topjohnwu/Magisk/blob/v26.3/scripts/addon.d.sh to /system/addon.d/99-magisk.sh on my device, can I stop having to do:

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.

Anderhar commented 11 months ago

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.

Found a module that works around the issue: https://github.com/mModule/mSurvival

brianjmurrell commented 11 months ago

Further, module that adds addon support for magisk: https://github.com/yujincheng08/Magisk-Addon.

This one supplies it's own script for /data/addon.d. What happens if a newer Magisk updates it's addon.d script?

Found a module that works around the issue: https://github.com/mModule/mSurvival

This one installs the addon.d script that is supplied by the Magisk that is installed. This seems more resilient than the above one supplying it's own. But it still begs the question of what happens if Magisk is updated to a newer version and that newer version of Magisk supplies an updated addon.d script.

Anderhar commented 11 months ago

what happens if Magisk is updated to a newer version and that newer version of Magisk supplies an updated addon.d script.

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?

optimumpr commented 10 months ago

Why can't Magisk, in case of A/B, simply patch both slots the same way TWRP installers do?

Joni-Ahmed-Dev commented 9 months ago

I think It's ok.

duven87 commented 8 months ago

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. Would it be possible that with the hide Magisk app option also edit that script name?

topjohnwu / Magisk

Nothing installed in Lineage /system/addon.d/ via Magisk app Direct Install + fix_env #3820

😷 🥳 🎉 🍾 🍿