From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx2.freebsd.org (mx2.freebsd.org [8.8.178.116]) by mandoc.bsd.lv (OpenSMTPD) with ESMTP id 8da3f8a3 for ; Wed, 17 Jul 2019 06:17:00 -0500 (EST) Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (Client CN "mx1.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx2.freebsd.org (Postfix) with ESMTPS id C01B380EA3 for ; Wed, 17 Jul 2019 11:16:58 +0000 (UTC) (envelope-from bapt@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id B19B074736; Wed, 17 Jul 2019 11:16:57 +0000 (UTC) (envelope-from bapt@FreeBSD.org) Received: from ivaldir.etoilebsd.net (etoilebsd.net [178.32.217.76]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) (Authenticated sender: bapt) by smtp.freebsd.org (Postfix) with ESMTPSA id 61B3F16E88; Wed, 17 Jul 2019 11:16:57 +0000 (UTC) (envelope-from bapt@FreeBSD.org) Received: by ivaldir.etoilebsd.net (Postfix, from userid 1001) id 54FE1C097F; Wed, 17 Jul 2019 13:16:56 +0200 (CEST) Date: Wed, 17 Jul 2019 13:16:56 +0200 From: Baptiste Daroussin To: Eygene Ryabinkin 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> References: <20190716193124.yrrntrtah22aky5n@phoenix.codelabs.ru> <20190717071201.beem6et6dybhby7m@ivaldir.net> <20190717103942.fkunwe3utxvmdc5n@void.codelabs.ru> X-Mailinglist: mandoc-tech Reply-To: tech@mandoc.bsd.lv MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="fqf72zv4xfpkmw3d" Content-Disposition: inline In-Reply-To: <20190717103942.fkunwe3utxvmdc5n@void.codelabs.ru> User-Agent: NeoMutt/20180716 X-Rspamd-Queue-Id: B19B074736 X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-2.98 / 15.00]; local_wl_from(0.00)[FreeBSD.org]; NEURAL_HAM_MEDIUM(-1.00)[-0.998,0]; NEURAL_HAM_SHORT(-0.98)[-0.981,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; ASN(0.00)[asn:11403, ipnet:96.47.64.0/20, country:US] --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 > 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 >=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 > + > +.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, . >=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-- -- To unsubscribe send an email to tech+unsubscribe@mandoc.bsd.lv