[mandoc] segfault due to missing tbl layout

[mandoc] segfault due to missing tbl layout

[Stephen Gregoratto]

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

Here's an interesting bug I found in the wild. First, some backstory.
The aerc[1] email client uses scdoc[2] to generate its manpages (same
author). Scdoc files are kinda like markdown, but they have a funky way
of setting out tables (see the TABLES section of scdoc(5)[3]).

So, the aerc-config document has a long table for command keys, and in
the middle of it the author forgot to set the alignment for a cell. I'm
attaching the formatted document, but I can trigger the same bug with
this (bug.5):

  .TS
  allbox;
  l c
  l
  l l.
  Foo	Bar
  FooBar
  Foo	Bar
  .TE

This occurs on Arch Linux and OpenBSD 6.5, with the latest changes from
CVS. This is the output from GDB:

#0  0x000055632eb7ff52 in tbl_hrule (tp=0x55632edc6d00, spp=0x55632edc31a0, sp=0x55632edc31a0, spn=0x55632edc3310, flags=1) at tbl_term.c:626
626                     col = tp->tbl.cols + cp->col;
(gdb) p tp->tbl.cols
$2 = (struct roffcol *) 0x55632edcb890
(gdb) p cp->col
Cannot access memory at address 0x24

I've tested this with groff and Plan 9 troff/tbl and they handle this
fine.

[1] https://git.sr.ht/~sircmpwn/aerc2
[2] https://git.sr.ht/~sircmpwn/scdoc
[3] https://git.sr.ht/~sircmpwn/scdoc/blob/master/scdoc.5.scd
[4] https://git.sr.ht/~sircmpwn/aerc/blob/master/doc/aerc-config.5.scd
-- 
Stephen Gregoratto
PGP: 3FC6 3D0E 2801 C348 1C44 2D34 A80C 0F8E 8BAB EC8B

