From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Jim Porter Newsgroups: gmane.emacs.bugs Subject: bug#53293: 29.0.50; [PATCH] `eshell-eval-using-options' should report error for unrecognized option, even with no :external Date: Sun, 16 Jan 2022 12:31:37 -0800 Message-ID: References: <60740b82-3bea-7fb9-bfc6-617488f656a8@gmail.com> <838rvgyrtu.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="36175"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 53293@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sun Jan 16 21:32:20 2022 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1n9CCK-0009A1-8m for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 16 Jan 2022 21:32:20 +0100 Original-Received: from localhost ([::1]:46806 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1n9CCI-00011G-HP for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 16 Jan 2022 15:32:18 -0500 Original-Received: from eggs.gnu.org ([209.51.188.92]:34872) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1n9CC8-00010n-4k for bug-gnu-emacs@gnu.org; Sun, 16 Jan 2022 15:32:08 -0500 Original-Received: from debbugs.gnu.org ([209.51.188.43]:51408) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1n9CC2-0000NX-87 for bug-gnu-emacs@gnu.org; Sun, 16 Jan 2022 15:32:07 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1n9CC1-00063w-W8 for bug-gnu-emacs@gnu.org; Sun, 16 Jan 2022 15:32:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Jim Porter Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 16 Jan 2022 20:32:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 53293 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 53293-submit@debbugs.gnu.org id=B53293.164236511023288 (code B ref 53293); Sun, 16 Jan 2022 20:32:01 +0000 Original-Received: (at 53293) by debbugs.gnu.org; 16 Jan 2022 20:31:50 +0000 Original-Received: from localhost ([127.0.0.1]:44311 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1n9CBo-00063V-NA for submit@debbugs.gnu.org; Sun, 16 Jan 2022 15:31:50 -0500 Original-Received: from mail-pf1-f170.google.com ([209.85.210.170]:35713) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1n9CBl-00063F-BW for 53293@debbugs.gnu.org; Sun, 16 Jan 2022 15:31:47 -0500 Original-Received: by mail-pf1-f170.google.com with SMTP id r5so793948pfl.2 for <53293@debbugs.gnu.org>; Sun, 16 Jan 2022 12:31:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=subject:to:cc:references:from:message-id:date:mime-version :in-reply-to:content-language:content-transfer-encoding; bh=DfIS5RAi0mybx9NMCOphxDUn1Nvu60D7cXcwDm3JWWM=; b=mmgvi0WbxyUxQXaaaXHTMtpkH+WN24eRnRg8lv9fPbjF+JLQarlM0TyFy0tGhqwu/d /X6EO7f2ycPL97fLWoEzAEezZeOoVsYF/PLR4t1/mqZj0VJIa7CPAUBaqE2wUqyER6Q8 aEYijOOIuGwdXamP5+5e8UeWm7q1D7OYVMTyGs5I3r7uotUMyyQH7S5JT15l67H/W/5z 30Ug/6ZPJ5zg4i5ekRrruR+T049uHPSWId9tqYcUIyIlo6Lv2mIAPrVb9RJxubL5vb8a NRlyL1xjo6pxh8QEni/4CT9ev9ny9YyeZD4KNho7f7uVNrZtwl+8e3Pv2SkbeRTMsI+i 9VqA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=DfIS5RAi0mybx9NMCOphxDUn1Nvu60D7cXcwDm3JWWM=; b=zj/2EzW8f2jLSsQS8TIGSk76JAIteKL0/puEvJu7ZxGREK9mWUGz7s8Nyvnf7knle1 5wojk6ukVhA/HwSaxBzRAinN0td6NlxsX+8qpzFkOob9o/ESkwYoLg8Si7m3WjIVFx5u jVllJwVYn6vhhjBwsshH5CKTxMOtlLfhOBCj+2n5ShHbgyi7Jp5TSx8Nw3RdqSfnzmVQ 45jgHxOJvaqLscIJoarzusy+Og2MK8odfMmVrxyLziRFVKZuw9ykU9fzpdoyPLbDgUQD 6D3ilDUUY7vnvMl3+qMXg6qKnwa0suHe25erg4XZRBJYSSk51/rPmHVTwktu3IozGeFH Z7/g== X-Gm-Message-State: AOAM530gsPnFPAYr9xTbjT8IFdRGndvQc5RhFLynnmL/WoqLD7yfCsCt tm1g20nKmqd4xWCDW9aRHYgJkvCc5e7bBw== X-Google-Smtp-Source: ABdhPJxcDsgbnz99wBTR6ats6YClHT/1Uoz3ze4TvjMBtkjp2Rb3M8ueK7pGHxlLw5vkiES3ZiNH5A== X-Received: by 2002:a63:ad4a:: with SMTP id y10mr16130563pgo.413.1642365098909; Sun, 16 Jan 2022 12:31:38 -0800 (PST) Original-Received: from [192.168.1.2] (cpe-76-168-148-233.socal.res.rr.com. [76.168.148.233]) by smtp.googlemail.com with ESMTPSA id a1sm12299893pfv.99.2022.01.16.12.31.37 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 16 Jan 2022 12:31:38 -0800 (PST) In-Reply-To: <838rvgyrtu.fsf@gnu.org> Content-Language: en-US X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:224416 Archived-At: On 1/16/2022 1:04 AM, Eli Zaretskii wrote: >> From: Jim Porter >> Date: Sat, 15 Jan 2022 20:03:46 -0800 >> >> Despite this incompatibility, I still think this is the right change to >> make for all these commands. If a user passes unrecognized options to >> any of these, they should be informed of that fact. > > OTOH, with the current behavior shell scripts that use options not > supported by Eshell commands will silently run unaltered, wheres with > your change they will error out. Do we care about this? Yeah, that's one of the tricky parts about this. On the one hand, some options (boolean flags) can be silently ignored without altering the meaning of the command *too* much; on the other hand, some options (especially those taking an argument) can't be ignored without drastically changing the meaning. The latter case comes up with commands like "su -c CMD". In a regular shell, this will run CMD as root, but in Eshell, it silently ignores the "-c", so the command looks like "su CMD"; thus, it means "switch to the user named CMD (via the TRAMP su method)". Since some options can be (relatively) safely ignored and others can't, maybe the best approach would be to audit the list of commands I posted in the original message to see if there are any options that users might specify that we can silently ignore. Then we can update the option spec for those commands to parse those options but not do anything with them. Here are all the affected Eshell commands compared to their shell equivalents: addpath No direct shell equivalent. Erroring out on unrecognized options should therefore be safe. echo Eshell echo works quite a bit differently by default. From eshell/em-basic.el, it outputs "in a Lisp-friendly fashion (so that the value of echo is useful to a Lisp command using the result of echo as an argument)". However, with `eshell-plain-echo-behavior' set to non-nil, it works more like shell echo. I'm aware of 3 options for various implementations of shell echo: -n: suppress trailing newline This is recognized by Eshell echo, but actually does the opposite: it *inserts* a trailing newline. I posted a patch to help improve this to bug#27361. -e: enable interpretation of escape sequences This could probably be accepted by Eshell echo and ignored for now, though it would cause some behavioral differences. Warning or erroring out when seeing this option would also be reasonable, I think. -E: disable interpretation of escape sequences This is the default behavior for echo, so there's really no reason *not* to support this option. --version: display the version I doubt this option is used much in scripts, but it wouldn't be hard to support. history Eshell history has some similar options to Bash history. It supports -r, -w, and -a to read, write, and append to the history file, respectively. It also supports getting the Nth history item via "history N". However, Bash history has some additional options: -c: clear history This could theoretically be accepted by Eshell history and ignored, but that would be pretty surprising. If a script is clearing history, there's probably a reason for it, so I think it's best to error out when seeing this option for now. However, it shouldn't be too hard to implement in Eshell if people want it. -d offset / -d start-end: delete history at offset or in range For the same reasons as -c, I think it's best to error out when seeing this option. -p args: perform history substitution on args Since this is supposed to show the result of a history substitution, I think it should error out so as not to cause any confusion or return an invalid result. -s: add history-substituted args to history list This is just an additional option to control the behavior of -p, so the same logic as above applies to this too. source / . It seems Eshell's implementation of these actually has a bug! In a regular shell, running "source file.sh -a -b" would pass -a and -b to file.sh. However, Eshell's source command currently drops -a and -b silently. There are two ways I can think of to fix this: a) stop using `eshell-eval-using-options' entirely. This means that "source --help" changes from showing the help message to sourcing a file named "--help" (this is consistent with source in regular shells) b) using :parse-leading-options-only, which will stop looking for options once it sees a non-option argument like "file.sh". su This (and sudo) both use the TRAMP methods of the same name, so there are probably pretty significant differences between the shell meaning and the Eshell meaning. I'm not super-familiar with how these TRAMP methods work though. Here are the options available: -c / --command CMD: run command CMD -s / --shell SHELL: run the specified shell --session-command CMD: run command CMD with no new session All of these take an argument that should affect what gets run by su. I think erroring out makes sense for all of them. -g / --group GROUP: specify a group -G / --supp-group GROUP: specify a supplemental group -w / --whitelist-environment LIST These are all options that take an argument and which users would probably expect to do what they say, since they affect security. I think erroring out makes sense for all of them. -f / --fast: pass -f to the shell Since the manual for su says this "may or may not be useful, depending on the shell", it would probably be ok to ignore. However, I'm not sure this is a very common option, so maybe erroring out is fine too. - / -l / --login: provide a login environment This is supported by Eshell's su command. -m / -p / --preserve-environment: preserve all env vars If a user wants to preserve the environment, we shouldn't just ignore that. However, since the shell su command ignores this when --login is set, we could ignore it then too. That's probably not particularly useful though. When would a user explicitly pass both --login and --preserve-environment? -P / --pty: create a pseudo-terminal I'm not sure what to do about this one. It might be reasonable to ignore this, but on the other hand, a user requesting a PTY probably has a reason for it and might be unpleasantly surprised if we just ignore that request. -V / --version: display version As with echo --version, I doubt this option is used much in scripts, but it wouldn't be hard to support. sudo This is similar to the su command above, but sudo has a lot more options. -E / --preserve-env[=LIST]: preserve all/selected env vars -g / --group GROUP: specify a group These work like the corresponding su options and so I think erroring out is best here too. -A / --askpass: use a helper program to ask for the password -C / --close-from NUM: close file descriptors >= NUM -H / --set-home: set the HOME env var -K / --remove-timestamp: remove cached credentials -k / --reset-timestamp: invalidate cached credentials -P / --preserve-groups: preserve invoking user's group -r / --role ROLE: set SELinux role -t / --type TYPE: set SELinux type -v / --validate: validate cached credentials These all directly affect security, so we should be safe and error out instead of ignoring them. -B / --bell: ring the bell in the password prompt Since this is just a beep, it would probably be ok to ignore. However, I'm not sure this is a very common option, so maybe erroring out is fine too. -b / --background: run command in background It might be ok to ignore this, but I'm not sure what the implications of doing so would be. I'd err on the side of caution and report an error for this option. -e / --edit: edit a file instead of running command -i / --login: run the user's login shell -s / --shell: run the shell set in SHELL env var These all affect what command is actually run, so we should error out instead of silently running the wrong command. -h / --host HOST: run command on specified HOST This isn't supported, but unfortunately, we use "-h" to mean "help", which could result in confusing output. (sudo uses a "-h" without an argument for "help" and a -h with one for "host".) However, I don't think it will *do* anything invalid (aside from printing the help message), so it should be ok for now. -l / --list: list allowed/forbidden commands for user -u / --user USER: set user for --list These both drastically change what sudo does, so erroring out is the right thing to do, I think. -n / --non-interactive: don't prompt user -p / --prompt PROMPT: customize the password prompt -S / --stdin: read password from stdin I think these are mostly applicable to using sudo from a script, but I'm not sure there's an easy analogue for using them in Eshell. I don't know what to do about these. -V / --version: display version As with echo --version, I doubt this option is used much in scripts, but it wouldn't be hard to support. umask Eshell's version of this is a bit limited in that it can only set the umask if you pass a numeric value. It also only supports the -S (symbolic format) option. -p: output in a form reusable as input I don't think Eshell's umask really supports this properly (in part because it can't accept symbol umasks as input), so it's probably good to error out in this case. ---------------------------------------- In conclusion, I think it would make sense to do the following: * Add support for "echo -E" (no-op) and possibly ignore "echo -e". * Fix the bug with the source command * Consider whether to add support for --version to various Eshell commands. * Decide what, if anything, to do with some of the su/sudo options listed above. However, I welcome other people's thoughts on this. Hopefully the audit above will be enough for people to make an informed opinion about the right thing to do.