PATCH: Don't crash when math recursion limit is exceeded

PATCH: Don't crash when math recursion limit is exceeded

[Mikael Magnusson]

This used to never trigger for me when compiling in debug mode, but I
finally had better luck today and tracked it down.

*ep seems to already be NULL, but I couldn't quite figure out where it
was set to NULL so I figured it can't hurt to make it explicitly NULL
at that point. I suppose setting *ep = ""; would also work (without the
second hunk)?

---
 Src/math.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Src/math.c b/Src/math.c
index f8a4eef..43cede9 100644
--- a/Src/math.c
+++ b/Src/math.c
@@ -362,6 +362,7 @@ mathevall(char *s, enum prec_type prec_tp, char **ep)
     if (mlevel >= MAX_MLEVEL) {
 	xyyval.type = MN_INTEGER;
 	xyyval.u.l = 0;
+	*ep = NULL;
 
 	zerr("math recursion limit exceeded");
 
@@ -1352,7 +1353,7 @@ matheval(char *s)
     }
     x = mathevall(s, MPREC_TOP, &junk);
     mtok = xmtok;
-    if (*junk)
+    if (junk && *junk)
 	zerr("bad math expression: illegal character: %c", *junk);
     return x;
 }
-- 
1.7.10.GIT

Re: PATCH: Don't crash when math recursion limit is exceeded

[Peter Stephenson]

On Sun, 10 Mar 2013 13:06:37 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> This used to never trigger for me when compiling in debug mode, but I
> finally had better luck today and tracked it down.
> 
> *ep seems to already be NULL, but I couldn't quite figure out where it
> was set to NULL so I figured it can't hurt to make it explicitly NULL
> at that point. I suppose setting *ep = ""; would also work (without the
> second hunk)?

Another way of doing it would probably be to set "*ep = s", which is
consistent with what *ep is supposed to be doing.

If you set it to NULL it's probably safest to change the value returned
by the other calls of mathevall() around line 960 pre-patch, which also
outputs the "bad math expression" error.  I don't see why they shouldn't
be fully consistent; zerr() already tests for errflag, so they shouldn't
need to test for that, but either neither or both should.

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/


 
> ---
>  Src/math.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Src/math.c b/Src/math.c
> index f8a4eef..43cede9 100644
> --- a/Src/math.c
> +++ b/Src/math.c
> @@ -362,6 +362,7 @@ mathevall(char *s, enum prec_type prec_tp, char **ep)
>      if (mlevel >= MAX_MLEVEL) {
>  	xyyval.type = MN_INTEGER;
>  	xyyval.u.l = 0;
> +	*ep = NULL;
>  
>  	zerr("math recursion limit exceeded");
>  
> @@ -1352,7 +1353,7 @@ matheval(char *s)
>      }
>      x = mathevall(s, MPREC_TOP, &junk);
>      mtok = xmtok;
> -    if (*junk)
> +    if (junk && *junk)
>  	zerr("bad math expression: illegal character: %c", *junk);
>      return x;
>  }
> -- 
> 1.7.10.GIT

Re: PATCH: Don't crash when math recursion limit is exceeded

[Mikael Magnusson]

On 10 March 2013 19:42, Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> On Sun, 10 Mar 2013 13:06:37 +0100
> Mikael Magnusson <mikachu@gmail.com> wrote:
>> This used to never trigger for me when compiling in debug mode, but I
>> finally had better luck today and tracked it down.
>>
>> *ep seems to already be NULL, but I couldn't quite figure out where it
>> was set to NULL so I figured it can't hurt to make it explicitly NULL
>> at that point. I suppose setting *ep = ""; would also work (without the
>> second hunk)?
>
> Another way of doing it would probably be to set "*ep = s", which is
> consistent with what *ep is supposed to be doing.
>
> If you set it to NULL it's probably safest to change the value returned
> by the other calls of mathevall() around line 960 pre-patch, which also
> outputs the "bad math expression" error.  I don't see why they shouldn't
> be fully consistent; zerr() already tests for errflag, so they shouldn't
> need to test for that, but either neither or both should.

Ah, I didn't even think to check for other users... I don't have a
good understanding of how zerr and errflag is meant to work (the
definition has a comment "error/break flag"), but it seems like
setting *ep=s is safer, yeah. (I'm a bit confused why this doesn't
cause the 'illegal character' warning to print, but this is probably
related to not knowing about zerr/errflag.)

Somewhat relatedly, bash prints the name of the token that caused the
recursion, and we could do this quite easily like this, I think;

@@ -362,9 +362,9 @@
     if (mlevel >= MAX_MLEVEL) {
        xyyval.type = MN_INTEGER;
        xyyval.u.l = 0;
+       *ep = s;

-       zerr("math recursion limit exceeded");
+       zerr("math recursion limit exceeded: %s", *ep);

        return xyyval;
     }

% bash -c 'foo=foo;((5 + foo + 7))'
bash: ((: foo: expression recursion level exceeded (error token is "foo")
% Src/zsh -c 'foo=foo;((5 + foo + 7))'
zsh:1: math recursion limit exceeded: foo

But maybe I'm getting carried away.

-- 
Mikael Magnusson