On Sun, Apr 22, 2018 at 04:31:08AM +0200, Klemens Nanni wrote:
> While looking at `time' for other things, tb@ pointed out that the
> following code is broken:
>
> $ time for i in 1 2; do echo $i; done
> ksh: : is read only
>
> Note the missing variable name: time's pipeline fails since for's
> identifier gets mangled to an empty string by get_command() at
> sync.c:368 which in turn causes global() at var.c:177/194 to set it
> RDONLY.
>
> This regression was introduced by
> https://marc.info/?l=openbsd-tech&m=121613951723808 and made it's way
> into 4.4 as
>
> revision 1.28
> date: 2008/07/23 16:34:38; author: jaredy; state: Exp; lines: +6 -1;
> fix stack abuse in the `time' commmand, using alloc()'d memory instead.
>
> reported by Thorsten Glaser, thanks.
>
> ok millert@, earlier version miod@
>
> The following diff fixes `time for ...' by copying the identifier over
> to the new stack if given.
>
> regress/bin/ksh still passes on amd64, but I'll add a new one for this
> once we have a final fix.
>
> Feedback?
Thanks for tracking this down, your fix looks correct to me.
ok
>
> Index: syn.c
> ===================================================================
> RCS file: /cvs/src/bin/ksh/syn.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 syn.c
> --- syn.c 30 Dec 2015 09:07:00 -0000 1.38
> +++ syn.c 22 Apr 2018 02:17:33 -0000
> @@ -365,9 +365,13 @@ get_command(int cf)
> syniocf &= ~(KEYWORD|ALIAS);
> t = pipeline(0);
> if (t) {
> - t->str = alloc(2, ATEMP);
> - t->str[0] = '\0'; /* TF_* flags */
> - t->str[1] = '\0';
> + if (t->str) {
> + t->str = str_save(t->str, ATEMP);
> + } else {
> + t->str = alloc(2, ATEMP);
> + t->str[0] = '\0'; /* TF_* flags */
> + t->str[1] = '\0';
> + }
> }
> t = block(TTIME, t, NULL, NULL);
> break;
>
