[v2] options: Always reset OPTIND in getoptsreset

Commit Message

Herbert Xu May 19, 2024, 2:21 p.m. UTC

On Sun, May 19, 2024 at 02:23:23PM +0100, Harald van Dijk wrote:
>
> This interacts terribly with the not fully reliable but nevertheless fairly
> commonly used "local OPTIND". When the local OPTIND goes out of scope and
> the prior value of OPTIND is restored, the expectation is not that option
> processing restarts from the beginning.

Dash doesn't actually need "local OPTIND".  In fact, if you do
"local OPTIND" then dash will already be broken because even
if OPTIND is reset to the same value, the other internal state
optoff will end up being wrong.

However, it's not hard to make dash ignore "local OPTIND" and
make it work as if it wasn't there.

---8<---
Always reset OPTIND if it is modified by the user, regardless of
its value.

Do not call getoptsreset when returning from a function because
of "local OPTIND" as this simply trashes the caller's getopts
state.

Reported-by: наб <[e-mail]>
Reported-by: Harald van Dijk <[e-mail]>
Signed-off-by: Herbert Xu <[e-mail]>

Comments

наб May 21, 2024, 2:38 p.m. UTC | #1

On Sun, May 19, 2024 at 10:21:42PM +0800, Herbert Xu wrote:
> Always reset OPTIND if it is modified by the user, regardless of
> its value.
I disagree in principle, but I think this is basically fine actually;
strictly, we are allowed to do this
  98842  If the application sets OPTIND to the value 1, a new set of parameters can be used: either the
  98843  current positional parameters or new arg values. Any other attempt to invoke getopts multiple
  98844  times in a single shell execution environment with parameters (positional parameters or arg
  98845  operands) that are not the same in all invocations, or with an OPTIND value modified to be a
  98846  value other than 1, produces unspecified results.
but I cannot prove this breaks existing code in the wild.

I mean such code most likely definitely exists,
but I cannot prove this; but even if it does,
it relies on "unspecified" behaviour, which means it
"cannot be assured to be portable across conforming implementations"
such as dash 1 and dash 2.
I still think it should keep working, since it had worked.

While I found at least one changelog entry in DCS saying
"remove unset OPTIND to work around broken dash shell"
by searching for "unset.*OPTIND\b",
searching "OPTIND=[^01]" gives me heaps of OPTIND=digit
and OPTIND=expression users, but most of them are explicit bash users;
the first one with /bin/sh is
	https://sources.debian.org/src/sptk/3.9-3/debian/scripts/raw2wav/?hl=74#L74
reading
	OPTIND=0
	OPT=""
	for i in "$@"
	do
	    OPTIND=`expr $OPTIND + 1`
	    if [ "$OPT" = "" ]
	    then
		OPTARG=""
but this program doesn't use getopts.

This turns "unset OPTIND must work but doesn't"
         + "OPTIND=asd is invalid but probably shouldn't be"
         + "OPTIND=3 makes getopts parse the third argument (as a happy accident)"
into "unset OPTIND works"
   + "OPTIND=asd works (and restarts getopts as a happy accident)"
   + "OPTIND=3 restarts getopts (as a happy accident)".

So, overall, reasonable, if a tad solomonic.

Patch

diff --git a/src/options.c b/src/options.c
index 4d0a53a..c74e4fe 100644
--- a/src/options.c
+++ b/src/options.c
@@ -393,7 +393,7 @@  setcmd(int argc, char **argv)
 
 void getoptsreset(const char *value)
 {
-	shellparam.optind = number(value) ?: 1;
+	shellparam.optind = 1;
 	shellparam.optoff = -1;
 }
 
diff --git a/src/var.c b/src/var.c
index df432b5..b06b36c 100644
--- a/src/var.c
+++ b/src/var.c
@@ -93,7 +93,7 @@  struct var varinit[] = {
 	{ 0,	VSTRFIXED|VTEXTFIXED,		"PS1=$ ",	0 },
 	{ 0,	VSTRFIXED|VTEXTFIXED,		"PS2=> ",	0 },
 	{ 0,	VSTRFIXED|VTEXTFIXED,		"PS4=+ ",	0 },
-	{ 0,	VSTRFIXED|VTEXTFIXED,		defoptindvar,	getoptsreset },
+	{ 0,	VSTRFIXED|VTEXTFIXED|VNOFUNC,	defoptindvar,	getoptsreset },
 #ifdef WITH_LINENO
 	{ 0,	VSTRFIXED|VTEXTFIXED,		linenovar,	0 },
 #endif
@@ -535,7 +535,7 @@  poplocalvars(void)
 				ckfree(vp->text);
 			vp->flags = lvp->flags;
 			vp->text = lvp->text;
-			if (vp->func)
+			if (vp->func && !(vp->flags & VNOFUNC))
 				(*vp->func)(varnull(vp->text));
 		}
 		ckfree(lvp);