rmyorston / busybox-w32

WIN32 native port of BusyBox.
https://frippery.org/busybox
Other
694 stars 126 forks source link

win32: su -c CMD: improvements #317

Closed avih closed 1 year ago

avih commented 1 year ago

First commit fixes an issue which was first mentioned here, where CMD was not constructed correctly for ShellExecute, as it was lacking escape for double-quotes and (some) backslashes, which could result in incorrect value and/or multiple parsed args instead of one.

The second commit allows adding arguments as su -c CMD [ARG...] so that it's easier to use values from the current shell instead of quoting them for shell-re-input and embedding them in CMD.

rmyorston commented 1 year ago

Thanks for persevering with this.

I've put a modified version on my su_cmd branch.

There's already a function to quote arguments, quote_arg() in win32/process.h, so I used that.

I also exported a function to append a string to a list from my make implementation.

avih commented 1 year ago

I've put a modified version on my su_cmd branch.

LGTM, with few comments/questions:

su: allow additional arguments after su -c ... Adds 64 bytes. feels a bit on the low side to me. I think only the longer strings (usage/help) add nearly 30 bytes, and it has quite more functionality (additional loop, function calls). Does 64 sound reasonable to you?

The existing quote_arg looks to me logically equivalent to my xwinq (even if the result is not necessarily identical, I think), but without counting bytes, I'd think that xwinq is a lot smaller at the binary, so consider replacing it if indeed it's equivalent?

xappendword can probably be made more efficent (faster) if needed, by concatenating "manually", similar to what my xappend does, though no idea if it matters. It certainly doesn't matter IMHO for su.

rmyorston commented 1 year ago

Does 64 sound reasonable to you?

It seems reasonable. The first patch added 0 bytes. Exporting xappendword() added 32 for the 32-bit build, subtracted 16 for the 64-bit build. The results for the third patch were:

function                                             old     new   delta
suw32_main                                           272     336     +64
packed_usage                                       13744   13760     +16
.rdata                                             53360   53344     -16
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 2/1 up/down: 80/-16)             Total: 64 bytes

The usage strings are compressed.

so consider replacing it if indeed it's equivalent?

I'll have a look. The existing function has been there since before my involvement with busybox-w32, so it must be doing something right.

The speed of xappendword is indeed mostly irrelevant.

avih commented 1 year ago

The existing function has been there since before my involvement with busybox-w32, so it must be doing something right.

Yeah, it looks good, even if possibly slightly more code than required, and I don't recall encountring issues with it.

Anyway, overall LGTM.

avih commented 1 year ago

For what it's worth, building on the commits in this PR, that's a sudo-like function which forwards stdout, stderr, and the exit code back to the current shell, and is fit mainly for non-interactive commands (because stdout and stderr are invisible at the su window):

EDIT (2023-05-02) Updated to use the new -W option, and now also forwards stdin from the current sh (if not a tty, i.e. pipe or some redirection) to the CMD which sudo runs. stdout/err from CMD are also forwarded back to the current shell.

# forward stdin (if not tty) to CMD, and its stdout/err back to the current sh
sudo() {(
    e() { >&2 printf %s\\n "$*"; exit 1; }
    [ "${1-}" ] || e "usage: sudo CMD [ARG...]  std{in,out,err} must be finite"

    f=$(mktemp -t bb-sudo-XXXXXX) && : >"$f" 2>"$f.err" || exit
    cleanup() { e=$?; rm -f -- "$f" "$f.err" "$f.in"; exit "$e"; }

    trap cleanup INT
    trap cleanup EXIT

    { [ -t 0 ] || cat; } >"$f.in" || exit  # stdin is intentionally nul if tty

    su -W -- root \
       -c 'echo "$*"; (shift; "$@") <"$1.in" >>"$1" 2>>"$1.err"' sudo "$f" "$@"

    e=$?
    cat <"$f"
    cat <"$f.err" >&2
    exit "$e"
)}
ale5000-git commented 1 year ago

At the moment I can't test it yet, but there are two test cases worth trying: su -c 'echo "0: $0"; echo "1: $1"; echo "2: $2"' -- my_zero '1a 1b' my_two su -- sh -c 'echo "0: $0"; echo "1: $1"; echo "2: $2"' my_zero '1a 1b' my_two

