Division by zero

Division by zero

[Kristaps Dzonsons]

[-- Attachment #1: Type: text/plain, Size: 514 bytes --]

Hi,

Enclosed is a patch to handle roff.c's division by zero.  This was found 
(indirectly) when playing with mandoc's new support for inline equations.

groff(1) seems to actually warn us when this happens--if we're to do the 
same, we'll need to pass the mparse function through to the offending 
function.

By the way, upon an audit, I see that term_ps.c is vulnerable to 
division by zero if the "scale" parameter in termp_ps is zero.  However, 
we don't ever seem to re-set that, so it's ok.

Best,

Kristaps

[-- Attachment #2: divbyzero.patch --]
[-- Type: text/plain, Size: 1172 bytes --]

? .DS_Store
? .test.1.swp
? Makefile.local
? TEST.sh
? TEST.sh.out
? bar.1
? cgi-doc.diff
? cgi.h
? config.h
? config.log
? configure.local
? demandoc
? ditto.1
? divbyzero.patch
? eqn-test.1
? eqn-test.1.html
? eqn.2.patch
? eqn.bak.c
? eqn.patch
? foo
? foo.1
? foo.1.html
? foo.1.ps
? foo.2
? foo.2.html
? foo.3
? foo.3.html
? foo.4
? foo.4.html
? foo.5
? foo.5.html
? foo.5.ps
? foo.sh
? makewhatis
? man.cgi
? mandoc
? plockstat.1
? preconv
? term.diff
? test-dirent-namlen.dSYM
? test-fgetln.dSYM
? test-fts.dSYM
? test-getsubopt.dSYM
? test-mmap.dSYM
? test-sqlite3.dSYM
? test-strcasestr.dSYM
? test-strlcat.dSYM
? test-strlcpy.dSYM
? test-strptime.dSYM
? test-strsep.dSYM
? test-wchar.dSYM
? test.1
Index: roff.c
===================================================================
RCS file: /usr/vhosts/mdocml.bsd.lv/cvs/mdocml/roff.c,v
retrieving revision 1.231
diff -u -p -r1.231 roff.c
--- roff.c	16 Oct 2014 01:28:38 -0000	1.231
+++ roff.c	19 Oct 2014 20:54:47 -0000
@@ -1545,6 +1545,8 @@ roff_evalnum(const char *v, int *pos, in
 			*res *= operand2;
 			break;
 		case '/':
+			if (0.0 == operand2)
+				break;
 			*res /= operand2;
 			break;
 		case '%':

Re: Division by zero

[Ingo Schwarze]

Hi Kristaps,

Kristaps Dzonsons wrote on Sun, Oct 19, 2014 at 11:02:27PM +0200:

> Enclosed is a patch to handle roff.c's division by zero.

Good point.

> This was found (indirectly) when playing with mandoc's new support
> for inline equations.
> 
> groff(1) seems to actually warn us when this happens--if we're to do
> the same, we'll need to pass the mparse function through to the
> offending function.

Makes sense to me.  Perhaps just hand the struct roff down into
roff_evalcond, roff_evalnum, and roff_evalpar?  That one is
already a usual argument for many functions, while struct mparse
is not.

I think the message should be an ERROR, not a WARNING, because it
can have arbitrarily severe consequences on formatting and content.

Two more nits:  You say "x / 0 = x".  Hum, maybe.  But groff prefers
a different lie:  "x / 0 = 0".  And operator2 is int, not float.
So i think you need something like

	case '/':
		if (operand2 == 0) {
			scream("WOLF!");
			*res = 0;
		} else
			*res /= operand2;
		break;

> By the way, upon an audit, I see that term_ps.c is vulnerable to
> division by zero if the "scale" parameter in termp_ps is zero.
> However, we don't ever seem to re-set that, so it's ok.

Seems true.

Yours,
  Ingo
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv

Re: Division by zero

[Kristaps Dzonsons]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

Ingo,

> Makes sense to me.  Perhaps just hand the struct roff down into
> roff_evalcond, roff_evalnum, and roff_evalpar?  That one is
> already a usual argument for many functions, while struct mparse
> is not.

As enclosed!  It also needed "ln" for the line numbering.

> I think the message should be an ERROR, not a WARNING, because it
> can have arbitrarily severe consequences on formatting and content.
>
> Two more nits:  You say "x / 0 = x".  Hum, maybe.  But groff prefers
> a different lie:  "x / 0 = 0".  And operator2 is int, not float.
> So i think you need something like
>
> 	case '/':
> 		if (operand2 == 0) {
> 			scream("WOLF!");
> 			*res = 0;
> 		} else
> 			*res /= operand2;
> 		break;

...or we could just be a bit more realistic and let *res = INT_MAX, with 
sign depending on the numerator. ;)

(I use *res = 0 now.)

Best,

Kristaps

[-- Attachment #2: divbyzero2.patch --]
[-- Type: text/plain, Size: 5398 bytes --]

? .DS_Store
? Makefile.local
? TEST.sh
? TEST.sh.out
? bar.1
? cgi-doc.diff
? cgi.h
? config.h
? config.log
? configure.local
? demandoc
? ditto.1
? divbyzero.patch
? divbyzero2.patch
? eqn-test.1
? eqn-test.1.html
? eqn.2.patch
? eqn.bak.c
? eqn.patch
? foo
? foo.1
? foo.1.html
? foo.1.ps
? foo.2
? foo.2.html
? foo.3
? foo.3.html
? foo.4
? foo.4.html
? foo.5
? foo.5.html
? foo.5.ps
? foo.sh
? makewhatis
? man.cgi
? mandoc
? plockstat.1
? preconv
? term.diff
? test.1
Index: mandoc.h
===================================================================
RCS file: /usr/vhosts/mdocml.bsd.lv/cvs/mdocml/mandoc.h,v
retrieving revision 1.163
diff -u -p -r1.163 mandoc.h
--- mandoc.h	14 Oct 2014 02:16:06 -0000	1.163
+++ mandoc.h	20 Oct 2014 10:10:50 -0000
@@ -166,6 +166,7 @@ enum	mandocerr {
 	MANDOCERR_IT_NONUM, /* skipping request without numeric argument */
 	MANDOCERR_ARG_SKIP, /* skipping all arguments: macro args */
 	MANDOCERR_ARG_EXCESS, /* skipping excess arguments: macro ... args */
+	MANDOCERR_DIVZERO, /* divide by zero */
 
 	MANDOCERR_FATAL, /* ===== start of fatal errors ===== */
 
Index: read.c
===================================================================
RCS file: /usr/vhosts/mdocml.bsd.lv/cvs/mdocml/read.c,v
retrieving revision 1.91
diff -u -p -r1.91 read.c
--- read.c	18 Oct 2014 15:57:34 -0000	1.91
+++ read.c	20 Oct 2014 10:10:50 -0000
@@ -211,6 +211,7 @@ static	const char * const	mandocerrs[MAN
 	"skipping request without numeric argument",
 	"skipping all arguments",
 	"skipping excess arguments",
+	"divide by zero",
 
 	"generic fatal error",
 
Index: roff.c
===================================================================
RCS file: /usr/vhosts/mdocml.bsd.lv/cvs/mdocml/roff.c,v
retrieving revision 1.232
diff -u -p -r1.232 roff.c
--- roff.c	20 Oct 2014 02:33:06 -0000	1.232
+++ roff.c	20 Oct 2014 10:10:50 -0000
@@ -186,9 +186,12 @@ static	enum rofferr	 roff_cond_sub(ROFF_
 static	enum rofferr	 roff_ds(ROFF_ARGS);
 static	enum rofferr	 roff_eqndelim(struct roff *,
 				char **, size_t *, int);
-static	int		 roff_evalcond(const char *, int *);
-static	int		 roff_evalnum(const char *, int *, int *, int);
-static	int		 roff_evalpar(const char *, int *, int *);
+static	int		 roff_evalcond(struct roff *r, int,
+				const char *, int *);
+static	int		 roff_evalnum(struct roff *, int,
+				const char *, int *, int *, int);
+static	int		 roff_evalpar(struct roff *, int,
+				const char *, int *, int *);
 static	int		 roff_evalstrcond(const char *, int *);
 static	void		 roff_free1(struct roff *);
 static	void		 roff_freereg(struct roffreg *);
@@ -622,7 +625,7 @@ roff_res(struct roff *r, char **bufp, si
 		case 'B':
 			npos = 0;
 			ubuf[0] = arg_complete &&
-			    roff_evalnum(stnam, &npos, NULL, 0) &&
+			    roff_evalnum(r, ln, stnam, &npos, NULL, 0) &&
 			    stnam + npos + 1 == cp ? '1' : '0';
 			ubuf[1] = '\0';
 			break;
@@ -1240,7 +1243,7 @@ out:
  * or string condition.
  */
 static int
-roff_evalcond(const char *v, int *pos)
+roff_evalcond(struct roff *r, int ln, const char *v, int *pos)
 {
 	int	 wanttrue, number;
 
@@ -1271,7 +1274,7 @@ roff_evalcond(const char *v, int *pos)
 		break;
 	}
 
-	if (roff_evalnum(v, pos, &number, 0))
+	if (roff_evalnum(r, ln, v, pos, &number, 0))
 		return((number > 0) == wanttrue);
 	else
 		return(roff_evalstrcond(v, pos) == wanttrue);
@@ -1300,7 +1303,7 @@ roff_cond(ROFF_ARGS)
 
 	r->last->rule = ROFF_el == tok ?
 	    (r->rstackpos < 0 ? 0 : r->rstack[r->rstackpos--]) :
-	    roff_evalcond(*bufp, &pos);
+	    roff_evalcond(r, ln, *bufp, &pos);
 
 	/*
 	 * An if-else will put the NEGATION of the current evaluated
@@ -1466,14 +1469,15 @@ roff_getop(const char *v, int *pos, char
  * or a single signed integer number.
  */
 static int
-roff_evalpar(const char *v, int *pos, int *res)
+roff_evalpar(struct roff *r, int ln, 
+	const char *v, int *pos, int *res)
 {
 
 	if ('(' != v[*pos])
 		return(roff_getnum(v, pos, res));
 
 	(*pos)++;
-	if ( ! roff_evalnum(v, pos, res, 1))
+	if ( ! roff_evalnum(r, ln, v, pos, res, 1))
 		return(0);
 
 	/*
@@ -1495,7 +1499,8 @@ roff_evalpar(const char *v, int *pos, in
  * Proceed left to right, there is no concept of precedence.
  */
 static int
-roff_evalnum(const char *v, int *pos, int *res, int skipwhite)
+roff_evalnum(struct roff *r, int ln, const char *v, 
+	int *pos, int *res, int skipwhite)
 {
 	int		 mypos, operand2;
 	char		 operator;
@@ -1509,7 +1514,7 @@ roff_evalnum(const char *v, int *pos, in
 		while (isspace((unsigned char)v[*pos]))
 			(*pos)++;
 
-	if ( ! roff_evalpar(v, pos, res))
+	if ( ! roff_evalpar(r, ln, v, pos, res))
 		return(0);
 
 	while (1) {
@@ -1524,7 +1529,7 @@ roff_evalnum(const char *v, int *pos, in
 			while (isspace((unsigned char)v[*pos]))
 				(*pos)++;
 
-		if ( ! roff_evalpar(v, pos, &operand2))
+		if ( ! roff_evalpar(r, ln, v, pos, &operand2))
 			return(0);
 
 		if (skipwhite)
@@ -1545,6 +1550,12 @@ roff_evalnum(const char *v, int *pos, in
 			*res *= operand2;
 			break;
 		case '/':
+			if (0 == operand2) {
+				mandoc_msg(MANDOCERR_DIVZERO, 
+					r->parse, ln, *pos, v);
+				*res = 0;
+				break;
+			}
 			*res /= operand2;
 			break;
 		case '%':
@@ -1719,7 +1730,7 @@ roff_nr(ROFF_ARGS)
 	if ('+' == sign || '-' == sign)
 		val++;
 
-	if (roff_evalnum(val, NULL, &iv, 0))
+	if (roff_evalnum(r, ln, val, NULL, &iv, 0))
 		roff_setreg(r, key, iv, sign);
 
 	return(ROFF_IGN);

Re: Division by zero

[Ingo Schwarze]

Hi Kristaps,

Kristaps Dzonsons wrote on Mon, Oct 20, 2014 at 12:11:56PM +0200:

> As enclosed!  It also needed "ln" for the line numbering.

OK schwarze@.

> ...or we could just be a bit more realistic and let *res = INT_MAX,
> with sign depending on the numerator. ;)

That would require coordinating with groff, i guess.
I'm not sure it's worth changing.

> (I use *res = 0 now.)

Yes, that's the right way.  Get stuff done following traditional
behaviour (unless that is completely crazy), such that progress in
mandoc isn't obstructed by having to coordinate with groff.  If a
good reason arises later to change behaviour, that's a different
matter and can be dealt with slowly.

Thanks,
  Ingo


> Index: mandoc.h
> ===================================================================
> RCS file: /usr/vhosts/mdocml.bsd.lv/cvs/mdocml/mandoc.h,v
> retrieving revision 1.163
> diff -u -p -r1.163 mandoc.h
> --- mandoc.h	14 Oct 2014 02:16:06 -0000	1.163
> +++ mandoc.h	20 Oct 2014 10:10:50 -0000
> @@ -166,6 +166,7 @@ enum	mandocerr {
>  	MANDOCERR_IT_NONUM, /* skipping request without numeric argument */
>  	MANDOCERR_ARG_SKIP, /* skipping all arguments: macro args */
>  	MANDOCERR_ARG_EXCESS, /* skipping excess arguments: macro ... args */
> +	MANDOCERR_DIVZERO, /* divide by zero */
>  
>  	MANDOCERR_FATAL, /* ===== start of fatal errors ===== */
>  
> Index: read.c
> ===================================================================
> RCS file: /usr/vhosts/mdocml.bsd.lv/cvs/mdocml/read.c,v
> retrieving revision 1.91
> diff -u -p -r1.91 read.c
> --- read.c	18 Oct 2014 15:57:34 -0000	1.91
> +++ read.c	20 Oct 2014 10:10:50 -0000
> @@ -211,6 +211,7 @@ static	const char * const	mandocerrs[MAN
>  	"skipping request without numeric argument",
>  	"skipping all arguments",
>  	"skipping excess arguments",
> +	"divide by zero",
>  
>  	"generic fatal error",
>  
> Index: roff.c
> ===================================================================
> RCS file: /usr/vhosts/mdocml.bsd.lv/cvs/mdocml/roff.c,v
> retrieving revision 1.232
> diff -u -p -r1.232 roff.c
> --- roff.c	20 Oct 2014 02:33:06 -0000	1.232
> +++ roff.c	20 Oct 2014 10:10:50 -0000
> @@ -186,9 +186,12 @@ static	enum rofferr	 roff_cond_sub(ROFF_
>  static	enum rofferr	 roff_ds(ROFF_ARGS);
>  static	enum rofferr	 roff_eqndelim(struct roff *,
>  				char **, size_t *, int);
> -static	int		 roff_evalcond(const char *, int *);
> -static	int		 roff_evalnum(const char *, int *, int *, int);
> -static	int		 roff_evalpar(const char *, int *, int *);
> +static	int		 roff_evalcond(struct roff *r, int,
> +				const char *, int *);
> +static	int		 roff_evalnum(struct roff *, int,
> +				const char *, int *, int *, int);
> +static	int		 roff_evalpar(struct roff *, int,
> +				const char *, int *, int *);
>  static	int		 roff_evalstrcond(const char *, int *);
>  static	void		 roff_free1(struct roff *);
>  static	void		 roff_freereg(struct roffreg *);
> @@ -622,7 +625,7 @@ roff_res(struct roff *r, char **bufp, si
>  		case 'B':
>  			npos = 0;
>  			ubuf[0] = arg_complete &&
> -			    roff_evalnum(stnam, &npos, NULL, 0) &&
> +			    roff_evalnum(r, ln, stnam, &npos, NULL, 0) &&
>  			    stnam + npos + 1 == cp ? '1' : '0';
>  			ubuf[1] = '\0';
>  			break;
> @@ -1240,7 +1243,7 @@ out:
>   * or string condition.
>   */
>  static int
> -roff_evalcond(const char *v, int *pos)
> +roff_evalcond(struct roff *r, int ln, const char *v, int *pos)
>  {
>  	int	 wanttrue, number;
>  
> @@ -1271,7 +1274,7 @@ roff_evalcond(const char *v, int *pos)
>  		break;
>  	}
>  
> -	if (roff_evalnum(v, pos, &number, 0))
> +	if (roff_evalnum(r, ln, v, pos, &number, 0))
>  		return((number > 0) == wanttrue);
>  	else
>  		return(roff_evalstrcond(v, pos) == wanttrue);
> @@ -1300,7 +1303,7 @@ roff_cond(ROFF_ARGS)
>  
>  	r->last->rule = ROFF_el == tok ?
>  	    (r->rstackpos < 0 ? 0 : r->rstack[r->rstackpos--]) :
> -	    roff_evalcond(*bufp, &pos);
> +	    roff_evalcond(r, ln, *bufp, &pos);
>  
>  	/*
>  	 * An if-else will put the NEGATION of the current evaluated
> @@ -1466,14 +1469,15 @@ roff_getop(const char *v, int *pos, char
>   * or a single signed integer number.
>   */
>  static int
> -roff_evalpar(const char *v, int *pos, int *res)
> +roff_evalpar(struct roff *r, int ln, 
> +	const char *v, int *pos, int *res)
>  {
>  
>  	if ('(' != v[*pos])
>  		return(roff_getnum(v, pos, res));
>  
>  	(*pos)++;
> -	if ( ! roff_evalnum(v, pos, res, 1))
> +	if ( ! roff_evalnum(r, ln, v, pos, res, 1))
>  		return(0);
>  
>  	/*
> @@ -1495,7 +1499,8 @@ roff_evalpar(const char *v, int *pos, in
>   * Proceed left to right, there is no concept of precedence.
>   */
>  static int
> -roff_evalnum(const char *v, int *pos, int *res, int skipwhite)
> +roff_evalnum(struct roff *r, int ln, const char *v, 
> +	int *pos, int *res, int skipwhite)
>  {
>  	int		 mypos, operand2;
>  	char		 operator;
> @@ -1509,7 +1514,7 @@ roff_evalnum(const char *v, int *pos, in
>  		while (isspace((unsigned char)v[*pos]))
>  			(*pos)++;
>  
> -	if ( ! roff_evalpar(v, pos, res))
> +	if ( ! roff_evalpar(r, ln, v, pos, res))
>  		return(0);
>  
>  	while (1) {
> @@ -1524,7 +1529,7 @@ roff_evalnum(const char *v, int *pos, in
>  			while (isspace((unsigned char)v[*pos]))
>  				(*pos)++;
>  
> -		if ( ! roff_evalpar(v, pos, &operand2))
> +		if ( ! roff_evalpar(r, ln, v, pos, &operand2))
>  			return(0);
>  
>  		if (skipwhite)
> @@ -1545,6 +1550,12 @@ roff_evalnum(const char *v, int *pos, in
>  			*res *= operand2;
>  			break;
>  		case '/':
> +			if (0 == operand2) {
> +				mandoc_msg(MANDOCERR_DIVZERO, 
> +					r->parse, ln, *pos, v);
> +				*res = 0;
> +				break;
> +			}
>  			*res /= operand2;
>  			break;
>  		case '%':
> @@ -1719,7 +1730,7 @@ roff_nr(ROFF_ARGS)
>  	if ('+' == sign || '-' == sign)
>  		val++;
>  
> -	if (roff_evalnum(val, NULL, &iv, 0))
> +	if (roff_evalnum(r, ln, val, NULL, &iv, 0))
>  		roff_setreg(r, key, iv, sign);
>  
>  	return(ROFF_IGN);
--
 To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv