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

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

[Sven Wischnowsky]

Bart Schaefer wrote:

> ...
> 
> 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?

It doesn't use YYERROR* because it has to return NULL and we have
YYERRORs only for void and integer, I think (I didn't change that
part).

And we don't need to reset ecused there, it's one of the top-level
functions that will set up their environments with init_parse() the
next time round.

Your patch looks good, as far as I can see.

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

Because ZTST_execchunk() uses eval to execute the test code and
bin_eval() uses parse_string() with a last argument of zero to turn of 
line numbering. Hm, the fix would be to add a global variable where
bin_eval()/parse_string() (and other functions?) can store the old
line number and the error functions use (lineno < 0 ? savedlineno : lineno)
or something like that, right?

Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

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

[Bart Schaefer]

On May 29, 10:27am, Sven Wischnowsky wrote:
} Subject: Re: PATCH (!): Re: PATCH: (very) bad syntax error checking
}
} Bart Schaefer wrote:
} 
} > 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?
} 
} It doesn't use YYERROR* because it has to return NULL and we have
} YYERRORs only for void and integer, I think (I didn't change that
} part).

Zero is NULL, and NULL is zero.  Using YYERROR(0) would return NULL.

But perhaps a better question is: `tok' is a global, right?  Why not put
the `tok = LEXERROR' into yyerror() itself?

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

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

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

[Sven Wischnowsky]

Bart Schaefer wrote:

> On May 29, 10:27am, Sven Wischnowsky wrote:
> } Subject: Re: PATCH (!): Re: PATCH: (very) bad syntax error checking
> }
> } Bart Schaefer wrote:
> } 
> } > 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?
> } 
> } It doesn't use YYERROR* because it has to return NULL and we have
> } YYERRORs only for void and integer, I think (I didn't change that
> } part).
> 
> Zero is NULL, and NULL is zero.  Using YYERROR(0) would return NULL.

But we are quite picky about distinguishing between the two.

> But perhaps a better question is: `tok' is a global, right? 

Yes.

> Why not put
> the `tok = LEXERROR' into yyerror() itself?

Good question. Haven't looked yet...

Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de