They should both result in:

0: my_zero
1: 1a 1b
2: my_two
avih commented 1 year ago

At the moment I can't test it yet, but there are two test cases worth trying: su -c 'echo "0: $0"; echo "1: $1"; echo "2: $2"' -- my_zero '1a 1b' my_two

They should both result in:

0: my_zero
1: 1a 1b
2: my_two

Define "should".

I think they should not, because the "template" for su -c is sh -c, therefore I think that su -c should support exactly the forms which work with sh -c, and in your examples, if you replace su with sh, then none of them print the output you expect.

The patches here (and in rmyorston's variant) do change how -c is handled, to be more like sh -c (it's a flag without argument, but it's an error to use this flag without operands). See the second commit message.

EDIT: However, on linux, su -c does expect an option argument, and while your first example (su -c '...' -- my_zero ...) doesn't work on linux, it's because the first operand before arguments of sh -c is the user, so this does work on linux:

su -c '...' -- root my_zero ...

So I'm not sure which model should be followed, but as long as only -c is supported (unlike the more extensive linux su), I think it's easier to use when the form is identical to sh -c ....

As for

su -- sh -c 'echo "0: $0"; echo "1: $1"; echo "2: $2"' my_zero '1a 1b' my_two

I don't think it should work, and it also doesn't work on linux, because unlike sudo, su doesn't run individual commands, but rather a shell string for sh -c.

The sudo function above almost runs it, except that this implementation doesn't take any options, and therefore also doesn't handle -- as suggested at the syntax guidelines, but if you remove the -- and replace su with sudo, then it does work

ale5000-git commented 1 year ago

The su on Android (the most complete version) accept both (obviously with the addition of the user, but I haven't written it since it isn't supported on Windows). The second version (without su paramters) is also more supported.

ale5000-git commented 1 year ago

I just wonder, why not reuse the parameter parsing of ash? It can be almost the same.

avih commented 1 year ago

The second version (without su paramters) is also more supported.

I didn't add it because the spec was for sh -c, and it's the same on linux with gnu su, and also with linux busybox su AFAICT. freebsd su doesn't support this form either, but I don't disagree that it could be useful - which is why I wrote sudo above.

I just wonder, why not reuse the parameter parsing of ash? It can be almost the same.

It is almost the same, but I don't see how it particularly utilizes the parameter parsing of ash.

At the sudo implementation above, if you remove the parts which forward back stdout/err and the exit code, and don't wait for the command to complete (which, anecdotally, is exactly what gsudo does, just in a much more elaborate way than sudo above), then it becomes trivial (untested):

sudo() { su -c -- '"$@"' su "$@"; }

and similarly, at the su C code, if operands are detected without -c, then it could simply add the (C) arguments "\"$@\"", "su", <the rest of the operands>. It would be very little code.

One thing worth mentioning though, is that one is then likely to expect this line to work similar to sudo:

su ln -s foo bar

However, it would not work, as su will complain that -s is unsupported.

Apparently, busybox argument parsing permutes the arguments like gnu getopt (which is why ls dir/ -l works), and is similarly affected by the override env var POSIXLY_CORRECT, and so will try to parse -s as a su option even when ln is clearly a non-option argument, which will require to modify the invocation as follows:

su -- ln -s foo bar

Obviously this can be solved too. It's mainly a matter of deciding how we want it to behave.

rmyorston commented 1 year ago

I don't think it'll ever be possible to have sh -c and su -c work in exactly the same way.

There's a new su_cmd2 branch:

avih commented 1 year ago

I don't think I get the value in adding a new applet with such overlapping functionality, while still not making it fully generic for sh invocation.

Also, technically, I don't see how sush FILE [ARG...] is even allowed like the synopsis suggests. Wouldn't that cause this clause to be true (!c_opt && command) which will reject it and print the usage?

Also, if FILE [ARG...] works, wouldn't it also allow things like sush -- -eux -s -c SCRIPT [ARG...] ? because you only add -- with c_opt, so basically the user can add arguments which sh can interpret as additional options.

And, at this stage, wouldn't it be simpler for sush to simply append all arguments, if any, to sh -d ... -t ... and that's it?

Then you don't need to parse options or perform any checks at all, and it's a lot simpler to understand, and you can remove su because such sush would already cover it.

The only thing you'd miss is that -s is not added automatically when -c is used (and effectively undocumented that -s and -c can be used together), and instead the user would need to add it on their own if they wish to use -c and stay interactive. This can be added to the (inline) docs of sh and/or of sush.

rmyorston commented 1 year ago

Wouldn't that cause this clause to be true (!c_opt && command) which will reject it and print the usage?

The !c_opt && command condition never happens because of the previous line

   command = c_opt ? *argv++ : NULL;

Happily the compiler is more observant than me and doesn't emit code for the test.

I don't want to remove the su applet. Though limited it matches how su works on Unix and is the preferred method to get an elevated shell.

The new applet provides more general functionality for people who want to write their own sudo script ;-)

