Assert fail in Perl_magic_get without other symptoms - '{tell$0;i${^LAST_FH}}' #15372
Comments
From @dcollinsnGreetings Porters, I have compiled bleadperl with the afl-gcc compiler using: ./Configure -Dusedevel -Dprefix='/usr/local/perl-afl' -Dcc='ccache afl-gcc' -Uuselongdouble -Duse64bitall -Doptimize=-g -Uversiononly -Uman1dir -Uman3dir -Dusequadmath -des And then fuzzed the resulting binary using: AFL_NO_VAR_CHECK=1 afl-fuzz -i in -o out bin/perl @@ After reducing testcases using `afl-tmin` and performing additional minimization by hand, I have located the following testcase that triggers an assert fail in debug buids of the perl interpreter. The testcase is the file below. On normal builds, this runs normally. On debug builds, this returns an assert fail. dcollins@nightshade64:~/perldebug$ ./perl -Ilib -e '{tell$0;i${^LAST_FH}}' Debugging tool output is below. A git bisect was performed and reported the following, which is presumably the commit where the assert was initially added. 8561ea1 is the first bad commit ${^LAST_FH} This was brought up in ticket #96672. This variable gives access to the last-read filehandle that Perl uses :100644 100644 55666f44cc99fb578eb299e45dc6c2b534094a96 08d41db074a06015b595273e07c4e11603544f22 M gv.c **GDB** (gdb) run Program received signal SIGABRT, Aborted. **VALGRIND** No reported memory management errors. **PERL -V** dcollins@nightshade64:~/perldebug$ ./perl -Ilib -V Characteristics of this binary (from libperl): |
From @cpansproutOn Thu May 26 19:24:25 2016, dcollinsn@gmail.com wrote:
Here is a clearer test case: $ ./perl -Ilib -e 'tell Without the method call, it doesn’t fail. -- Father Chrysostomos |
|
The RT System itself - Status changed from 'new' to 'open' |
From @tonycozOn Thu May 26 19:44:50 2016, sprout wrote:
It's pretty much any reference to The attached fixes it for me. Tony |
From @tonycoz0001-perl-128263-don-t-leave-LAST_FH-as-a-non-GV.patchFrom 3dff64555e9806caef34ea3fbe92d3734c268615 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Thu, 11 Aug 2016 11:37:07 +1000
Subject: (perl #128263) don't leave ${^LAST_FH} as a non-GV
---
doio.c | 7 ++++++-
t/op/magic.t | 11 ++++++++++-
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/doio.c b/doio.c
index 2dc7082..241534a 100644
--- a/doio.c
+++ b/doio.c
@@ -1151,8 +1151,10 @@ Perl_do_eof(pTHX_ GV *gv)
PERL_ARGS_ASSERT_DO_EOF;
- if (!io)
+ if (!io) {
+ PL_last_in_gv = NULL;
return TRUE;
+ }
else if (IoTYPE(io) == IoTYPE_WRONLY)
report_wrongway_fh(gv, '>');
@@ -1199,6 +1201,7 @@ Perl_do_tell(pTHX_ GV *gv)
if (io && (fp = IoIFP(io))) {
return PerlIO_tell(fp);
}
+ PL_last_in_gv = NULL;
report_evil_fh(gv);
SETERRNO(EBADF,RMS_IFI);
return (Off_t)-1;
@@ -1213,6 +1216,7 @@ Perl_do_seek(pTHX_ GV *gv, Off_t pos, int whence)
if (io && (fp = IoIFP(io))) {
return PerlIO_seek(fp, pos, whence) >= 0;
}
+ PL_last_in_gv = NULL;
report_evil_fh(gv);
SETERRNO(EBADF,RMS_IFI);
return FALSE;
@@ -1235,6 +1239,7 @@ Perl_do_sysseek(pTHX_ GV *gv, Off_t pos, int whence)
return PerlLIO_lseek(fd, pos, whence);
}
}
+ PL_last_in_gv = NULL;
report_evil_fh(gv);
SETERRNO(EBADF,RMS_IFI);
return (Off_t)-1;
diff --git a/t/op/magic.t b/t/op/magic.t
index f8c822b..79b4559 100644
--- a/t/op/magic.t
+++ b/t/op/magic.t
@@ -5,7 +5,7 @@ BEGIN {
chdir 't' if -d 't';
@INC = '../lib';
require './test.pl';
- plan (tests => 192);
+ plan (tests => 196);
}
# Test that defined() returns true for magic variables created on the fly,
@@ -642,6 +642,15 @@ is ${^LAST_FH}, \*STDIN, '${^LAST_FH} after another tell';
# This also tests that ${^LAST_FH} is a weak reference:
is ${^LAST_FH}, undef, '${^LAST_FH} is undef when PL_last_in_gv is NULL';
+# all of these would set PL_last_in_gv to a non-GV which would
+# assert when referenced by the magic for ${^LAST_FH}.
+# Instead it should act like <$0> which NULLs PL_last_in_gv and the magic
+# returns that as undef.
+for my $code ('tell $0', 'sysseek $0, 0, 0', 'seek $0, 0, 0', 'eof $0') {
+ fresh_perl_is("$code; print defined \${^LAST_FH} ? qq(not ok\n) : qq(ok\n)", "ok\n",
+ undef, "check $code doesn't define \${^LAST_FH}");
+}
+
# $|
fresh_perl_is 'print $| = ~$|', "1\n", {switches => ['-l']},
--
2.1.4
|
From @cpansproutOn Wed Aug 10 18:40:21 2016, tonyc wrote:
Before$ ./miniperl -le '*$0; tell I would have expected the rv2gv op to prevent PL_last_in_gv from being set to a non-glob. $ ./perl -Ilib -MO=Concise -e 'tell $0' But rv2gv can sometimes return &PL_sv_undef. To avoid a change in behaviour, I think a better fix would be to use if (PL_last_in_gv && PL_last_in_gv != &PL_sv_undef) { in mg.c (and to leave the assertion as-is, since having PL_last_in_gv set to anything else is a bug we want to be alerted about). -- Father Chrysostomos |
From @tonycozOn Wed Aug 10 19:50:01 2016, sprout wrote:
With that change (with a cast to make the pointers the same type), PL_last_in_gv = MUTABLE_GV(*PL_stack_sp--); Tony |
From @cpansproutOn Mon Sep 05 21:34:29 2016, tonyc wrote:
Aha! I’m not consistent, am I? commit 84ee769 pp_readline: Don’t set PL_last_in_gv to &PL_sv_undef Yes, I do think 84ee769 can be reverted (except for the test). It did not occur to me at the time that many other ops are likewise affected, and that the much cleaner approach is to check in just one spot in mg.c. -- Father Chrysostomos |
From @tonycozOn Mon, 05 Sep 2016 21:50:43 -0700, sprout wrote:
Something like the attached, I presume. Tony |
From @tonycoz0001-perl-128263-handle-PL_last_in_gv-being-PL_sv_undef.patchFrom eb5f9c4a74fe61c3fcff32dbabac036d8c37588d Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Thu, 9 Feb 2017 15:47:48 +1100
Subject: (perl #128263) handle PL_last_in_gv being &PL_sv_undef
rv2gv will return &PL_sv_undef when it can't get a GV, previously
this could cause an assertion failure in mg.c
My original fix for this changed each op that deals with GVs for I/O
to set PL_last_in_gv to NULL if there was no io object in the GV, but
this changes other behaviour as noted by FatherC.
Also partly reverts 84ee769f, which did the
---
mg.c | 2 +-
pp_hot.c | 5 +----
t/op/magic.t | 12 +++++++++++-
3 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/mg.c b/mg.c
index 172127c..bddde19 100644
--- a/mg.c
+++ b/mg.c
@@ -951,7 +951,7 @@ Perl_magic_get(pTHX_ SV *sv, MAGIC *mg)
break;
case '\014': /* ^LAST_FH */
if (strEQ(remaining, "AST_FH")) {
- if (PL_last_in_gv) {
+ if (PL_last_in_gv && (SV*)PL_last_in_gv != &PL_sv_undef) {
assert(isGV_with_GP(PL_last_in_gv));
SV_CHECK_THINKFIRST_COW_DROP(sv);
prepare_SV_for_RV(sv);
diff --git a/pp_hot.c b/pp_hot.c
index a3ee2a7..ea4f01e 100644
--- a/pp_hot.c
+++ b/pp_hot.c
@@ -445,10 +445,7 @@ PP(pp_readline)
PUTBACK;
Perl_pp_rv2gv(aTHX);
PL_last_in_gv = MUTABLE_GV(*PL_stack_sp--);
- if (PL_last_in_gv == (GV *)&PL_sv_undef)
- PL_last_in_gv = NULL;
- else
- assert(isGV_with_GP(PL_last_in_gv));
+ assert((SV*)PL_last_in_gv == &PL_sv_undef || isGV_with_GP(PL_last_in_gv));
}
}
return do_readline();
diff --git a/t/op/magic.t b/t/op/magic.t
index 3f71f8e..36abafb 100644
--- a/t/op/magic.t
+++ b/t/op/magic.t
@@ -5,7 +5,7 @@ BEGIN {
chdir 't' if -d 't';
require './test.pl';
set_up_inc( '../lib' );
- plan (tests => 192); # some tests are run in BEGIN block
+ plan (tests => 196); # some tests are run in BEGIN block
}
# Test that defined() returns true for magic variables created on the fly,
@@ -643,6 +643,16 @@ is ${^LAST_FH}, \*STDIN, '${^LAST_FH} after another tell';
# This also tests that ${^LAST_FH} is a weak reference:
is ${^LAST_FH}, undef, '${^LAST_FH} is undef when PL_last_in_gv is NULL';
+# all of these would set PL_last_in_gv to a non-GV which would
+# assert when referenced by the magic for ${^LAST_FH}.
+# Instead it should act like <$0> which NULLs PL_last_in_gv and the magic
+# returns that as undef.
+# The approach to fixing this has changed (#128263), but it's still useful
+# to check each op.
+for my $code ('tell $0', 'sysseek $0, 0, 0', 'seek $0, 0, 0', 'eof $0') {
+ fresh_perl_is("$code; print defined \${^LAST_FH} ? qq(not ok\n) : qq(ok\n)", "ok\n",
+ undef, "check $code doesn't define \${^LAST_FH}");
+}
# $|
fresh_perl_is 'print $| = ~$|', "1\n", {switches => ['-l']},
--
2.1.4
|
From @tonycozOn Wed, 10 Aug 2016 18:40:21 -0700, tonyc wrote:
Applied as 745e740, fixing the incomplete sentence at the end. Tony |
|
@tonycoz - 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#128263 (status was 'resolved')
Searchable as RT128263$
The text was updated successfully, but these errors were encountered: