Discussion:
bug#25390: Segfault with sed 4.3
(too old to reply)
Assaf Gordon
2017-01-08 17:01:11 UTC
Permalink
Raw Message
Hello,
I have a reliable segfault with (vanilla) sed 4.3 which does not appear
on (vanilla) 4.2.2.
Thank you for the report!
I can confirm the segfault is reproducible.
The immediate cause is somewhere in gnulib's DFA module.

A shorter example:

printf '$LINENO $LINEN\nB\n' | sed -e 'N;s/\$LINENO\(.*\n\)/\1/'

====
$ printf '$LINENO $LINEN\nB\n' > in.txt
$ printf '%s\n' 'N;s/\$LINENO\(.*\n\)/\1/' > prog.sed
$ gdb ./sed/sed
(gdb) r -f prog.sed in.txt
Starting program: /home/gordon/projects/sed/sed/sed -f prog.sed in.txt

Program received signal SIGSEGV, Segmentation fault.
0x0000000000412384 in dfaexec_main (d=0x6250b0, begin=0x623b50 "$LINENO $LINEN\nB\n", end=0x623b60 "\n",
allow_nl=true, count=0x0, multibyte=false) at lib/dfa.c:3169
3169 s1 = t[*p++];
(gdb) bt
#0 0x0000000000412384 in dfaexec_main (d=0x6250b0, begin=0x623b50 "$LINENO $LINEN\nB\n",
end=0x623b60 "\n", allow_nl=true, count=0x0, multibyte=false) at lib/dfa.c:3169
#1 0x0000000000412833 in dfaexec_sb (d=0x6250b0, begin=0x623b50 "$LINENO $LINEN\nB\n",
end=0x623b60 "\n", allow_nl=true, count=0x0, backref=0x7fffffffbff7) at lib/dfa.c:3266
#2 0x00000000004128a5 in dfaexec (d=0x6250b0, begin=0x623b50 "$LINENO $LINEN\nB\n", end=0x623b60 "\n",
allow_nl=true, count=0x0, backref=0x7fffffffbff7) at lib/dfa.c:3287
#3 0x0000000000409359 in match_regex (regex=0x623c10, buf=0x623b50 "$LINENO $LINEN\nB\n", buflen=16,
buf_start_offset=0, regarray=0x61ff10 <regs>, regsize=2) at sed/regexp.c:345
#4 0x0000000000407859 in do_subst (sub=0x622500) at sed/execute.c:1030
#5 0x00000000004086d4 in execute_program (vec=0x6224d0, input=0x7fffffffe170) at sed/execute.c:1517
#6 0x0000000000408abc in process_files (the_program=0x6224d0, argv=0x7fffffffe3c0) at sed/execute.c:1687
#7 0x0000000000409d88 in main (argc=4, argv=0x7fffffffe3a8) at sed/sed.c:377
===


Looking into it, hope to have fix soon.


regards,
- assaf
Paul Eggert
2017-01-08 20:49:42 UTC
Permalink
Raw Message
Post by Assaf Gordon
The immediate cause is somewhere in gnulib's DFA module.
The bug was introduced in Gnulib, in commit
403adf1b40897ba108075008c10bd38d937e1539 dated 2016-11-25 and labeled "dfa:
addition of new state on demand". It's not a bug that grep runs into, since grep
doesn't use the newline transition that sed does. I installed the attached patch
to fix the Gnulib bug. I'll leave Bug#25390 open, as I assume you'll want to
check it for 'sed' and add a test case for 'sed'.
Norihiro Tanaka
2017-01-09 02:38:40 UTC
Permalink
Raw Message
On Sun, 8 Jan 2017 12:49:42 -0800
Post by Assaf Gordon
The immediate cause is somewhere in gnulib's DFA module.
The bug was introduced in Gnulib, in commit 403adf1b40897ba108075008c10bd38d937e1539
dated 2016-11-25 and labeled "dfa: addition of new state on demand".
It's not a bug that grep runs into, since grep doesn't use the
newline transition that sed does. I installed the attached patch to
fix the Gnulib bug. I'll leave Bug#25390 open, as I assume you'll
want to check it for 'sed' and add a test case for 'sed'.
Thanks for fixing quickly.

I wrote two additional patches for dfa. First, derive number of
allocation from not argument but number of state in transition table
allocation. Second, melt down dfastate() into build_state(). Now, I
think that there do not have to be separated.