Like -d and -t I'd prefer not to document the use of -s with -c. They're all very handy in the implementation of su but are non-standard.

I tried all the shells I could find on Fedora. The only one that allows -s and -c together is dash. Even upstream BusyBox ash doesn't allow it.

avih commented 1 year ago

The !c_opt && command condition never happens because of the previous line

Right, my bad :) thanks.

I don't want to remove the su applet. Though limited it matches how su works on Unix and is the preferred method to get an elevated shell.

But it doesn't match linux. On linux you can do this:

$ su -c 'printf "[%s]\n "$0" "$@"' root self foo bar
[self]
[foo]
[bar]

While the current su (also after sush was added) doesn't take arguments that I can tell, so I think at the very least this should be added to su.

The new applet provides more general functionality

So any reason to not make it fully generic, don't parse options at all, and just append all the arguments directly to sh -d ... -t ... ? it would also be easier to document - just like sh with standard sh options/arguments, but elevated and without waiting for it to complete (note, it's not hard to wait for it, but there's no real value in it IMHO if it doesn't actually run/proxied into the current terminal, like gsudo does and to some extent (sans input) my sudo above).

Like -d and -t I'd prefer not to document the use of -s with -c. They're all very handy...

What is the point of undocumented features? If you think it's handly (and I agree it is) then just let users know about it, and you could add that it's experimental and/or subject to removal etc. It doesn't strictly conflict with POSIX, in the sense that POSIX does say that with -c the shell is non interactive, therefore in -s is ignored if -s -c is used, while not ignoring it allows adding a new functionality which was not possible before.

The only one that allows -s and -c together is dash. Even upstream BusyBox ash doesn't allow it.

Right. For what it's worth, freebsd and netbsd sh, and gwsh (all different derivatives of ash) also keep the shell interactive with -s -c.

ale5000-git commented 1 year ago

I don't think it'll ever be possible to have sh -c and su -c work in exactly the same way.

There's a new su_cmd2 branch:

  • The limited Windows version of su has been fixed to properly quote the command. It doesn't support specifying a user so no additional arguments are allowed.

Isn't possible to support specifying a user, even if only root is allowed? This will make the syntax more similar to the real one: https://man7.org/linux/man-pages/man1/su.1.html

ale5000-git commented 1 year ago

The most useful use case is auto-elevate from inside a script with an indefined amount of parameters (that may contain spaces): su -c "AUTO_ELEVATED=true sh -- '${0:?}' \"\${@}\"" root '[su]my script name' "${@}"

ale5000-git commented 1 year ago

What is the point of undocumented features? If you think it's handly (and I agree it is) then just let users know about it, and you could add that it's experimental and/or subject to removal etc. It doesn't strictly conflict with POSIX, in the sense that POSIX does say that with -c the shell is non interactive, therefore in -s is ignored if -s -c is used, while not ignoring it allows adding a new functionality which was not possible before.

The only one that allows -s and -c together is dash. Even upstream BusyBox ash doesn't allow it.

Right. For what it's worth, freebsd and netbsd sh, and gwsh (all different derivatives of ash) also keep the shell interactive with -s -c.

