FreeBSD Mail Archives

Date:      Wed, 17 Jul 2019 13:39:42 +0300
From:      Eygene Ryabinkin <rea@freebsd.org>
To:        Baptiste Daroussin <bapt@FreeBSD.org>
Cc:        freebsd-current@FreeBSD.org
Subject:   Re: [CFT][patch] mandoc: don't segfault on empty tbl(1) continuation blocks
Message-ID:  <20190717103942.fkunwe3utxvmdc5n@void.codelabs.ru>
In-Reply-To: <20190717071201.beem6et6dybhby7m@ivaldir.net>
References:  <20190716193124.yrrntrtah22aky5n@phoenix.codelabs.ru> <20190717071201.beem6et6dybhby7m@ivaldir.net>


--nzno7o2fwb6xlvhr
Content-Type: multipart/mixed; boundary="mg6b44fawrvf627e"
Content-Disposition: inline


--mg6b44fawrvf627e
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Baptiste, good day.

Wed, Jul 17, 2019 at 09:12:02AM +0200, Baptiste Daroussin wrote:
> On Tue, Jul 16, 2019 at 10:31:24PM +0300, Eygene Ryabinkin wrote:
> > Attached is the patch that makes built-in tbl(1) processor in
> > mandoc to avoid dumping core when it renders the table with empty
> > "T{ T}" block and horizontally-ruled table.
>
> Thank you for the patch! Have it been discussed with upstream? I
> kind of remind something like this being reported to upstream, but I
> haven't checked the status.

Was fixed:
  https://mandoc.bsd.lv/cgi-bin/cvsweb/tbl_term.c.diff?r1=3D1.69&r2=3D1.70
  https://github.com/openbsd/src/commit/5f6e3232931ab08da9c8121d568c8207c0c=
4662c#diff-bc5842dc5d7897de7bdac08f74804c57
A bit differently: people just check for dpn->string being NULL.

And there is another one NULL pointer fix,
  https://mandoc.bsd.lv/cgi-bin/cvsweb/tbl_term.c.diff?r1=3D1.70&r2=3D1.71
  https://github.com/openbsd/src/commit/7514a273fe4561e94f1277f4ee5991c9af9=
cba2e#diff-bc5842dc5d7897de7bdac08f74804c57
Can't trigger it with upstream's testcase,
  https://mandoc.bsd.lv/cgi-bin/cvsweb/regress/tbl/layout/shortlines.in?rev=
=3D1.1&content-type=3Dtext/x-cvsweb-markup
  https://raw.githubusercontent.com/openbsd/src/7514a273fe4561e94f1277f4ee5=
991c9af9cba2e/regress/usr.bin/mandoc/tbl/layout/shortlines.in
since current FreeBSD's mandoc lacks this modification,
  https://mandoc.bsd.lv/cgi-bin/cvsweb/tbl_term.c.diff?r1=3D1.68&r2=3D1.69
  https://github.com/openbsd/src/commit/b3e6a3251dfa92e66aa539518119564bd19=
45cc0#diff-bc5842dc5d7897de7bdac08f74804c57
but I believe that 'cpp' still can be NULL and will try to see
if it is triggerable.

So, the patch that corresponds to the upstream change is attached.

Nothing was released after 1.14.5 (yet).  What will be the route?
Will you
 - wait for the new release;
   - if yes, will you incorporate the testing part?
 - if no, I think you will use the closer-to-upstream patch?