[-- Attachment #2: bug.5 --]
[-- Type: application/x-troff-man, Size: 59 bytes --]

[-- Attachment #3: aerc-config.5 --]
[-- Type: application/x-troff-man, Size: 9793 bytes --]

Re: [mandoc] segfault due to missing tbl layout

[Ingo Schwarze]

Hi Stephen,

Stephen Gregoratto wrote on Tue, Jun 04, 2019 at 12:53:29PM +1000:

> Here's an interesting bug I found in the wild. First, some backstory.
> The aerc[1] email client uses scdoc[2] to generate its manpages (same
> author). Scdoc files are kinda like markdown,

Bleah.  Why on earth do people think using markup languages with
weakly defined, highly context dependent syntax is a good idea when
better languages are readily available?  Anyway, that's not the
point here.

> but they have a funky way
> of setting out tables (see the TABLES section of scdoc(5)[3]).

Right.  Start with a crappy input language, then convert it to
low-quality output in the target language.  So typical...  :-(

The only purpose such crap is good for is compatibility testing
and finding bugs (an approach similar to fuzzing), so i added
a TODO entry, see in the committed patch below.

> So, the aerc-config document has a long table for command keys, and in
> the middle of it the author forgot to set the alignment for a cell. I'm
> attaching the formatted document, but I can trigger the same bug with
> this (bug.5):
> 
>   .TS
>   allbox;
>   l c
>   l
>   l l.
>   Foo	Bar
>   FooBar
>   Foo	Bar
>   .TE
> 
> This occurs on Arch Linux and OpenBSD 6.5, with the latest changes from
> CVS. This is the output from GDB:
> 
> #0  0x000055632eb7ff52 in tbl_hrule (tp=0x55632edc6d00, spp=0x55632edc31a0, sp=0x55632edc31a0, spn=0x55632edc3310, flags=1) at tbl_term.c:626
> 626                     col = tp->tbl.cols + cp->col;
> (gdb) p tp->tbl.cols
> $2 = (struct roffcol *) 0x55632edcb890
> (gdb) p cp->col
> Cannot access memory at address 0x24

Thank you for the complete information and thorough analysis, that
is very helpful.  Yes, this was certainly a bug in mandoc, possibly
introduced while improving line drawing in tables (in particular
for using UTF-8 line drawing characters) a few months ago.

I committed the patch below to OpenBSD and bsd.lv; i assume it fixes
the bug for you, too?

Yours,
  Ingo

> I've tested this with groff and Plan 9 troff/tbl and they handle this
> fine.
> 
> [1] https://git.sr.ht/~sircmpwn/aerc2
> [2] https://git.sr.ht/~sircmpwn/scdoc
> [3] https://git.sr.ht/~sircmpwn/scdoc/blob/master/scdoc.5.scd
> [4] https://git.sr.ht/~sircmpwn/aerc/blob/master/doc/aerc-config.5.scd


Log Message:
-----------
Do not access a NULL pointer if a table contains a horizontal line
next to a table line having fewer columns than the table as a whole.
Bug found by Stephen Gregoratto <dev at sgregoratto dot me>
with aerc-config(5).

Modified Files:
--------------
    mandoc:
        TODO
        tbl_term.c
    mandoc/regress/tbl/layout:
        Makefile

Added Files:
-----------
    mandoc/regress/tbl/layout:
        shortlines.in
        shortlines.out_ascii

Revision Data
-------------
Index: tbl_term.c
===================================================================
RCS file: /home/cvs/mandoc/mandoc/tbl_term.c,v
retrieving revision 1.70
retrieving revision 1.71
diff -Ltbl_term.c -Ltbl_term.c -u -p -r1.70 -r1.71
--- tbl_term.c
+++ tbl_term.c
@@ -622,8 +622,12 @@ tbl_hrule(struct termp *tp, const struct
 		    (spp == NULL || cpn == NULL ||
 		     cpn->pos != TBL_CELL_DOWN ? BRIGHT * hw : 0), 1);
 
+	col = tp->tbl.cols;
 	for (;;) {
-		col = tp->tbl.cols + cp->col;
+		if (cp == NULL)
+			col++;
+		else
+			col = tp->tbl.cols + cp->col;
 
 		/* Print the horizontal line inside this column. */
 
@@ -649,7 +653,8 @@ tbl_hrule(struct termp *tp, const struct
 					uw = 1;
 			}
 			cpp = cpp->next;
-		}
+		} else if (spp != NULL && opts & TBL_OPT_ALLBOX)
+			uw = 1;
 		if (cp != NULL)
 			cp = cp->next;
 		if (cpn != NULL) {
@@ -661,8 +666,9 @@ tbl_hrule(struct termp *tp, const struct
 			cpn = cpn->next;
 			while (dpn != NULL && dpn->layout != cpn)
 				dpn = dpn->next;
-		}
-		if (cpp == NULL && cp == NULL && cpn == NULL)
+		} else if (spn != NULL && opts & TBL_OPT_ALLBOX)
+			dw = 1;
+		if (col + 1 == tp->tbl.cols + sp->opts->cols)
 			break;
 
 		/* Vertical lines do not cross spanned cells. */
Index: TODO
===================================================================
RCS file: /home/cvs/mandoc/mandoc/TODO,v
retrieving revision 1.294
retrieving revision 1.295
diff -LTODO -LTODO -u -p -r1.294 -r1.295
--- TODO
+++ TODO
@@ -285,6 +285,9 @@ are mere guesses, and some may be wrong.
   https://github.com/schmonz/ikiwiki/compare/mandoc
   Amitai Schlair  Mon, 19 May 2014 14:05:53 -0400
 
+- check compatibility with
+  https://git.sr.ht/~sircmpwn/scdoc
+
 - check features of the Slackware man.conf(5) format
   Carsten Kunze  Wed, 11 Mar 2015 17:57:24 +0100
 
Index: Makefile
===================================================================
RCS file: /home/cvs/mandoc/mandoc/regress/tbl/layout/Makefile,v
retrieving revision 1.2
retrieving revision 1.3
diff -Lregress/tbl/layout/Makefile -Lregress/tbl/layout/Makefile -u -p -r1.2 -r1.3
--- regress/tbl/layout/Makefile
+++ regress/tbl/layout/Makefile
@@ -1,7 +1,7 @@
-# $OpenBSD: Makefile,v 1.2 2015/01/30 00:27:09 schwarze Exp $
+# $OpenBSD: Makefile,v 1.4 2019/06/11 15:40:41 schwarze Exp $
 
 REGRESS_TARGETS	 = center complex empty emptyline
-REGRESS_TARGETS	+= lines lines-nogroff numbers span
+REGRESS_TARGETS	+= lines lines-nogroff numbers shortlines span
 LINT_TARGETS	 = complex empty
 
 # groff-1.22.3 defects:
--- /dev/null
+++ regress/tbl/layout/shortlines.in
@@ -0,0 +1,49 @@
+.\" $OpenBSD: shortlines.in,v 1.1 2019/06/11 15:40:41 schwarze Exp $
+.TH TBL-LAYOUT-SHORTLINES 1 "June 11, 2019"
+.SH NAME
+tbl-layout-shortlines \- table lines of different length
+.SH DESCRIPTION
+normal text
+.TS
+allbox tab(:);
+L L
+L
+L L.
+left:right
+short
+left:right
+.TE
+.sp
+.TS
+allbox tab(:);
+L L
+L
+L
+L L.
+left:right
+first short
+second short
+left:right
+.TE
+.sp
+.TS
+allbox tab(:);
+L L L
+L L
+L.
+left:middle:right
+short:line
+very short
+.TE
+.sp
+.TS
+allbox tab(:);
+L
+L L
+L L L.
+very short
+short:line
+left:middle:right
+.TE
+
+
--- /dev/null
+++ regress/tbl/layout/shortlines.out_ascii
@@ -0,0 +1,48 @@
+TBL-LAYOUT-SHORTLINES(1)    General Commands Manual   TBL-LAYOUT-SHORTLINES(1)
+
+
+
+N\bNA\bAM\bME\bE
+       tbl-layout-shortlines - table lines of different length
+
+D\bDE\bES\bSC\bCR\bRI\bIP\bPT\bTI\bIO\bON\bN
+       normal text
+
+       +------+-------+
+       |left  | right |
+       +------+-------+
+       |short |       |
+       +------+-------+
+       |left  | right |
+       +------+-------+
+
+       +-------------+-------+
+       |left         | right |
+       +-------------+-------+
+       |first short  |       |
+       +-------------+-------+
+       |second short |       |
+       +-------------+-------+
+       |left         | right |
+       +-------------+-------+
+
+       +-----------+--------+-------+
+       |left       | middle | right |
+       +-----------+--------+-------+
+       |short      | line   |       |
+       +-----------+--------+-------+
+       |very short |        |       |
+       +-----------+--------+-------+
+
+       +-----------+--------+-------+
+       |very short |        |       |
+       +-----------+--------+-------+
+       |short      | line   |       |
+       +-----------+--------+-------+
+       |left       | middle | right |
+       +-----------+--------+-------+
+
+
+
+
+OpenBSD                          June 11, 2019        TBL-LAYOUT-SHORTLINES(1)
--
 To unsubscribe send an email to tech+unsubscribe@mandoc.bsd.lv

Re: [mandoc] segfault due to missing tbl layout

[Stephen Gregoratto]

On 2019-06-11 18:23, Ingo Schwarze wrote:
> Bleah.  Why on earth do people think using markup languages with
> weakly defined, highly context dependent syntax is a good idea when
> better languages are readily available?

The author posted this on his blog[1] and expanded further on a comment
thread[2] on lobste.rs. It boils down to not using extra deps and
wanting to roll a simple solution in C.
 
> Thank you for the complete information and thorough analysis, that
> is very helpful.  Yes, this was certainly a bug in mandoc, possibly
> introduced while improving line drawing in tables (in particular
> for using UTF-8 line drawing characters) a few months ago.
> 
> I committed the patch below to OpenBSD and bsd.lv; i assume it fixes
> the bug for you, too?

Indeed it does, cheers.

[1] https://drewdevault.com/2018/05/13/scdoc.html
[2] https://lobste.rs/s/q5awqv/introducing_scdoc_man_page_generator#c_fkleig
-- 
Stephen Gregoratto
PGP: 3FC6 3D0E 2801 C348 1C44 2D34 A80C 0F8E 8BAB EC8B
--
 To unsubscribe send an email to tech+unsubscribe@mandoc.bsd.lv