If I remeber correctly it was allowed in the past in upstream busybox but they removed it without providing an alternative.

rmyorston commented 1 year ago

OK, there's another attempt on branch su_cmd3.

This adds support for arguments to the -c script and command files. To match the real su it requires a user name to be supplied in these cases. This must be 'root'.

avih commented 1 year ago

OK, there's another attempt on branch su_cmd3.

So, if I understand correctly, the usage is one of those: su -c CMD_STRING [--] [root [NAME [ARG...]]] su [--] [root [CMD_FILE [ARG...]]]

If yes, then I think that the -s -c also needs -- before opt_command, or else, (unlikely) commands which begin with - would be parsed incorrectly as sh options, which then allow the user to add arbitrary arguments to sh, for instance: su -c -eux -- root -Cf 'printf %s\\n $0 $*' name foo bar which would invoke, I think: ... -s -c -eux -Cf 'printf %s\\n $0 $*' name foo bar

And similarly, it needs to add -- before CMD_FILE.

And, this has unbalanced square brackets: "[-c 'CMD'] [root [FILE ARGS | ARG0 ARGS]"

Other than those, LGTM.

rmyorston commented 1 year ago

needs -- before opt_command

Neither upstream BusyBox su nor su on Linux seem to do that. Maybe it should be considered a feature rather than a bug?

unbalanced square brackets

I can fix that!

avih commented 1 year ago

Neither upstream BusyBox su nor su on Linux seem to do that. Maybe it should be considered a feature rather than a bug?

But wait, it gets worse! A script or user which would rightly want to cover all bases for arbitrary arguments after root, would need to invoke it like so: su -c CMD_STRING -- root -- NAME "$@" su -- root -- path/to/script "$@"

