PATCH: (very) bad syntax error checking

PATCH: (very) bad syntax error checking

[Peter Stephenson]

It's rather late in the day to come across problems like this.


% cat test.zsh
if true; # then missing
  print hello
fi

print This line should not be executed.
% zsh -f ./test.zsh
./test.zsh:3: parse error near `fi'
This line should not be executed.


Yuk.  This is the way people accidentally delete all the files on their
disks, etc.  I can't believe this has always been there --- I suspect it
must be part of the parsing changes that we've somehow all missed.  I won't
be releasing anything until this gets fixed.  If it's a lot of code to fix
(I would guess it's just an errflag test or two) I'll hold off for another
week.

-- 
Peter Stephenson <pws@cambridgesiliconradio.com>
Cambridge Silicon Radio, Unit 300, Science Park, Milton Road,
Cambridge, CB4 0XL, UK                          Tel: +44 (0)1223 392070

Re: PATCH: (very) bad syntax error checking

[Bart Schaefer]

On May 26,  3:24pm, Peter Stephenson wrote:
} Subject: PATCH: (very) bad syntax error checking

Patch?

} It's rather late in the day to come across problems like this.
} 
} % zsh -f ./test.zsh
} ./test.zsh:3: parse error near `fi'
} This line should not be executed.
} 
} Yuk.  This is the way people accidentally delete all the files on their
} disks, etc.  I can't believe this has always been there --- I suspect it
} must be part of the parsing changes that we've somehow all missed.

zagzig[233] ../zsh-2.4/src/zsh -f /tmp/foo/test.zsh
/tmp/foo/test.zsh: parse error near `fi' [3]
This line should not be executed.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

PATCH (!): Re: PATCH: (very) bad syntax error checking

[Bart Schaefer]

On May 26,  3:24pm, Peter Stephenson wrote:
} Subject: PATCH: (very) bad syntax error checking
}
} It's rather late in the day to come across problems like this.
} 
} % zsh -f ./test.zsh
} ./test.zsh:3: parse error near `fi'
} This line should not be executed.

This was supposedly fixed, by PWS, in zsh-workers/4191, before the 3.1.5
release; though Zefram changed it slightly, according to the ChangeLog.

As it happens, one of the customizations I have in my local copy of 3.1.6
is the patch from zsh-workers/4196, which I've been stubbornly hanging on
to ever since Zefram chose not to include it in 3.1.5, so I tried it with
ARGV0=ksh:

zagzig[274] Src/zsh -f /tmp/foo/test.zsh
/tmp/foo/test.zsh:3: parse error near `fi'
This line should not be executed.
zagzig[275] ARGV0=ksh Src/zsh -f /tmp/foo/test.zsh
/tmp/foo/test.zsh:3: parse error near `fi'
zagzig[276] 

That would seem to mean the problem is with the value of `noerrexit' around
line 123 of init.c, but in fact that's NOT the case.  Here again is the
shell code in question:

    if true; # then missing
      print hello
    fi

Remove the first two lines; then:

zagzig[277] Src/zsh -f /tmp/foo/test.zsh
/tmp/foo/test.zsh:1: parse error near `fi'
This line should not be executed.
zagzig[278] ARGV0=ksh Src/zsh -f /tmp/foo/test.zsh
/tmp/foo/test.zsh:1: parse error near `fi'
This line should not be executed.

So the problem is twofold; first, there appears to be some very old cruft
in par_if() in parse.c left over from when the SHORT_LOOPS option affected
the bodies of "if" statements.  This causes the ksh/zsh difference I first
noticed.  Unfortunately, I don't know how to remove it without breaking
still-valid csh-compat syntax like `if (true) print hello'.  It probably
doesn't need to be removed after fixing the next problem.

Second, encountering the token "fi" alone on a line is not setting the
returned token to LEXERR, so in init.c the test of (tok == LEXERR) fails
(the value of `tok' is, not surpisingly, `FI').

What follows appears to fix it, but Sven should confirm.  For one thing,
I wonder why parse_list() doesn't use YYERROR() ... is there some reason
why `ecused' should not be set to 0 in that specific case?

I included a hunk for the grammar test, but why is the line number `-1'?

(The init.c hunk is just some whitespace cleanup.)

Index: Src/parse.c
===================================================================
@@ -459,6 +459,7 @@
 	}
     }
     if (!r) {
+	tok = LEXERR;
 	if (errflag) {
 	    yyerror(0);
 	    ecused--;
@@ -491,10 +492,8 @@
     yylex();
     init_parse();
     par_list(&c);
-#if 0 
-   if (tok == LEXERR)
-#endif
-   if (tok != ENDINPUT) {
+    if (tok != ENDINPUT) {
+	tok = LEXERR;
 	yyerror(0);
 	return NULL;
     }
Index: Src/init.c
===================================================================
@@ -944,7 +944,7 @@
 	execode(prog, 1, 0);
 	popheap();
     } else
-	loop(0, 0);		     /* loop through the file to be sourced        */
+	loop(0, 0);		     /* loop through the file to be sourced  */
     sourcelevel--;
 
     /* restore the current shell state */
Index: Test/01grammar.ztst
===================================================================
@@ -105,6 +105,11 @@
   fi
 0:`if ...' (iii)
 >false
+  if true;
+    :
+  fi
+1d:`if ...' (iv)
+?ZTST_execchunk:-1: parse error near `fi'
 
   for name in word to term; do
     print $name

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: PATCH (!): Re: PATCH: (very) bad syntax error checking

[Peter Stephenson]

"Bart Schaefer" wrote:
> } % zsh -f ./test.zsh
> } ./test.zsh:3: parse error near `fi'
> } This line should not be executed.
> 
> This was supposedly fixed, by PWS, in zsh-workers/4191, before the 3.1.5
> release; though Zefram changed it slightly, according to the ChangeLog.
>...
> What follows appears to fix it, but Sven should confirm.

(I don't know why I originally put patch in the subject line.  Force of
habit, I suppose.)

Seems the whole thing was older than I remembered, so maybe I shouldn't
have worried so much, but it still strikes me as rather poor form.
I tried this on bash (2.03.19(1)-release)...

ztst.zsh: line 3: syntax error near unexpected token `fi'
ztst.zsh: line 3: `fi'

so it obviously takes this very seriously :-)

I'll wait to see if Sven has any remarks and if not make the release on
Monday or Tuesday (with the new header problem to worry about).  It will no
doubt become known as the `Late Spring Bank Holiday Edition'.  Not.

A propos of the release... there may be a delay with releasing it on
Sourceforge, since I haven't yet studied how to do that, but that doesn't
seem particularly serious since we have a good network of mirrors already.
However, I'm keen for releases to be posted everywhere relevant, which
these days means free software web sites rather more than Usenet, for example
Freshmeat.  If anyone knows any such places, they should feel free to go
ahead and post information (after the release, obviously), or I'm willing
to collect information and keep it for future maintainers.

-- 
Peter Stephenson <pws@pwstephenson.fsnet.co.uk>
Work: pws@CambridgeSiliconRadio.com
Web: http://www.pwstephenson.fsnet.co.uk