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:
[CFT][patch] mandoc: don't segfault on empty tbl(1) continuation blocks
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 ]
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->pos not being TBL_DATA_DATA. 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: 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, .
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$
+
+
+SR
Re: [CFR][PATCH] drm2: don't assume that dev-driver-max_ioctl *dev-driver-compat_ioctls_nr
Fri, Nov 28, 2014 at 07:32:26PM +0200, Konstantin Belousov wrote: On Fri, Nov 28, 2014 at 04:29:42PM +0300, Eygene Ryabinkin wrote: I noticed that the current ioctl processing code for drm2 implicitely assumes that the number of native ioctls is higher than that of 32-bit compat ones, so it immediately gives EINVAL when nr = dev-driver-max_ioctl. Seems that in future such assumption may not be true in all cases. I very much doubt that it could become true. Compat32 ioctl cannot exist without its wider counterpart. OK. This can be fixed with the following patch: http://codelabs.ru/fbsd/patches/drm2/drm_drv-untangle-32bit-compat.diff Any thoughts on it? I think either current way or patch are fine, but why changing something which is fine ? Because the patched code will work with less assumptions and the patch isn't big or complex and introduces no additional code paths, just rearranges things. It also has more unified logics: if 32-bit compat is present and ioctl fits into its range -- do that. If ioctl fits into the native driver ioctl range -- do that. Otherwise -- bail out with an error. And if conditions are more uniform w.r.t. check for value of nr. -- Eygene Ryabinkin,,,^..^,,, [ Life's unfair - but root password helps! | codelabs.ru ] [ 82FE 06BC D497 C0DE 49EC 4FF0 16AF 9EAE 8152 ECFB | freebsd.org ] pgpKRkTfHMt6t.pgp Description: PGP signature
[CFR][PATCH] drm2: don't assume that dev-driver-max_ioctl *dev-driver-compat_ioctls_nr
Konstantin, *, good day. I noticed that the current ioctl processing code for drm2 implicitely assumes that the number of native ioctls is higher than that of 32-bit compat ones, so it immediately gives EINVAL when nr = dev-driver-max_ioctl. Seems that in future such assumption may not be true in all cases. This can be fixed with the following patch: http://codelabs.ru/fbsd/patches/drm2/drm_drv-untangle-32bit-compat.diff Any thoughts on it? Thanks. -- Eygene Ryabinkin,,,^..^,,, [ Life's unfair - but root password helps! | codelabs.ru ] [ 82FE 06BC D497 C0DE 49EC 4FF0 16AF 9EAE 8152 ECFB | freebsd.org ] pgp4NKeZVlbnb.pgp Description: PGP signature
Re: [CFT][CFR] Resurrect handling of VersionAddendum in OpenSSH
Sat, May 26, 2012 at 02:34:20PM +0400, Eygene Ryabinkin wrote: Can anyone who uses SSH test this patch and report their findings to the respective PR. Also, code reviews are welcome too. I head been blessed by des@ to commit the code into -CURRENT and I did so, http://svnweb.freebsd.org/base?view=revisionrevision=236139 so people who run HEAD will automatically receive this fix starting from now. -- Eygene Ryabinkin,,,^..^,,, [ Life's unfair - but root password helps! | codelabs.ru ] [ 82FE 06BC D497 C0DE 49EC 4FF0 16AF 9EAE 8152 ECFB | freebsd.org ] pgp7UPlMJwV5C.pgp Description: PGP signature
[CFT][CFR] Resurrect handling of VersionAddendum in OpenSSH
Good day.
I had created patches for OpenSSH to resurrect handling of
VersionAddendum and, additionally, enable/disable advertisements
for HPN feature in sshd version banner. des@, bz@ and brooks@
are aware of this patch, Bjoern even reviewed the first version
of the patch, but the second one isn't yet reviewed.
Can anyone who uses SSH test this patch and report their findings
to the respective PR. Also, code reviews are welcome too.
Thanks!
PR: http://www.freebsd.org/cgi/query-pr.cgi?pr=163843
Patches:
* 8-STABLE,
http://codelabs.ru/fbsd/patches/openssh/OpenSSH-fix-VersionAddendum-handling-8-STABLE.diff
* 9-STABLE,
http://codelabs.ru/fbsd/patches/openssh/OpenSSH-fix-VersionAddendum-handling-9-STABLE.diff
* 10-CURRENT,
http://codelabs.ru/fbsd/patches/openssh/OpenSSH-fix-VersionAddendum-handling.diff
Instructions:
{{{
cd /usr/src
fetch -o ssh-addendum.diff
http://codelabs.ru/fbsd/patches/openssh/OpenSSH-fix-VersionAddendum-handling.diff
patch -p1 ssh-addendum.diff
cd secure/lib/libssh
make make install
cd ../../usr.sbin/sshd
make make install service sshd restart
cd ../..//usr.bin/ssh
make make install
}}}
--
Eygene Ryabinkin,,,^..^,,,
[ Life's unfair - but root password helps! | codelabs.ru ]
[ 82FE 06BC D497 C0DE 49EC 4FF0 16AF 9EAE 8152 ECFB | freebsd.org ]
pgpSRvvgrwBwo.pgp
Description: PGP signature
Re: No working IDE in FreeBSD!
Thu, Mar 01, 2012 at 02:31:06PM +0200, Alexander Yerenkow wrote: I tried to take some time of Philip Paeps, but he is always busy with something. Maybe someone of you guys could be interested in filling request for open source license. Are you requesting someone to create FreeBSD port for this IDE or you meant something else? -- Eygene Ryabinkin,,,^..^,,, [ Life's unfair - but root password helps! | codelabs.ru ] [ 82FE 06BC D497 C0DE 49EC 4FF0 16AF 9EAE 8152 ECFB | freebsd.org ] pgpuP6wNZwsie.pgp Description: PGP signature
Anyone is currently using the rc_fast_and_loose rc.conf variable?
Good day. Sorry for cross-posting, but this question is really belongs to all three lists. Crawling over the rc.d scripts I had found the rc_fast_and_loose variable that affects the way rc.d scripts are processed inside /etc/rc script. There are some problems with certain rc.d script and this variable: they are described in my post to freebsd-rc@, http://lists.freebsd.org/pipermail/freebsd-rc/2011-December/002617.html The question is: does anyone uses rc_fast_and_loose? It seems to be undocumented and not referenced in any scripts and/or manuals. There are at least two ways of proceeding: fix rc.d scripts to work with fast_and_loose and just to eliminate it from rc.subr, so it will be good to know if the second way won't hurt anyone. Thanks. -- Eygene Ryabinkin,,,^..^,,, [ Life's unfair - but root password helps! | codelabs.ru ] [ 82FE 06BC D497 C0DE 49EC 4FF0 16AF 9EAE 8152 ECFB | freebsd.org ] pgpP3Gj4wQ9va.pgp Description: PGP signature