(or move the -- to after root, like so: su -c CMD_STRING root -- -- NAME "$@".

One -- to stop su parsing option-like arguments after root, because it premutes by default, and one to prevent sh from interpreting option-like arguments as options.

And, if you disable getopt permutations by changing "c:" to "+c:", then it becomes even more weird:

su -c CMD_STRING root -- "$@"

Because now the -- is strictly needed for sh and not for su at all! This is not easy to explain...

All of those are fixed if you add -- to the sh ... command before opt_command with -c, or before the rest of the arguments otherwise, and disable permutations.

rmyorston commented 1 year ago

Comment in upstream BusyBox su code:

    /* Note: we don't use "'+': stop at first non-option" idiom here.
     * For su, "SCRIPT ARGS" or "-c CMD ARGS" do not stop option parsing:
     * ARGS starting with dash will be treated as su options,
     * not passed to shell. (Tested on util-linux 2.28).
     */
avih commented 1 year ago

Meh. Indeed this works in su from util-linux

su -- root -- ./myscript -a -b

where one -- goes to su, and the other to sh (and myscript sees "$@" of -a and -b).

The manpage says:

When user is specified, additional arguments can be supplied, in which case they are passed to the shell.

Which doesn't make it obvious how it behaves.

Personally I'd consider it a bug that two -- might be required, but whatever...

Thanks for cross referencing it with upstream and util-linux. I should have done the same too first.

EDIT Actually, the busybox synopsis is simply incorrect. it's not FILE [ARG...], but rather just what the su manpage says above - args to sh, and so this works on linux (because su permutes the args by default):

$ su root -- -c 'printf %s\\n $0 $*' ./myscript -a -b
Password:
./myscript
-a
-b

and this (which works also if su doesn't permute the args)

$ su -- root -c 'printf %s\\n $0 $*' ./myscript -a -b
Password:
./myscript
-a
-b

At which case the -c su argument is redundant, because it can equally be passed directly to sh instead as demonstrated above...

ale5000-git commented 1 year ago

Why not just stop options parsing at first not-option arg or --? root before -c just seems wrong.

ale5000-git commented 1 year ago

For completeness this is the help of one of the most complete su for Android:

su --help
Usage: su [options] [--] [-] [LOGIN] [--] [args...]

Options:
  --daemon                      start the su daemon agent
  -c, --command COMMAND         pass COMMAND to the invoked shell
  -h, --help                    display this help message and exit
  -, -l, --login                pretend the shell to be a login shell
  -m, -p,
  --preserve-environment        do not change environment variables
  -s, --shell SHELL             use SHELL instead of the default /system/bin/sh
  -v, --version                 display version number and exit
  -V                            display version code and exit

It support two --, but the second -- probably serve just to omit the username: su -c 'command' -- -- my_arg0

rmyorston commented 1 year ago

root before -c just seems wrong.

How it seems is immaterial, it's accepted by util-linux su and upstream BusyBox.

ale5000-git commented 1 year ago

With this: su -c 'echo "$0"; echo "$*"' -- root -- script_name -p param_options1 -q param_options2 Are the parameters passed in the same order? Otherwise everything break.

avih commented 1 year ago

root before -c just seems wrong.

Not at all. The gist is that after the user (root), all arguments go directly to sh. It's not FILE [ARG...] like the busybox synopsis suggests, but rather what the util-linux manpage says - simply arguments to sh, and this is the most general invocation of sh in this context.

Using -c as an argument of su, rather than of sh, is redundant, and its only usefulness is to run a shell command without having to input the user name, and/or without using -- to stop the arguments parsing of su. Generic where -c goes to sh: su -- root -c CMD_STRING Identical but simpler to write: su -c CMD_STRING

So if su didn't support -c, then only the first form would work, you'd need both the user and the --.

rmyorston commented 1 year ago

The usage message has been fixed, the patch has been applied to master and a prerelease is available.

Thanks for your patience while this got beaten into shape!

avih commented 1 year ago

The usage message has been fixed

The behavior is good, I think, and that's currently the message as far as I can tell:

"[-c 'CMD'] [root [FILE ARGS | ARG0 ARGS]]"

But I think it's still misleading here and in upstream busybox, for two reasons:

  1. It's not obvious that ARG0 ARGS is only relevant with -c CMD, and that FILE ARGS is only relevant without -c CMD.
  2. It's not communicated at all that after root any additional shell arguments can be added before the ARG0/FILE part, and I think it's actually really important, because that's how it becomes a fully generic sh elevation invocation, rather than two specific modes: -c CMD... and FILE... .

The fact that it does behave like su in util-linux is great, but the busybox docs wrongly paint a different picture IMHO.

avih commented 1 year ago

Also, for reference, another enhancement I had (on top of my patches, so probably doesn't apply over current master) is to support -w for "wait for the process to exit and get its exit code" (it could be argued that this should be the default...). Here's the diff:

diff --git a/loginutils/suw32.c b/loginutils/suw32.c
index 14fda73c3..3b38287db 100644
--- a/loginutils/suw32.c
+++ b/loginutils/suw32.c
@@ -71,12 +71,13 @@ int suw32_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
 int suw32_main(int argc UNUSED_PARAM, char **argv)
 {
    SHELLEXECUTEINFO info;
-   unsigned opts, c_opt;
+   unsigned opts, c_opt, w_opt;
    char *bb_path, *cwd;
    DECLARE_PROC_ADDR(BOOL, ShellExecuteExA, SHELLEXECUTEINFOA *);

-   opts = getopt32(argv, "c");
+   opts = getopt32(argv, "cw");
    c_opt = opts & 1;
+   w_opt = opts & 2;
    if (!c_opt != !argv[optind])
        bb_show_usage();  // -c witout CMD, or operand without -c

@@ -117,8 +118,23 @@ int suw32_main(int argc UNUSED_PARAM, char **argv)
    /* info.lpDirectory = NULL; */
    info.nShow = SW_SHOWNORMAL;

+   if (w_opt)
+       info.fMask |= SEE_MASK_NOCLOSEPROCESS;
+
    if (!INIT_PROC_ADDR(shell32.dll, ShellExecuteExA))
        return -1;

-   return !ShellExecuteExA(&info);
+   if (!ShellExecuteExA(&info))
+       return 1;
+
+   if (w_opt) {
+       WORD r;
+       WaitForSingleObject(info.hProcess, INFINITE);
+       if (!GetExitCodeProcess(info.hProcess, &r))
+           r = 1;
+       CloseHandle(info.hProcess);
+       return r;
+   }
+
+   return 0;
 }
ale5000-git commented 1 year ago

@rmyorston It works properly, thank you :)

If possible it would be nice to also support the second --. Two examples:

su -c 'echo "$0"; echo "$*"' -- root -- script_name
su -c 'echo "$0"; echo "$*"' -- -- script_name
avih commented 1 year ago

If possible it would be nice to also support the second --.

-- is not supported explicitly, but rather it's handled by the options parser of the respective utility, and indicates end-of-options (and begining of operands, if any).

Therefore a first -- will be handled by the su options parser, and indicates that none of the following arguments are options to su.

In normal options parsing (POSIX guidelines, and POSIX getopt), -- is not needed if root is given, because root doesn't begin with - and therefore interpreted as the first operand. If -- is given with standard options parsing, then it must come before root.

However, by default, util-linux, upstream busybox, and busybox-w32 permute the arguments of su, and "search" options even after the first argument which doesn't begin with -. And so, -- right before or right after root will be handled by su.

su -c 'echo "$0"; echo "$*"' -- -- script_name

You can't do -- -- because after the su options, if there are any operands, the first must be the user, or, in busybox-w32, root. This is the same in util-linux su, and also in upstream busybox upstream su.

So the first -- indicates end of su options, and, if root was not given, then the second -- would be treated as the user, which is not root, hence rejected.

su -c 'echo "$0"; echo "$*"' -- root -- script_name

This wouldn't work, because -c of su already adds the command string to the arguments it passed to sh, and sh treats it as the first operand, and so the second -- is not needed and won't be interpreted by sh as "end of options" if prior operands were already supplied. When you use -c of su, what follows after root is [script-name [arg...]].

So you simply don't need the second -- here.

If you want generic invocation arguments to sh, then append them to su -- root, like you would for sh:

sh         -c -- 'echo "$0"; echo "$*"' script_name
su -- root -c -- 'echo "$0"; echo "$*"' script_name

We use -- before root because, if standard options parsing is enabled, using env POSIXLY_CORRECT=1, then this would still have identical semantics, but if you put the -- after root, then enabling standard parsing will also change the semantics.

rmyorston commented 1 year ago

I've taken some of the argument quoting code from this PR.

What would people like to see in the usage message?

The "wait for the process to exit and get its exit code" looks reasonable. But I see util-linux su already has a w option. What would be a good alternative?

avih commented 1 year ago

I've taken some of the argument quoting code from this PR.

Sure. In mpv it uses quotes if the string has space/tab/double-quote. Apparently newline doesn't need quoting, but double-quote does trigger quoting. Not sure whether DQ value actually requires quoting.

What would people like to see in the usage message?

Good question. Upstream has this, which I don't think is great either:

Usage: su [-lmp] [-s SH] [-] [USER [FILE ARGS | -c 'CMD' [ARG0 ARGS]]]

Maybe something along these lines?

Usage: su [root]
       su -c CMD_STRING [-- root [ARG0 [ARG...]]
       su -- root [arbitrary sh arguments]

It's a bit overlapping, but it shows that it can be used without arguments, or with CMD_STRING and then if args are needed then -- root is the bulletproof method, or with a fixed prefix followed by arbitrary arguments to sh.

The "wait for the process to exit and get its exit code" looks reasonable. But I see util-linux su already has a w option. What would be a good alternative?

Hmm, I checked only upstream busybox su. Maybe -e for "exit code"?

ale5000-git commented 1 year ago

A minor improvement could also be that if the shell is already root, then it could execute the code without spawn another window and further imitate the real su / sudo.

rmyorston commented 1 year ago

if the shell is already root, then it could execute the code without spawn another window

That's not unreasonable. Though this might be:

diff --git a/loginutils/suw32.c b/loginutils/suw32.c
index 34cb175ab..87b5616c9 100644
--- a/loginutils/suw32.c
+++ b/loginutils/suw32.c
@@ -41,6 +41,15 @@ int suw32_main(int argc UNUSED_PARAM, char **argv)
        ++argv;
    }

+   if (getuid() == 0) {
+       if (opt_command) {
+           *--argv = opt_command;
+           *--argv = (char *)"-c";
+       }
+       *--argv = (char *)"ash";
+       return ash_main(argc, argv);
+   }
+
    /* ShellExecuteEx() needs backslash as separator in UNC paths. */
    bb_path = xstrdup(bb_busybox_exec_path);
    slash_to_bs(bb_path);
avih commented 1 year ago

That's not unreasonable. Though this might be:

Might be unreasonable? :)

What happens with su -cecho ? (didn't try too hard to follow the logic of this *--argv=... stuff).

Also, at the very least it would change the behavior. One of them opens a new window and doesn't wait for it to complete by default, while the other waits for completion at the current window.

I don't think that's desirable.

I also have some suspicion that it could change the command, because with su CMD_STRING is the option-argument of -c, while for sh it's not, so I have a hunch that in some cases the command would end up different.

But in general I agree that not showing the UAC dialog is desirable if we're already elevated.

However, I just tested, and running su from an already-elevated window indeed doesn't show the UAC dialog. So i think that it already works as expected without this additional patch.

rmyorston commented 1 year ago

Might be unreasonable? :)

Yes, that was the implication. ;-)

