perl: sv.c:12530: void Perl_sv_vcatpvfn_flags() Assertion '0' failed. #16172
Comments
From @geeknikTriggered while fuzzing v5.27.4-28-g60dfa51. ./perl -e '$p00="[\0\\N{U+.}";qr/$p00/' perl: sv.c:12530: void Perl_sv_vcatpvfn_flags(SV *const, const char |
From @tonycozOn Mon, 25 Sep 2017 16:21:55 -0700, brian.carpenter@gmail.com wrote:
Backtrace: ... which looks embarassing similar to 131598. Tony |
|
The RT System itself - Status changed from 'new' to 'open' |
From @khwilliamsonOn 09/25/2017 06:44 PM, Tony Cook via RT wrote:
But it isn't the same cause. I haven't had lately and won't have time |
From @atoomicI agree with Tony Cook this is a problem in REPORT_LOCATION_ARGS, 1/ minor, we should not use vFAIL2 as the second arg is a 'N', the vFAIL is probably more appropriate there. 2/ I confirm that using FAIL or FAIL2 instead of the vFAIL family fixes the issue I think the problem is to use a negative offset for the length in REPORT_LOCATION_ARGS (view attached patch) note, as it a few tests are failing from re/regexp.t & co, can adjust them, but want to confirm that the suggested patch is correct first On Mon, 25 Sep 2017 21:48:24 -0700, public@khwilliamson.com wrote:
|
From @atoomic0001-Fixup-REPORT_LOCATION_ARGS-to-use-positive-length.patchFrom 770dec97adda5c2b37e1f647747a9d1c779cc289 Mon Sep 17 00:00:00 2001
From: Nicolas R <atoomic@cpan.org>
Date: Tue, 26 Sep 2017 12:26:15 -0500
Subject: [PATCH] Fixup REPORT_LOCATION_ARGS to use positive length
Use vFAIL instead of vFAIL2, and adjust the length
when the offset is negative.
RT-132163
---
regcomp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/regcomp.c b/regcomp.c
index cc0ff96064..38b0fa9e59 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -628,7 +628,7 @@ static const scan_data_t zero_scan_data = {
UTF8fARG(UTF, \
(xI(xC) > eC) /* Don't run off end */ \
? eC - sC /* Length before the <--HERE */ \
- : xI_offset(xC), \
+ : ( xI_offset(xC) > 0 ? xI_offset(xC) : 0 ), \
sC), /* The input pattern printed up to the <--HERE */ \
UTF8fARG(UTF, \
(xI(xC) > eC) ? 0 : eC - xI(xC), /* Length after <--HERE */ \
@@ -12056,7 +12056,7 @@ S_grok_bslash_N(pTHX_ RExC_state_t *pRExC_state,
endbrace = strchr(RExC_parse, '}');
if (! endbrace) { /* no trailing brace */
- vFAIL2("Missing right brace on \\%c{}", 'N');
+ vFAIL("Missing right brace on xyz \\N{}");
}
else if(!(endbrace == RExC_parse /* nothing between the {} */
|| (endbrace - RExC_parse >= 2 /* U+ (bad hex is checked... */
--
2.14.2
|
From @atoomicOops, my bad, my previous patch was not clean, which explains why a few tests were failing, this is much better once updated like this. Only porting/diag.t is failing one test View updated patch version attached to this message. On Tue, 26 Sep 2017 10:31:43 -0700, atoomic wrote:
|
From @atoomic0001-Fixup-REPORT_LOCATION_ARGS-to-use-positive-length.patchFrom a43682a478801430f81e9982cd9b7312410fc421 Mon Sep 17 00:00:00 2001
From: Nicolas R <atoomic@cpan.org>
Date: Tue, 26 Sep 2017 12:26:15 -0500
Subject: [PATCH] Fixup REPORT_LOCATION_ARGS to use positive length
Use vFAIL instead of vFAIL2, and adjust the length
when the offset is negative.
RT-132163
---
regcomp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/regcomp.c b/regcomp.c
index cc0ff96064..b8233ac699 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -628,7 +628,7 @@ static const scan_data_t zero_scan_data = {
UTF8fARG(UTF, \
(xI(xC) > eC) /* Don't run off end */ \
? eC - sC /* Length before the <--HERE */ \
- : xI_offset(xC), \
+ : ( xI_offset(xC) > 0 ? xI_offset(xC) : 0 ), \
sC), /* The input pattern printed up to the <--HERE */ \
UTF8fARG(UTF, \
(xI(xC) > eC) ? 0 : eC - xI(xC), /* Length after <--HERE */ \
@@ -12056,7 +12056,7 @@ S_grok_bslash_N(pTHX_ RExC_state_t *pRExC_state,
endbrace = strchr(RExC_parse, '}');
if (! endbrace) { /* no trailing brace */
- vFAIL2("Missing right brace on \\%c{}", 'N');
+ vFAIL("Missing right brace on \\N{}");
}
else if(!(endbrace == RExC_parse /* nothing between the {} */
|| (endbrace - RExC_parse >= 2 /* U+ (bad hex is checked... */
--
2.14.2
|
From @atoomicsorry for the spam and extra message, took me some time to understood how that porting/diag.t test was designed nicolas On Mon, 25 Sep 2017 17:44:06 -0700, tonyc wrote:
|
From @atoomic0001-Fixup-REPORT_LOCATION_ARGS-to-use-positive-length.patchFrom 35b8492b237191408cd989bd19f2ea996e4b7ee3 Mon Sep 17 00:00:00 2001
From: Nicolas R <atoomic@cpan.org>
Date: Tue, 26 Sep 2017 12:26:15 -0500
Subject: [PATCH] Fixup REPORT_LOCATION_ARGS to use positive length
Use vFAIL instead of vFAIL2, and adjust the length
when the offset is negative.
RT-132163
---
pod/perldiag.pod | 2 ++
regcomp.c | 4 ++--
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/pod/perldiag.pod b/pod/perldiag.pod
index d417fb296e..dd03913fb7 100644
--- a/pod/perldiag.pod
+++ b/pod/perldiag.pod
@@ -3658,6 +3658,8 @@ L<perlfunc/require EXPR> and L<perlfunc/do EXPR>.
=item Missing right brace on \%c{} in regex; marked by S<<-- HERE> in m/%s/
+=item Missing right brace on \N{} in regex; marked by S<<-- HERE> in m/%s/
+
(F) Missing right brace in C<\x{...}>, C<\p{...}>, C<\P{...}>, or C<\N{...}>.
=item Missing right brace on \N{}
diff --git a/regcomp.c b/regcomp.c
index cc0ff96064..b8233ac699 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -628,7 +628,7 @@ static const scan_data_t zero_scan_data = {
UTF8fARG(UTF, \
(xI(xC) > eC) /* Don't run off end */ \
? eC - sC /* Length before the <--HERE */ \
- : xI_offset(xC), \
+ : ( xI_offset(xC) > 0 ? xI_offset(xC) : 0 ), \
sC), /* The input pattern printed up to the <--HERE */ \
UTF8fARG(UTF, \
(xI(xC) > eC) ? 0 : eC - xI(xC), /* Length after <--HERE */ \
@@ -12056,7 +12056,7 @@ S_grok_bslash_N(pTHX_ RExC_state_t *pRExC_state,
endbrace = strchr(RExC_parse, '}');
if (! endbrace) { /* no trailing brace */
- vFAIL2("Missing right brace on \\%c{}", 'N');
+ vFAIL("Missing right brace on \\N{}");
}
else if(!(endbrace == RExC_parse /* nothing between the {} */
|| (endbrace - RExC_parse >= 2 /* U+ (bad hex is checked... */
--
2.14.2
|
From @atoomicAdd one extra unit test for this case to t/re/re_tests On Tue, 26 Sep 2017 10:56:13 -0700, atoomic wrote:
|
From @atoomic0001-Add-unit-test-for-RT-132163.patchFrom ed25d8f69a6bb1e1bbcac4b452042ddbca7fdef4 Mon Sep 17 00:00:00 2001
From: Nicolas R <atoomic@cpan.org>
Date: Tue, 26 Sep 2017 13:27:57 -0500
Subject: [PATCH] Add unit test for RT #132163
---
t/re/re_tests | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/re/re_tests b/t/re/re_tests
index 0bd9b5541f..7adc000658 100644
--- a/t/re/re_tests
+++ b/t/re/re_tests
@@ -1941,6 +1941,7 @@ A+(*PRUNE)BC(?{}) AAABC y $& AAABC
/w\z\R\z/i \x{100}a\x{80}a n - -
/(a+){1}+a/ aaa n - - # [perl #125825]
+[\0\N{U+.} aaa c - Unmatched [ in regex; marked by <-- HERE in m/\[ <-- HERE \\0\\N{U+.}/ # [perl #132163]
^((?(?=x)xb|ya)z) xbz y $1 xbz
^((?(?=x)xb|ya)z) yaz y $1 yaz
--
2.14.2
|
From @khwilliamsonOn Tue, 26 Sep 2017 10:31:43 -0700, atoomic wrote:
I finally had a good look at this problem. It turns out that the code patch is not the correct fix. It causes valgrind errors when I run it, and there is instead a deeper problem. The patch substitutes 0 when a number is negative. But it turns out that if the number is negative, it means something is terribly wrong. I will submit a patch that asserts against that. I thought I understood why this number is getting negative, but now I realize that I still don't understand that. It turns out that I have been overhauling this area of the code for other reasons, and the most productive path forward is to start with the overhaul that I've done so far. I'll look at the .t patch after that is done. The reason for the vFAIL2 instead of the more obvious vFAIL is so that the existing entry in perldiag would work. Extra entries slow down the process for someone trying to figure out what's going on, and when I wrote the code I thought the tradeoff was worthwhile, to improve the user experience. Since we are about to croak, there's no performance impetus. The other option would have been to add a 'diag listed as' comment there. I try to avoid those. But I could have added an explanation why the non-obvious thing was done. |
From @atoomicusing v5.27.6 (v5.27.5-349-gb9a5a78fe9), I cannot reproduce this SEGV. v5.27.5-349-gb9a5a78fe9> ./perl -e '$p00="[\0\\N{U+.}";qr/$p00/' At this point, I think we should close this ticket. On Sat, 21 Oct 2017 16:23:26 -0700, khw wrote:
|
From @khwilliamsonOn Mon, 13 Nov 2017 14:01:02 -0800, atoomic@cpan.org wrote:
The problem with closing the ticket is that just adding the missing ']' causes it to be buggy in a different way. Compiling REx "[%0\N{U+.}]" There is no '|' in this pattern; and it should fail instead of compiling into the wrong stuff. -- |
From @khwilliamsonThanks for finding this; fixed by |
|
@khwilliamson - Status changed from 'open' to 'pending release' |
From @khwilliamsonThank you for filing this report. You have helped make Perl better. With the release yesterday of Perl 5.28.0, this and 185 other issues have been Perl 5.28.0 may be downloaded via: If you find that the problem persists, feel free to reopen this ticket. |
|
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#132163 (status was 'resolved')
Searchable as RT132163$
The text was updated successfully, but these errors were encountered: