Date: Wed, 17 Jul 2019 13:16:56 +0200 From: Baptiste Daroussin <bapt@FreeBSD.org> To: Eygene Ryabinkin <rea@freebsd.org> Cc: freebsd-current@FreeBSD.org, tech@mandoc.bsd.lv Subject: Re: [CFT][patch] mandoc: don't segfault on empty tbl(1) continuation blocks Message-ID: <20190717111655.eyq673itr76fj224@ivaldir.net> In-Reply-To: <20190717103942.fkunwe3utxvmdc5n@void.codelabs.ru> References: <20190716193124.yrrntrtah22aky5n@phoenix.codelabs.ru> <20190717071201.beem6et6dybhby7m@ivaldir.net> <20190717103942.fkunwe3utxvmdc5n@void.codelabs.ru>
next in thread | previous in thread | raw e-mail | index | archive | help
--fqf72zv4xfpkmw3d
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Wed, Jul 17, 2019 at 01:39:42PM +0300, Eygene Ryabinkin wrote:
> Baptiste, good day.
>=20
> 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.
>=20
> 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/5f6e3232931ab08da9c8121d568c8207c=
0c4662c#diff-bc5842dc5d7897de7bdac08f74804c57
> A bit differently: people just check for dpn->string being NULL.
>=20
> 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/7514a273fe4561e94f1277f4ee5991c9a=
f9cba2e#diff-bc5842dc5d7897de7bdac08f74804c57
> Can't trigger it with upstream's testcase,
> https://mandoc.bsd.lv/cgi-bin/cvsweb/regress/tbl/layout/shortlines.in?r=
ev=3D1.1&content-type=3Dtext/x-cvsweb-markup
> https://raw.githubusercontent.com/openbsd/src/7514a273fe4561e94f1277f4e=
e5991c9af9cba2e/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/b3e6a3251dfa92e66aa539518119564bd=
1945cc0#diff-bc5842dc5d7897de7bdac08f74804c57
> but I believe that 'cpp' still can be NULL and will try to see
> if it is triggerable.
>=20
> So, the patch that corresponds to the upstream change is attached.
>=20
> 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?
>=20
> Thanks.
Thank you for the patch and the test case, with mandoc, usually I try to be=
as
close as upstream as possible (targetting 100% ;).
So my approach in such case is to move to a snapshot of their cvs tree (as =
soon
as it has the fix incorporated).
As for the test case, the best would be that this test ends up incorporated=
in
the upstream testsuite (note that I need to plug it into our test framework
one day) I added the tech mailing list of mandoc in CC to give a chance to =
Ingo
to step on this.
I will be off for a week starting tonight, but I will update to the latest
snapshot of mandoc once back.o
We can still integrate some test case of our own as well, and I will be hap=
py to
integrate yours if not integrated in the upstream testsuite.
Best regards,
Bapt
> --=20
> Eygene Ryabinkin ,,,^..^,,,
> [ Life's unfair - but root password helps! | codelabs.ru ]
> [ 82FE 06BC D497 C0DE 49EC 4FF0 16AF 9EAE 8152 ECFB | freebsd.org ]
> mandoc: fix built-in tbl(1) processing of empty continuation blocks
>=20
> 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.
>=20
> 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.
>=20
> The segfault in question was plain NULL pointer dereference
> triggered from tbl_term.c::tbl_hrule().
>=20
> 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.
>=20
> The real-world case when manpage was provoking core dump
> is notmuch-config.1 for mail/notmuch port: it is auto-generated
> from reStructuredText, so has empty blocks at the places where
> it would be enough just to specify the empty value.
>=20
> 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>
>=20
> 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
>=20
> 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>.
>=20
> 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/man=
doc)
> +
> + atf_check -s exit: -o not-empty $mandoc "$SRCDIR"/empty-table-cdata.1
> +}
> +
> +
> +atf_init_test_cases() {
> + atf_add_test_case empty_table_cdata
> +}
>=20
> 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. */
--fqf72zv4xfpkmw3d
Content-Type: application/pgp-signature; name="signature.asc"
-----BEGIN PGP SIGNATURE-----
iQIzBAABCAAdFiEEgOTj3suS2urGXVU3Y4mL3PG3PloFAl0vA6cACgkQY4mL3PG3
PlokVhAA1+VsMD3EOA9cP5fDlJOUvE08HMumeQaov32HxpZxSEat5SM82Pej1jTp
ywRM9no0Bji+17pt5oNzXi6Jix+LEfnFl2UndAGzkQpfu+631VNySFMxcbGdVYEG
UOIiIZBKMVqD2J4ew/d3FGIgesy84VaxU033nGBREskR1Z7WC54W7MPsFrPq0qri
kbT3hOq2FA8L22pSgDx/eWNWBoasTJKqcgHQkBiNc+IQSDCFPOYKw1e3CP1W/vnx
7dOGN3gZ+1xy8rgk26cEcS2L1sKqklUDLlmMMPQuAyzyUtRjyIwjZAjKf2//ugHo
rY73tCiKdt6/enYDLX+S4Ps8f24oFi+bwyBp8G45QiC8EVDVb/dVrStOxPhiNQCH
iW0cTXJLKBd/e4gQcCzlJcR4Ddki2ua9yI1C4o9J7MzOY9Po1hCBFSjww94BpwdX
nj2Zb+Uru8r134Bf1x/Mob8IfPjIRa5Ch3938MbcFl6/VKoisB5yncmEFaiHngOR
jCI6KLtEmspD0pApUJndYFNaxfZYkfVW/2omcE8lB8hGVi4ZZ0jQQpO5qkXXnBwz
OGs8UdEIrbLMUWUazf1YS9W+apCqSOJKeK7zrBhEi5ETIK0LYWIiMm15VggbRK+6
1rImFnZsdNN8PIHF4br/UovuzIapzSpRZGgdu6ZeMYbCCqi+rwE=
=dzti
-----END PGP SIGNATURE-----
--fqf72zv4xfpkmw3d--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190717111655.eyq673itr76fj224>