Thanks.
--=20
Eygene Ryabinkin                                        ,,,^..^,,,
[ Life's unfair - but root password helps!           | codelabs.ru ]
[ 82FE 06BC D497 C0DE 49EC  4FF0 16AF 9EAE 8152 ECFB | freebsd.org ]

--mg6b44fawrvf627e
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="mandoc-fix-empty-cdata-crash--upstream-ver.patch"
Content-Transfer-Encoding: quoted-printable

mandoc: fix built-in tbl(1) processing of empty continuation blocks

Empty "T{ T}" (continuation) blocks produce NULL-valued string
for their data block: getdata() allocates structure with string
set to NULL and tbl_cdata() will just return when it sees
the end ("T}") of the block without any further manipulations
with dat->string.

This is completely legal; moreover, tbl.h specifies that for
'struct tbl_dat' the 'string' member is NULL when entry type
is not TBL_DATA_DATA.  This is not so all the time, but one
shouldn't rely on this.

The segfault in question was plain NULL pointer dereference
triggered from tbl_term.c::tbl_hrule().

Added check for dpn->string not being NULL to be in sync
with upstream,
  https://mandoc.bsd.lv/cgi-bin/cvsweb/tbl_term.c.diff?r1=3D1.69&r2=3D1.70
Also added regression test to find such problems in the future.

The real-world case when manpage was provoking core dump
is notmuch-config.1 for mail/notmuch port: it is auto-generated
=66rom reStructuredText, so has empty blocks at the places where
it would be enough just to specify the empty value.

Index: usr.bin/mandoc/Makefile
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- usr.bin/mandoc/Makefile	(revision 349971)
+++ usr.bin/mandoc/Makefile	(working copy)
@@ -101,4 +101,7 @@
 CFLAGS.gcc+=3D	-Wno-format
 LIBADD=3D	openbsd z
=20
+HAS_TESTS=3D
+SUBDIR.${MK_TESTS}+=3D tests
+
 .include <bsd.prog.mk>
Index: usr.bin/mandoc/tests/Makefile
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- usr.bin/mandoc/tests/Makefile	(nonexistent)
+++ usr.bin/mandoc/tests/Makefile	(working copy)
@@ -0,0 +1,11 @@
+# $FreeBSD$
+
+PACKAGE=3D	tests
+
+${PACKAGE}FILES+=3D	empty-table-cdata.1
+
+ATF_TESTS_SH+=3D		regression-tests
+
+BINDIR=3D	${TESTSDIR}
+
+.include <bsd.test.mk>

Property changes on: usr.bin/mandoc/tests/Makefile
___________________________________________________________________
Added: svn:eol-style
## -0,0 +1 ##
+native
\ No newline at end of property
Added: svn:keywords
## -0,0 +1 ##
+FreeBSD=3D%H
\ No newline at end of property
Added: svn:mime-type
## -0,0 +1 ##
+text/plain
\ No newline at end of property
Index: usr.bin/mandoc/tests/Makefile.depend
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- usr.bin/mandoc/tests/Makefile.depend	(nonexistent)
+++ usr.bin/mandoc/tests/Makefile.depend	(working copy)
@@ -0,0 +1,11 @@
+# $FreeBSD$
+# Autogenerated - do NOT edit!
+
+DIRDEPS =3D \
+
+
+.include <dirdeps.mk>
+
+.if ${DEP_RELDIR} =3D=3D ${_DEP_RELDIR}
+# local dependencies - needed for -jN in clean tree
+.endif

Property changes on: usr.bin/mandoc/tests/Makefile.depend
___________________________________________________________________
Added: svn:eol-style
## -0,0 +1 ##
+native
\ No newline at end of property
Added: svn:keywords
## -0,0 +1 ##
+FreeBSD=3D%H
\ No newline at end of property
Added: svn:mime-type
## -0,0 +1 ##
+text/plain
\ No newline at end of property
Index: usr.bin/mandoc/tests/empty-table-cdata.1
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- usr.bin/mandoc/tests/empty-table-cdata.1	(nonexistent)
+++ usr.bin/mandoc/tests/empty-table-cdata.1	(working copy)
@@ -0,0 +1,21 @@
+.\" $FreeBSD$
+.
+.TH EMPTY-TABLE-CDATA 1 1970-01-01
+.SH Empty table cdata test for tbl processor
+.
+.PP
+The following table should not make mandoc to dump core:
+.
+.TS
+|l|l|.
+_
+A	test
+_
+table	T{
+T}
+_
+.TE
+.
+.SH Author
+.PP
+Eygene Ryabinkin, <rea@FreeBSD.org>.

Property changes on: usr.bin/mandoc/tests/empty-table-cdata.1
___________________________________________________________________
Added: svn:eol-style
## -0,0 +1 ##
+native
\ No newline at end of property
Added: svn:keywords
## -0,0 +1 ##
+FreeBSD=3D%H
\ No newline at end of property
Added: svn:mime-type
## -0,0 +1 ##
+text/plain
\ No newline at end of property
Index: usr.bin/mandoc/tests/regression-tests.sh
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- usr.bin/mandoc/tests/regression-tests.sh	(nonexistent)
+++ usr.bin/mandoc/tests/regression-tests.sh	(working copy)
@@ -0,0 +1,20 @@
+# $FreeBSD$
+
+
+SRCDIR=3D$(atf_get_srcdir)
+
+
+atf_test_case empty_table_cdata
+empty_table_cdata_head() {
+	atf_set "descr" "Normal processing of empty T{ T} blocks in tables"
+}
+empty_table_cdata_body() {
+	local mandoc=3D$(atf_config_get usr.bin.mandoc.test_mandoc /usr/bin/mando=
c)
+
+	atf_check -s exit: -o not-empty $mandoc "$SRCDIR"/empty-table-cdata.1
+}
+
+
+atf_init_test_cases() {
+	atf_add_test_case empty_table_cdata
+}

Property changes on: usr.bin/mandoc/tests/regression-tests.sh
___________________________________________________________________
Added: svn:eol-style
## -0,0 +1 ##
+native
\ No newline at end of property
Added: svn:executable
## -0,0 +1 ##
+*
\ No newline at end of property
Added: svn:keywords
## -0,0 +1 ##
+FreeBSD=3D%H
\ No newline at end of property
Added: svn:mime-type
## -0,0 +1 ##
+text/plain
\ No newline at end of property
Index: etc/mtree/BSD.tests.dist
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- etc/mtree/BSD.tests.dist	(revision 349971)
+++ etc/mtree/BSD.tests.dist	(working copy)
@@ -1004,6 +1004,8 @@
         ..
         m4
         ..
+        mandoc
+        ..
         mkimg
         ..
         ncal
Index: contrib/mandoc/tbl_term.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- contrib/mandoc/tbl_term.c	(revision 349971)
+++ contrib/mandoc/tbl_term.c	(working copy)
@@ -626,7 +626,8 @@
=20
 		lw =3D cpp =3D=3D NULL || cpn =3D=3D NULL ||
 		    (cpn->pos !=3D TBL_CELL_DOWN &&
-		     (dpn =3D=3D NULL || strcmp(dpn->string, "\\^") !=3D 0))
+		     (dpn =3D=3D NULL || dpn->string =3D=3D NULL ||
+		      strcmp(dpn->string, "\\^") !=3D 0))
 		    ? hw : 0;
 		tbl_direct_border(tp, BHORIZ * lw,
 		    col->width + col->spacing / 2);