What happens with su -cecho ?

Right, we'd be pushing back two arguments when only one was consumed. And this:

    *--argv = xasprintf("-c%s", opt_command);

won't work because the shell doesn't like -cecho. Hmm.

avih commented 1 year ago

Well, I don't think it should change the behavior, other than not-showing the UAC dialog if we're already elevated - which is the case already anyway.

Otherwise a script which uses su doesn't know if the command runs "in the background" or the foreground, which could definitely break the script IMO.

ale5000-git commented 1 year ago

A parameter like -n (like: no spawn if possible)?

avih commented 1 year ago

A parameter like -n (like: no spawn if possible)?

What is the value in that?

The way I see it, there could be two cases:

  1. You know you're already elevated, at which case why do you use su at all?
  2. You don't know whether you're elevated, at which case why do you want the behavior to be unexpected?
ale5000-git commented 1 year ago

It is easier to test su if it doesn't spawn a new window every time. Maybe won't be used in production scripts but for testing is useful.

avih commented 1 year ago

It is easier to test su if it doesn't spawn a new window every time.

What does that mean? testing of what? If you're still trying to figure out how to pass arguments to su, then I already told you: just like on linux and with every other version of su, whatever arguments you want to use with an elevated sh, add the exact same arguments after su -- root. That includes any -- which you would have used with sh.

