Skip to content

su: pass all unknown options#9017

Open
vvb2060 wants to merge 5 commits into
topjohnwu:masterfrom
vvb2060:su
Open

su: pass all unknown options#9017
vvb2060 wants to merge 5 commits into
topjohnwu:masterfrom
vvb2060:su

Conversation

@vvb2060
Copy link
Copy Markdown
Collaborator

@vvb2060 vvb2060 commented May 13, 2025

-c, -l and more are now handled by remote shell.
http://www.mirbsd.org/htman/i386/man1/mksh.htm
This allows users to pass more options to the remote shell, such as rcfile.

@vvb2060 vvb2060 requested a review from yujincheng08 May 13, 2025 10:52
Comment thread native/src/core/su/su.cpp Outdated
Comment thread native/src/core/su/su.cpp Outdated
@yujincheng08
Copy link
Copy Markdown
Collaborator

yujincheng08 commented May 13, 2025

I also recommend adding -- support to it, like other UNIX commands, to pass all stuff after -- to the inner command.

For example,

su -- ls -i

if no -- support,

su ls -i

will make su absorb -i?

@vvb2060
Copy link
Copy Markdown
Collaborator Author

vvb2060 commented May 13, 2025

Already supported --

@yujincheng08
Copy link
Copy Markdown
Collaborator

Add -- to help message?

@vvb2060
Copy link
Copy Markdown
Collaborator Author

vvb2060 commented May 13, 2025

done

@vvb2060 vvb2060 marked this pull request as ready for review May 14, 2025 04:12
@aviraxp
Copy link
Copy Markdown
Collaborator

aviraxp commented May 14, 2025

doc also need ipdate

@vvb2060
Copy link
Copy Markdown
Collaborator Author

vvb2060 commented May 14, 2025

done

@selfmusing
Copy link
Copy Markdown

After PR #9017 was merged into the Magisk Alpha (starting with v29.x), several apps that previously worked without issues are now failing to function properly.

This seems to be related to the recent changes in how su is handled, as reverting to an older Alpha build (pre-v29) or switching to the official Magisk release restores full functionality.

Examples of affected apps:

Valhalla Thor (Play Store)

Google Shortcuts Launcher v5.5 (GitHub)

Is there a recommended way to make these apps work with the latest magisk alpha (v29.x & above), or is this an intentional change that breaks compatibility?

Looking forward to any guidance.

@vvb2060
Copy link
Copy Markdown
Collaborator Author

vvb2060 commented Jul 25, 2025

I recommend that these apps use libsu instead of su -c

@selfmusing
Copy link
Copy Markdown

I recommend that these apps use libsu instead of su -c

Screenshot_20250725-115321_MT Manager Screenshot_20250725-115346_MT Manager

Thanks for the response.

After inspecting the classes.dex of both apps, I can confirm that they indeed rely on executing su -c commands directly to request root access, rather than using libsu.

I've attached screenshots showing the relevant code references for both apps.

This likely explains why their root functionality breaks on Magisk Alpha v29.x and above — due to the recent changes in how su is handled.

Is there any way to temporarily restore compatibility for such apps without needing them to update to libsu?

@vvb2060
Copy link
Copy Markdown
Collaborator Author

vvb2060 commented Jul 25, 2025

Add quotes around the command, for example su -c "ls /data"

@selfmusing
Copy link
Copy Markdown

Add quotes around the command, for example su -c "ls /data"

I've change su -c to su -s on shortcuts launcher app and it worked.
But the other app ain't working even after using su -c "cmd"

Screenshot_20250725-131639_Thor

@vvb2060
Copy link
Copy Markdown
Collaborator Author

vvb2060 commented Jul 25, 2025

not you, ask the app developer to fix it

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request modifies the su command to remove built-in handling of -c (command) and -l (login) options and instead pass all unknown options directly to the remote shell. This allows users more flexibility to pass shell-specific options like --rcfile.

Changes:

  • Removed -c and -l option handling from su, allowing these and other unknown options to be passed to the shell
  • Changed the command structure from a single string to a vector of strings to properly handle arguments
  • Updated help text and documentation to reflect the new behavior and option parsing rules

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
native/src/core/su/su.cpp Main changes to option parsing, command building, and execution logic; removed -c and -l handlers; updated help text
native/src/core/su/daemon.rs Updated SuRequest default to use empty command vector instead of empty string
native/src/core/lib.rs Changed SuRequest struct to use Vec for command instead of separate shell and command strings; removed login field
native/src/core/su/connect.rs Updated command logging to join vector elements with spaces
docs/tools.md Updated documentation to match code changes in help text

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread native/src/core/su/su.cpp
Comment thread docs/tools.md
Comment thread docs/tools.md Outdated
Comment thread native/src/core/su/su.cpp
Comment thread native/src/core/su/su.cpp Outdated
@topjohnwu
Copy link
Copy Markdown
Owner

@vvb2060 is this behavior backwards compatible? I'd prefer to preserve the existing behavior, and only extend the functionality to forward unknown options to the inner shell.

@yujincheng08
Copy link
Copy Markdown
Collaborator

@topjohnwu difference:

$ # before
$ su -c echo 'hello'
hello
$ # after
$ su -c echo 'hello'
$

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread native/src/core/su/su.cpp Outdated
Comment thread native/src/core/su/su.cpp Outdated
Comment thread native/src/core/su/su.cpp
Comment thread docs/tools.md
Comment thread docs/tools.md Outdated
@topjohnwu
Copy link
Copy Markdown
Owner

This changes the CLI API in a backwards incompatible way. I'll need more consideration before merging this PR.

@topjohnwu topjohnwu added the next Planned for the release *after* the next release label Apr 14, 2026
@vvb2060
Copy link
Copy Markdown
Collaborator Author

vvb2060 commented Apr 14, 2026

It is already compatible with the old commands. b915da2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

next Planned for the release *after* the next release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants