tech@mandoc.bsd.lv
 help / color / mirror / Atom feed
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
Date: Wed, 17 Jul 2019 13:16:56 +0200
Message-ID: <20190717111655.eyq673itr76fj224@ivaldir.net> (raw)
In-Reply-To: <20190717103942.fkunwe3utxvmdc5n@void.codelabs.ru>

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

On Wed, Jul 17, 2019 at 01:39:42PM +0300, Eygene Ryabinkin wrote:
> 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=1.69&r2=1.70
>   https://github.com/openbsd/src/commit/5f6e3232931ab08da9c8121d568c8207c0c4662c#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=1.70&r2=1.71
>   https://github.com/openbsd/src/commit/7514a273fe4561e94f1277f4ee5991c9af9cba2e#diff-bc5842dc5d7897de7bdac08f74804c57
> Can't trigger it with upstream's testcase,
>   https://mandoc.bsd.lv/cgi-bin/cvsweb/regress/tbl/layout/shortlines.in?rev=1.1&content-type=text/x-cvsweb-markup
>   https://raw.githubusercontent.com/openbsd/src/7514a273fe4561e94f1277f4ee5991c9af9cba2e/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=1.68&r2=1.69
>   https://github.com/openbsd/src/commit/b3e6a3251dfa92e66aa539518119564bd1945cc0#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.

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 happy to
integrate yours if not integrated in the upstream testsuite.

Best regards,
Bapt

> -- 
> 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
> 
> 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=1.69&r2=1.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
> from reStructuredText, so has empty blocks at the places where
> it would be enough just to specify the empty value.
> 
> Index: usr.bin/mandoc/Makefile
> ===================================================================
> --- usr.bin/mandoc/Makefile	(revision 349971)
> +++ usr.bin/mandoc/Makefile	(working copy)
> @@ -101,4 +101,7 @@
>  CFLAGS.gcc+=	-Wno-format
>  LIBADD=	openbsd z
>  
> +HAS_TESTS=
> +SUBDIR.${MK_TESTS}+= tests
> +
>  .include <bsd.prog.mk>
> Index: usr.bin/mandoc/tests/Makefile
> ===================================================================
> --- usr.bin/mandoc/tests/Makefile	(nonexistent)
> +++ usr.bin/mandoc/tests/Makefile	(working copy)
> @@ -0,0 +1,11 @@
> +# $FreeBSD$
> +
> +PACKAGE=	tests
> +
> +${PACKAGE}FILES+=	empty-table-cdata.1
> +
> +ATF_TESTS_SH+=		regression-tests
> +
> +BINDIR=	${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=%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
> ===================================================================
> --- 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 = \
> +
> +
> +.include <dirdeps.mk>
> +
> +.if ${DEP_RELDIR} == ${_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=%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
> ===================================================================
> --- 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=%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
> ===================================================================
> --- usr.bin/mandoc/tests/regression-tests.sh	(nonexistent)
> +++ usr.bin/mandoc/tests/regression-tests.sh	(working copy)
> @@ -0,0 +1,20 @@
> +# $FreeBSD$
> +
> +
> +SRCDIR=$(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=$(atf_config_get usr.bin.mandoc.test_mandoc /usr/bin/mandoc)
> +
> +	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=%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
> ===================================================================
> --- 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
> ===================================================================
> --- contrib/mandoc/tbl_term.c	(revision 349971)
> +++ contrib/mandoc/tbl_term.c	(working copy)
> @@ -626,7 +626,8 @@
>  
>  		lw = cpp == NULL || cpn == NULL ||
>  		    (cpn->pos != TBL_CELL_DOWN &&
> -		     (dpn == NULL || strcmp(dpn->string, "\\^") != 0))
> +		     (dpn == NULL || dpn->string == NULL ||
> +		      strcmp(dpn->string, "\\^") != 0))
>  		    ? hw : 0;
>  		tbl_direct_border(tp, BHORIZ * lw,
>  		    col->width + col->spacing / 2);
> @@ -670,7 +671,8 @@
>  
>  		rw = cpp == NULL || cpn == NULL ||
>  		    (cpn->pos != TBL_CELL_DOWN &&
> -		     (dpn == NULL || strcmp(dpn->string, "\\^") != 0))
> +		     (dpn == NULL || dpn->string == NULL ||
> +		      strcmp(dpn->string, "\\^") != 0))
>  		    ? hw : 0;
>  
>  		/* The line crossing at the end of this column. */




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

       reply	other threads:[~2019-07-17 11:17 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20190716193124.yrrntrtah22aky5n@phoenix.codelabs.ru>
     [not found] ` <20190717071201.beem6et6dybhby7m@ivaldir.net>
     [not found]   ` <20190717103942.fkunwe3utxvmdc5n@void.codelabs.ru>
2019-07-17 11:16     ` Baptiste Daroussin [this message]
2019-07-18 15:00       ` Ingo Schwarze

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190717111655.eyq673itr76fj224@ivaldir.net \
    --to=bapt@freebsd.org \
    --cc=freebsd-current@FreeBSD.org \
    --cc=rea@freebsd.org \
    --cc=tech@mandoc.bsd.lv \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

tech@mandoc.bsd.lv

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.vuxu.org/mandoc-tech

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 mandoc-tech mandoc-tech/ http://inbox.vuxu.org/mandoc-tech \
		tech@mandoc.bsd.lv
	public-inbox-index mandoc-tech

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.mandoc.tech


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git