* 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).