* Division by zero
@ 2014-10-19 21:02 Kristaps Dzonsons
2014-10-19 21:38 ` Ingo Schwarze
0 siblings, 1 reply; 4+ messages in thread
From: Kristaps Dzonsons @ 2014-10-19 21:02 UTC (permalink / raw)
To: tech
[-- 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 '%':
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Division by zero
2014-10-19 21:02 Division by zero Kristaps Dzonsons
@ 2014-10-19 21:38 ` Ingo Schwarze
2014-10-20 10:11 ` Kristaps Dzonsons
0 siblings, 1 reply; 4+ messages in thread
From: Ingo Schwarze @ 2014-10-19 21:38 UTC (permalink / raw)
To: tech
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Division by zero
2014-10-19 21:38 ` Ingo Schwarze
@ 2014-10-20 10:11 ` Kristaps Dzonsons
2014-10-20 13:09 ` Ingo Schwarze
0 siblings, 1 reply; 4+ messages in thread
From: Kristaps Dzonsons @ 2014-10-20 10:11 UTC (permalink / raw)
To: tech
[-- 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);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Division by zero
2014-10-20 10:11 ` Kristaps Dzonsons
@ 2014-10-20 13:09 ` Ingo Schwarze
0 siblings, 0 replies; 4+ messages in thread
From: Ingo Schwarze @ 2014-10-20 13:09 UTC (permalink / raw)
To: Kristaps Dzonsons; +Cc: tech
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-10-20 13:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-19 21:02 Division by zero Kristaps Dzonsons
2014-10-19 21:38 ` Ingo Schwarze
2014-10-20 10:11 ` Kristaps Dzonsons
2014-10-20 13:09 ` Ingo Schwarze
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).