@@ -670,7 +671,8 @@
=20
 		rw =3D cpp =3D=3D NULL || cpn =3D=3D NULL ||
 		    (cpn->pos !=3D TBL_CELL_DOWN &&
-		     (dpn =3D=3D NULL || strcmp(dpn->string, "\\^") !=3D 0))
+		     (dpn =3D=3D NULL || dpn->string =3D=3D NULL ||
+		      strcmp(dpn->string, "\\^") !=3D 0))
 		    ? hw : 0;
=20
 		/* The line crossing at the end of this column. */

--mg6b44fawrvf627e--

--nzno7o2fwb6xlvhr
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iNUEABEKAH0WIQSC/ga81JfA3knsT/AWr56ugVLs+wUCXS766V8UgAAAAAAuAChp
c3N1ZXItZnByQG5vdGF0aW9ucy5vcGVucGdwLmZpZnRoaG9yc2VtYW4ubmV0ODJG
RTA2QkNENDk3QzBERTQ5RUM0RkYwMTZBRjlFQUU4MTUyRUNGQgAKCRAWr56ugVLs
+7DYAQCYSOj28O7ejYIzX24D9QCfdJ0TOT3TekFAvf7PO8Ji9AD+J55WeU82unCy
hp721dQnO+v65hpTl08ug2NWSVMT3GY=
=Dqau
-----END PGP SIGNATURE-----

--nzno7o2fwb6xlvhr--

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190717103942.fkunwe3utxvmdc5n>

Header And Logo

Peripheral Links

Site Navigation

Header And Logo

Peripheral Links

Search

Site Navigation