Also, it won't be su anymore, because it would simply run a new sh, and it will modify the arguments in some way, just like the suggested patch above tried to do, and ended up buggy.

ale5000-git commented 1 year ago

Is there a better way to test parameters? For example if I use su -- root wrongtext a new window appears and closes and I can't see anything. This is just an example but maybe something to avoid closing the new window would be nice.

avih commented 1 year ago

Is there a better way to test parameters?

I don't understand the question.

Can you please describe a real world example where you would use such -n option?

ale5000-git commented 1 year ago

How I have said is for my personal testing, instead of a previous question what about a -n as not close the spawned window on error?

avih commented 1 year ago

The "wait for the process to exit and get its exit code" looks reasonable. But I see util-linux su already has a w option. What would be a good alternative?

Hmm, I checked only upstream busybox su. Maybe -e for "exit code"?

Maybe -W (capital)?

How I have said is for my personal testing [...]

Sorry, I don't understand what you're saying, and you still didn't give any example of an actual scenario where -n would be used. Even if only for personal testing, can you give an example of how you would use that, from start to finish?

rmyorston commented 1 year ago

The latest prerelease has two new flags:

avih commented 1 year ago

The latest prerelease has two new flags: ...

LGTM, however, we already know that here and in su elsewhere the -c option is redundant because it can also be passed to sh directly as part of the arbitrary arguments, but we do keep it because other su support it too.

But for -N, which is similarly redundant at su, why add more su code instead of only at ash?

Also, I'd think that the "press any key" message should be at stderr, in case something like sudo above tries to capture it.

(and I'll update sudo above to use -W instead of the poll loop)