| Message ID | 20221214023130.u7pn4ca6ma4kuxot@tarta.nabijaczleweli.xyz (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | options: don't error when unsetting OPTIND | expand |
While we're thinking about it, is this the behavior we want? Or should `readonly` produce an error? ``` $ getopts abc ARG -ab -c foo $ echo $OPTIND 2 $ echo $ARG a $ readonly OPTIND $ getopts abc ARG -ab -c foo /Users/mgree/smoosh/libdash/src/dash: 5: getopts: OPTIND: is read only $ echo $ARG a $ echo $OPTIND 2 ``` There are similar questions for PWD and other shell-set nameable variables: ``` $ readonly PWD $ pwd /Users/mgree/pash-medium/budgeting $ cd .. /Users/mgree/smoosh/libdash/src/dash: 13: cd: PWD: is read only $ pwd /Users/mgree/pash-medium ``` But special parameters (like @ and ?) are outright rejected by `readonly`. Bafflingly, the POSIX spec gives the example `readonly HOME PWD`, but with no explanation about why one might want to do that. (I get making `HOME` readonly, but `PWD`? Is that supposed to stop `cd` from working?) Cheers, Michael On 2022-12-14 at 03:31:30 AM, наб wrote: > unset OPTIND ends up calling getoptsreset("") which errors out with > sh: 1: unset: Illegal number: > or even > sh: 1: unset: Illegal number: leweli/bin:/usr/l�� > > Pass the current flags to struct var->func, set the getopts optind to 1 > and continue with allowing the unset. > > We still forbid OPTIND=, OPTIND=-1, OPTIND=abc, &c. > > Fixes: https://bugs.debian.org/985478 > --- > src/exec.c | 2 +- > src/exec.h | 2 +- > src/mail.c | 2 +- > src/mail.h | 2 +- > src/options.c | 5 ++--- > src/options.h | 2 +- > src/var.c | 4 ++-- > src/var.h | 2 +- > 8 files changed, 10 insertions(+), 11 deletions(-) > > diff --git a/src/exec.c b/src/exec.c > index 87354d4..68fa8ab 100644 > --- a/src/exec.c > +++ b/src/exec.c > @@ -565,7 +565,7 @@ hashcd(void) > */ > > void > -changepath(const char *newval) > +changepath(const char *newval, int unused) > { > const char *new; > int idx; > diff --git a/src/exec.h b/src/exec.h > index 423b07e..0f74be4 100644 > --- a/src/exec.h > +++ b/src/exec.h > @@ -69,7 +69,7 @@ int hashcmd(int, char **); > void find_command(char *, struct cmdentry *, int, const char *); > struct builtincmd *find_builtin(const char *); > void hashcd(void); > -void changepath(const char *); > +void changepath(const char *, int); > #ifdef notdef > void getcmdentry(char *, struct cmdentry *); > #endif > diff --git a/src/mail.c b/src/mail.c > index 8eacb2d..e81d2b4 100644 > --- a/src/mail.c > +++ b/src/mail.c > @@ -109,7 +109,7 @@ chkmail(void) > > > void > -changemail(const char *val) > +changemail(const char *val, int unused) > { > changed++; > } > diff --git a/src/mail.h b/src/mail.h > index 3c6b21d..70b54a4 100644 > --- a/src/mail.h > +++ b/src/mail.h > @@ -35,4 +35,4 @@ > */ > > void chkmail(void); > -void changemail(const char *); > +void changemail(const char *, int); > diff --git a/src/options.c b/src/options.c > index a46c23b..81f2c4b 100644 > --- a/src/options.c > +++ b/src/options.c > @@ -390,10 +390,9 @@ setcmd(int argc, char **argv) > > > void > -getoptsreset(value) > - const char *value; > +getoptsreset(const char *value, int flags) > { > - shellparam.optind = number(value) ?: 1; > + shellparam.optind = (flags & VUNSET) ? 1 : number(value) ?: 1; > shellparam.optoff = -1; > } > > diff --git a/src/options.h b/src/options.h > index 975fe33..10bcb88 100644 > --- a/src/options.h > +++ b/src/options.h > @@ -83,4 +83,4 @@ int shiftcmd(int, char **); > int setcmd(int, char **); > int getoptscmd(int, char **); > int nextopt(const char *); > -void getoptsreset(const char *); > +void getoptsreset(const char *, int); > diff --git a/src/var.c b/src/var.c > index ef9c2bd..a7d4a92 100644 > --- a/src/var.c > +++ b/src/var.c > @@ -266,7 +266,7 @@ struct var *setvareq(char *s, int flags) > goto out; > > if (vp->func && (flags & VNOFUNC) == 0) > - (*vp->func)(strchrnul(s, '=') + 1); > + (*vp->func)(strchrnul(s, '=') + 1, flags); > > if ((vp->flags & (VTEXTFIXED|VSTACK)) == 0) > ckfree(vp->text); > @@ -531,7 +531,7 @@ poplocalvars(void) > unsetvar(vp->text); > } else { > if (vp->func) > - (*vp->func)(strchrnul(lvp->text, '=') + 1); > + (*vp->func)(strchrnul(lvp->text, '=') + 1, lvp->flags); > if ((vp->flags & (VTEXTFIXED|VSTACK)) == 0) > ckfree(vp->text); > vp->flags = lvp->flags; > diff --git a/src/var.h b/src/var.h > index aa7575a..4329e22 100644 > --- a/src/var.h > +++ b/src/var.h > @@ -56,7 +56,7 @@ struct var { > struct var *next; /* next entry in hash list */ > int flags; /* flags are defined above */ > const char *text; /* name=value */ > - void (*func)(const char *); > + void (*func)(const char *, int flags); > /* function to be called when */ > /* the variable gets set/unset */ > }; > -- > 2.30.2
Michael Greenberg wrote in <m2edt2nk5q.fsf@greenberg.science>: ... |Bafflingly, the POSIX spec gives the example `readonly HOME PWD`, but |with no explanation about why one might want to do that. (I get making |`HOME` readonly, but `PWD`? Is that supposed to stop `cd` from working?) PWD is a beast with lots of "behaviors of the cd and pwd utilities are unspecified" in case of user edits, see "2.5.3 Shell Variables". --steffen | |Der Kragenbaer, The moon bear, |der holt sich munter he cheerfully and one by one |einen nach dem anderen runter wa.ks himself off |(By Robert Gernhardt)
On 2022-12-14 at 06:49:46 PM, Steffen Nurpmeso wrote: > Michael Greenberg wrote in > <m2edt2nk5q.fsf@greenberg.science>: > ... > |Bafflingly, the POSIX spec gives the example `readonly HOME PWD`, but > |with no explanation about why one might want to do that. (I get making > |`HOME` readonly, but `PWD`? Is that supposed to stop `cd` from working?) > > PWD is a beast with lots of "behaviors of the cd and pwd utilities > are unspecified" in case of user edits, see "2.5.3 Shell Variables". Is running `readonly PWD` the same as setting `PWD`? The language in the documentation for `cd` and in 2.5.3 only talks about setting or unsetting `PWD`, but not the export/readonly bits.
Hi! On Wed, Dec 14, 2022 at 05:36:22PM -0500, Michael Greenberg wrote: > On 2022-12-14 at 06:49:46 PM, Steffen Nurpmeso wrote: > > > Michael Greenberg wrote in > > <m2edt2nk5q.fsf@greenberg.science>: > > ... > > |Bafflingly, the POSIX spec gives the example `readonly HOME PWD`, but > > |with no explanation about why one might want to do that. (I get making > > |`HOME` readonly, but `PWD`? Is that supposed to stop `cd` from working?) > > > > PWD is a beast with lots of "behaviors of the cd and pwd utilities > > are unspecified" in case of user edits, see "2.5.3 Shell Variables". > > Is running `readonly PWD` the same as setting `PWD`? > > The language in the documentation for `cd` and in 2.5.3 only talks about setting or > unsetting `PWD`, but not the export/readonly bits. Isssue 8 Draft 2.1, XBD, 8.1 Environment Variable Definition para. 11 (p. 156, ll. 5381-5390) says: Additionally, a subset of the above variables are manipulated by shell built-in utilities outside of shell assignments. If an attempt is made to mark any of the following variables as readonly, then either the readonly utility shall reject the attempt, or readonly shall succeed but the shell can still modify the variables outside of assignment context, or readonly shall succeed but use of a shell built-in that would otherwise modify such a variable shall fail. LINENO OLDPWD OPTARG OPTIND PWD I.e., readonly PWD; cd /; PWD=dupa must: * fail in readonly, or * fail in the cd, or * fail in the assignment at implementer's choice. Which is just a long-winded way of saying "just do whatever but only accept readonly if it does anything". Dash appears to do option 2: $ ./dash -c 'readonly PWD; cd /' ./dash: 1: cd: PWD: is read only $ ./dash -c 'readonly OPTIND; getopts a a -a' ./dash: 1: getopts: OPTIND: is read only which is fine. OLDPWD behaves like PWD and OPTARG like OPTIND. LINENO behaves like option 3 for obvious reasons. (For completeness: in ibid., para. 12 (ll. 5391-5392) there's a provision for special treatment for any non-standard variable we want for our built-ins: Implementations may provide an implementation-defined set of additional variables which are manipulated by implementation-specific built-in utilities not defined in this standard. The readonly utility shall not reject marking these additional variables as readonly, but when marked readonly, those extension utilities shall either continue to modify the variables, or shall fail because the variable is readonly. None of the variables defined by this standard shall be in this implementation-defined set.) Best, наб
On 2022-12-15 at 12:07:38 AM, наб wrote: > Hi! > > On Wed, Dec 14, 2022 at 05:36:22PM -0500, Michael Greenberg wrote: >> On 2022-12-14 at 06:49:46 PM, Steffen Nurpmeso wrote: >> >> > Michael Greenberg wrote in >> > <m2edt2nk5q.fsf@greenberg.science>: >> > ... >> > |Bafflingly, the POSIX spec gives the example `readonly HOME PWD`, but >> > |with no explanation about why one might want to do that. (I get making >> > |`HOME` readonly, but `PWD`? Is that supposed to stop `cd` from working?) >> > >> > PWD is a beast with lots of "behaviors of the cd and pwd utilities >> > are unspecified" in case of user edits, see "2.5.3 Shell Variables". >> >> Is running `readonly PWD` the same as setting `PWD`? >> >> The language in the documentation for `cd` and in 2.5.3 only talks about setting or >> unsetting `PWD`, but not the export/readonly bits. > > Isssue 8 Draft 2.1, XBD, 8.1 Environment Variable Definition para. 11 > (p. 156, ll. 5381-5390) says: Thank you for finding this! I didn't think to check the current draft. Cheers, Michael
diff --git a/src/exec.c b/src/exec.c index 87354d4..68fa8ab 100644 --- a/src/exec.c +++ b/src/exec.c @@ -565,7 +565,7 @@ hashcd(void) */ void -changepath(const char *newval) +changepath(const char *newval, int unused) { const char *new; int idx; diff --git a/src/exec.h b/src/exec.h index 423b07e..0f74be4 100644 --- a/src/exec.h +++ b/src/exec.h @@ -69,7 +69,7 @@ int hashcmd(int, char **); void find_command(char *, struct cmdentry *, int, const char *); struct builtincmd *find_builtin(const char *); void hashcd(void); -void changepath(const char *); +void changepath(const char *, int); #ifdef notdef void getcmdentry(char *, struct cmdentry *); #endif diff --git a/src/mail.c b/src/mail.c index 8eacb2d..e81d2b4 100644 --- a/src/mail.c +++ b/src/mail.c @@ -109,7 +109,7 @@ chkmail(void) void -changemail(const char *val) +changemail(const char *val, int unused) { changed++; } diff --git a/src/mail.h b/src/mail.h index 3c6b21d..70b54a4 100644 --- a/src/mail.h +++ b/src/mail.h @@ -35,4 +35,4 @@ */ void chkmail(void); -void changemail(const char *); +void changemail(const char *, int); diff --git a/src/options.c b/src/options.c index a46c23b..81f2c4b 100644 --- a/src/options.c +++ b/src/options.c @@ -390,10 +390,9 @@ setcmd(int argc, char **argv) void -getoptsreset(value) - const char *value; +getoptsreset(const char *value, int flags) { - shellparam.optind = number(value) ?: 1; + shellparam.optind = (flags & VUNSET) ? 1 : number(value) ?: 1; shellparam.optoff = -1; } diff --git a/src/options.h b/src/options.h index 975fe33..10bcb88 100644 --- a/src/options.h +++ b/src/options.h @@ -83,4 +83,4 @@ int shiftcmd(int, char **); int setcmd(int, char **); int getoptscmd(int, char **); int nextopt(const char *); -void getoptsreset(const char *); +void getoptsreset(const char *, int); diff --git a/src/var.c b/src/var.c index ef9c2bd..a7d4a92 100644 --- a/src/var.c +++ b/src/var.c @@ -266,7 +266,7 @@ struct var *setvareq(char *s, int flags) goto out; if (vp->func && (flags & VNOFUNC) == 0) - (*vp->func)(strchrnul(s, '=') + 1); + (*vp->func)(strchrnul(s, '=') + 1, flags); if ((vp->flags & (VTEXTFIXED|VSTACK)) == 0) ckfree(vp->text); @@ -531,7 +531,7 @@ poplocalvars(void) unsetvar(vp->text); } else { if (vp->func) - (*vp->func)(strchrnul(lvp->text, '=') + 1); + (*vp->func)(strchrnul(lvp->text, '=') + 1, lvp->flags); if ((vp->flags & (VTEXTFIXED|VSTACK)) == 0) ckfree(vp->text); vp->flags = lvp->flags; diff --git a/src/var.h b/src/var.h index aa7575a..4329e22 100644 --- a/src/var.h +++ b/src/var.h @@ -56,7 +56,7 @@ struct var { struct var *next; /* next entry in hash list */ int flags; /* flags are defined above */ const char *text; /* name=value */ - void (*func)(const char *); + void (*func)(const char *, int flags); /* function to be called when */ /* the variable gets set/unset */ };
unset OPTIND ends up calling getoptsreset("") which errors out with sh: 1: unset: Illegal number: or even sh: 1: unset: Illegal number: leweli/bin:/usr/l�� Pass the current flags to struct var->func, set the getopts optind to 1 and continue with allowing the unset. We still forbid OPTIND=, OPTIND=-1, OPTIND=abc, &c. Fixes: https://bugs.debian.org/985478 --- src/exec.c | 2 +- src/exec.h | 2 +- src/mail.c | 2 +- src/mail.h | 2 +- src/options.c | 5 ++--- src/options.h | 2 +- src/var.c | 4 ++-- src/var.h | 2 +- 8 files changed, 10 insertions(+), 11 deletions(-)