I also wrote a simple test, but the issue are not always caused, as it
depends on state of memory. Should we rely to complate the test on
valgrind?
Paul Eggert
2017-01-09 06:12:02 UTC
Permalink
Raw Message
Post by Norihiro Tanaka
I wrote two additional patches for dfa. First, derive number of
allocation from not argument but number of state in transition table
allocation. Second, melt down dfastate() into build_state(). Now, I
think that there do not have to be separated.
Thanks, I installed those two patches into Gnulib.
Post by Norihiro Tanaka
I also wrote a simple test, but the issue are not always caused, as it
depends on state of memory. Should we rely to complate the test on
valgrind?
Yes, I expect the 'sed' folks would prefer the test to use valgrind. You can
look at testsuite/invalid-mb-seq-UMR.sh for an example of a test that does that.
Norihiro Tanaka
2017-01-09 14:04:05 UTC
Permalink
Raw Message
On Sun, 8 Jan 2017 22:12:02 -0800
Post by Paul Eggert
Post by Norihiro Tanaka
I wrote two additional patches for dfa. First, derive number of
allocation from not argument but number of state in transition table
allocation. Second, melt down dfastate() into build_state(). Now, I
think that there do not have to be separated.
Thanks, I installed those two patches into Gnulib.
Thanks.
Post by Paul Eggert
Post by Norihiro Tanaka
I also wrote a simple test, but the issue are not always caused, as it
depends on state of memory. Should we rely to complate the test on
valgrind?
Yes, I expect the 'sed' folks would prefer the test to use valgrind. You can look at testsuite/invalid-mb-seq-UMR.sh for an example of a test that does that.
Thanks, I updated the test. The new test uses valgrind.
Norihiro Tanaka
2017-01-09 14:11:38 UTC
Permalink
Raw Message
On Mon, 09 Jan 2017 23:04:05 +0900
Post by Norihiro Tanaka
Thanks, I updated the test. The new test uses valgrind.
Sorry, I adjusted commit log, New patch does not change
testsuite/Makefile.tests.
Assaf Gordon
2017-01-10 05:27:09 UTC
Permalink
Raw Message
Hello all,
Post by Norihiro Tanaka
Sorry, I adjusted commit log, New patch does not change
testsuite/Makefile.tests.
<0001-tests-new-test-for-dfa-crash-bug.patch>
Paul,
Thank you for the quick fix.

Norihiro,
Thank you for the dfa improvements and sed-tests.

Using your example of:
printf '0123456789abcd\nx\n' | valgrind ./sed/sed 'N;s/0123456789abcd\n//'
I wasn't able to trigger the segfault (or even a valgrind warning) on sed-4.3 / x86_64.

I suggest modifying the input just a bit, making it slightly more similar to the original bug report - with it I'm able to always reproduce the segfault:
printf "abcdefg abcdefg\nB\n" | valgrind ./sed/sed 'N;s/abcdefg.*\n//'

What do you think ?

I'm also considering duplicating the test - once with and once without valgrind.
Is this warranted or an overkill ?

Attach patch contains updated tests (and slightly modified git-comment).
The second commit updates gnulib (comes after adding the tests just temporarily, to make testing before/after gnulib update easier).

comments welcomed,
- assaf
Paul Eggert
2017-01-10 09:11:36 UTC
Permalink
Raw Message
Post by Assaf Gordon
I'm also considering duplicating the test - once with and once without valgrind.
Is this warranted or an overkill ?
Sounds like overkill to me. If it works with valgrind, it'll work without.
Jim Meyering
2017-01-14 23:40:31 UTC
Permalink
Raw Message
Post by Assaf Gordon
Hello all,
Post by Norihiro Tanaka
Sorry, I adjusted commit log, New patch does not change
testsuite/Makefile.tests.
<0001-tests-new-test-for-dfa-crash-bug.patch>
Paul,
Thank you for the quick fix.
Norihiro,
Thank you for the dfa improvements and sed-tests.
printf '0123456789abcd\nx\n' | valgrind ./sed/sed 'N;s/0123456789abcd\n//'
I wasn't able to trigger the segfault (or even a valgrind warning) on sed-4.3 / x86_64.
printf "abcdefg abcdefg\nB\n" | valgrind ./sed/sed 'N;s/abcdefg.*\n//'
What do you think ?
I'm also considering duplicating the test - once with and once without valgrind.
Is this warranted or an overkill ?
Attach patch contains updated tests (and slightly modified git-comment).
The second commit updates gnulib (comes after adding the tests just temporarily, to make testing before/after gnulib update easier).
Hi Assaf,

Thank you for adjusting the tests and commit log. Those look fine.

The only problem is that the new newline-valgrind.sh test would fail
when run against an ASAN-enabled sed. That is because valgrind just
doesn't work when the binary is ASAN-enabled. So I have extemded
init.cfg's require_valgrind_ function so that it also detects this
case and skips the test. I will push the attached shortly, after which
you are welcome to push your commits.
Assaf Gordon
2017-01-17 04:53:08 UTC
Permalink
Raw Message
tag 25390 fixed
close 25390
thanks

Hello,

Thanks again for everyone who reported, fixed and improved sed and dfa/gnulib.


sed's update to latest gnulib is pushed here:
http://git.savannah.gnu.org/cgit/sed.git/commit/?id=44d99bf5c98ea77de0addf55ad7fe281396de996

followed by the new test:
http://git.savannah.gnu.org/cgit/sed.git/commit/?id=0f98f0055e4b505de2696bf862798e88a14956f9


I'm thus closing this bug,
but discussion can continue by replying to this thread.

regards,
- assaf

S. Gilles
2017-01-09 01:17:34 UTC
Permalink
Raw Message
Post by Paul Eggert
Post by Assaf Gordon
The immediate cause is somewhere in gnulib's DFA module.
The bug was introduced in Gnulib, in commit
addition of new state on demand". It's not a bug that grep runs into, since
grep doesn't use the newline transition that sed does. I installed the
attached patch to fix the Gnulib bug. I'll leave Bug#25390 open, as I assume
you'll want to check it for 'sed' and add a test case for 'sed'.
I applied the patch (after some line number tweaks) on top of the 4.3
release tarball, and I can confirm that this fixes the issue for me.
Thanks for the quick fix!
--
S. Gilles
Loading...