Re: [CFT][patch] mandoc: don't segfault on empty tbl(1) continuation blocks
Hi Baptiste, hi Eygene,
Baptiste Daroussin wrote on Wed, Jul 17, 2019 at 01:16:56PM +0200:
> On Wed, Jul 17, 2019 at 01:39:42PM +0300, Eygene Ryabinkin wrote:
>> but I believe that 'cpp' still can be NULL and will try to see
>> if it is triggerable.
I'm not sure what you mean here.
Do you think that in tbl_hrule(), there is a possibility that *cpp
might still be accessed even though cpp == NULL? If so, where and
how exactly?
As far as i can see, cpp can indeed easily be NULL anywhere in
tbl_hrule(), but it seems to me that for each access, != NULL
is checked immediately before.
If you still see a potential problem somewhere, please do speak up.
The table formatting logic is indeed complicated, so it's not
inconceivable that i still missed an edge case.
> As for the test case, the best would be that this test ends up
> incorporated in the upstream testsuite
Done, see the commit below.
Yours,
Ingo
Log Message:
---
new test for an empty text block; from rea@ via bapt@ (FreeBSD)
Modified Files:
--
mandoc/regress/tbl/data:
Makefile
Added Files:
---
mandoc/regress/tbl/data:
block_empty.in
block_empty.out_ascii
Revision Data
-
--- /dev/null
+++ regress/tbl/data/block_empty.in
@@ -0,0 +1,19 @@
+.\" $OpenBSD: block_empty.in,v 1.1 2019/07/18 14:38:47 schwarze Exp $
+.TH TBL-DATA-BLOCK_EMPTY 1 "July 17, 2019"
+.SH NAME
+tbl-data-block_empty \- empty text block
+.SH DESCRIPTION
+normal text
+.TS
+|l|l|.
+_
+A test
+_
+table T{
+T}
+_
+.TE
+.SH AUTHORS
+.MT r...@freebsd.org
+Eygene Ryabinkin
+.ME
--- /dev/null
+++ regress/tbl/data/block_empty.out_ascii
@@ -0,0 +1,22 @@
+TBL-DATA-BLOCK_EMPTY(1) General Commands ManualTBL-DATA-BLOCK_EMPTY(1)
+
+
+
+NNAAMMEE
+ tbl-data-block_empty - empty text block
+
+DDEESSCCRRIIPPTTIIOONN
+ normal text
+
+ +--+--+
+ |A | test |
+ +--+--+
+ |table | |
+ +--+--+
+
+AAUUTTHHOORRSS
+ Eygene Ryabinkin
+
+
+
+OpenBSD July 17, 2019 TBL-DATA-BLOCK_EMPTY(1)
Index: Makefile
===
RCS file: /home/cvs/mandoc/mandoc/regress/tbl/data/Makefile,v
retrieving revision 1.4
retrieving revision 1.5
diff -Lregress/tbl/data/Makefile -Lregress/tbl/data/Makefile -u -p -r1.4 -r1.5
--- regress/tbl/data/Makefile
+++ regress/tbl/data/Makefile
@@ -1,6 +1,7 @@
-# $OpenBSD: Makefile,v 1.4 2017/07/04 20:59:17 schwarze Exp $
+# $OpenBSD: Makefile,v 1.5 2019/07/18 14:38:47 schwarze Exp $
-REGRESS_TARGETS = blankline block_unclosed block_width block_wrap empty insert
+REGRESS_TARGETS = blankline block_empty block_unclosed block_width
+REGRESS_TARGETS+= block_wrap empty insert
LINT_TARGETS= block_unclosed empty insert
# groff-1.22.3 defect:
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: [CFT][patch] mandoc: don't segfault on empty tbl(1) continuation blocks
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=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=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=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=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=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
> 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
>
> Property changes on:
Re: [CFT][patch] mandoc: don't segfault on empty tbl(1) continuation blocks
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=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=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=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=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.
--
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=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
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
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
+
+.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:
Re: [CFT][patch] mandoc: don't segfault on empty tbl(1) continuation blocks
On Tue, Jul 16, 2019 at 10:31:24PM +0300, Eygene Ryabinkin wrote:
> Good day.
>
> 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.
>
> The simplest way to reproduce the issue is to either
> - run 'man notmuch-config' with mail/notmuch installed;
> - run 'mandoc tests/empty-table-cdata.1' against the attached
>test-only manpage.
>
> With the patch applied, one can utilize 'make check': regression
> test was added. Perhaps an invocation of
> {{{
> mtree -deU -f /usr/src/etc/mtree/BSD.tests.dist -p /usr/tests
> }}}
> will be needed to run 'make check' without remaking/installing
> the world.
>
> The patch is for the fresh -CURRENT. Be interested in any results
> of its application and usage.
>
> Thanks!
>
> P.S.: please, CC me: I am not subscribed to the list.
> --
> Eygene Ryabinkin,,,^..^,,,
> [ Life's unfair - but root password helps! | codelabs.ru ]
> [ 82FE 06BC D497 C0DE 49EC 4FF0 16AF 9EAE 8152 ECFB | freebsd.org ]
Hello,
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.
Best regards,
Bapt
signature.asc
Description: PGP signature
