SIGSEGV in hek_eq_pvn_flags #15307
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 a segmentation fault in the perl interpreter. The testcase is the 25-character file: /${*::::::=0}$0{*::::=0}/ On normal and debug builds, this crashes with a segmentation fault. dcollins@nightshade64:~/perl$ ~/perl/perl -e '/${*::::::=0}$0{*::::=0}/' Debugging tool output is below. I don't think this has been reported yet, strategic searches for "S_mro_gather_and_rename" don't seem to yield any past reports. It's dereferencing something that isn't a pointer. **GDB** dcollins@nightshade64:~/perl$ gdb --args ~/perl/perl -e '/${*::::::=0}$0{*::::=0}/' Program received signal SIGSEGV, Segmentation fault. **VALGRIND** dcollins@nightshade64:~/perl$ valgrind ~/perl/perl -e '/${*::::::=0}$0{*::::=0}/' **PERL -V** dcollins@nightshade64:~/perl$ ./perl -Ilib -V Characteristics of this binary (from libperl): |
From @hvdsThis appears to be a precedence error, fixable with an extra pair of parens as below. I don't understand this area well though, and I'd like to add a test that's less obscure than the reported test case. Any suggestions? Hugo Inline Patchdiff --git a/hv.c b/hv.c
index 7b5ad95..5523475 100644
--- a/hv.c
+++ b/hv.c
@@ -2476,9 +2476,10 @@ Perl_hv_ename_delete(pTHX_ HV *hv, const char *name, U32
return;
}
if (
- count > 0 && (HEK_UTF8(*namep) || (flags & SVf_UTF8))
+ count > 0 && ((HEK_UTF8(*namep) || (flags & SVf_UTF8))
? hek_eq_pvn_flags(aTHX_ *namep, name, (I32)len, flags)
: (HEK_LEN(*namep) == (I32)len && memEQ(HEK_KEY(*namep), name, l
+ )
) {
aux->xhv_name_count = -count;
} |
|
The RT System itself - Status changed from 'new' to 'open' |
From @cpansproutOn Tue May 10 01:00:01 2016, hv wrote:
I think there may be more to this bug than meets the eye. If I add the debugging code in the attached patch, I get this: SV = PVHV(0x7fda9280aec0) at 0x7fda9282a938 etc. ENAME should never have null in it. That’s a bug, possibly elsewhere. I need to dig deeper. -- Father Chrysostomos |
From @cpansproutInline Patchdiff --git a/hv.c b/hv.c
index 7b5ad95..697a0aa 100644
--- a/hv.c
+++ b/hv.c
@@ -2478,7 +2478,7 @@ Perl_hv_ename_delete(pTHX_ HV *hv, const char *name, U32 len, U32 flags)
if (
count > 0 && (HEK_UTF8(*namep) || (flags & SVf_UTF8))
? hek_eq_pvn_flags(aTHX_ *namep, name, (I32)len, flags)
- : (HEK_LEN(*namep) == (I32)len && memEQ(HEK_KEY(*namep), name, len))
+ : (*namep || (Perl_sv_dump(aTHX_ (SV*)hv),0), (HEK_LEN(*namep) == (I32)len && memEQ(HEK_KEY(*namep), name, len)))
) {
aux->xhv_name_count = -count;
} |
From @cpansproutOn Tue May 10 21:13:32 2016, sprout wrote:
This is the simplest test case I can come up with: %h; *::::::=*h; delete $::{"::"} It has to do with two code paths dividing up and reconstructing names like :::::: differently. Perl keeps track of all the effective names of a stash, if it gets aliased or reassigned. That is what HvENAME is for, whereas HvNAME is the original, canonical name of the stash. In the short case above, %h must be vivified first, before the assignment (any compile-time mention will work). Then, when the assignment happens, it gets the *effective* name :::: assigned to it. Internally, a stash may have a list of names. If the canonical name (the first name in the list) is not one of the effective names, then the name count is negative, to indicate that. After the assignment above, we have: NULL, "::::" (with a name count of -2) such that HvNAME is NULL, but HvENAME is a hek containing "::::". The count>0 check in hv_ename_delete protects against reading that null, but this is where the precedence problem prevents it and causes a crash. I think you can go ahead and apply your fix, for this reason.
Actually two bugs. First, *::::=anything is assigning the empty string as a name to whatever hash is put there. Secondly, dump.c is dumping the empty string as (null), which should only be used for a null pointer. (The null-handling code was added when ename supported was not partially written and nulls were appearing in the array due to bugs elsewhere that were fixed before the code was merged.) -- Father Chrysostomos |
From @cpansproutOn Sat May 14 10:24:41 2016, sprout wrote:
I have applied your patch in 60a26c7. Thank you. I have applied the test in 7f1bd06.
I have fixed dump.c in 8e5993c. As for the discrepancy in how :::: names are parsed, I think we can leave things as they are unless some other real bug shows up as a result. I am marking this ticket as resolved. -- Father Chrysostomos |
|
@cpansprout - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#128086 (status was 'resolved')
Searchable as RT128086$
The text was updated successfully, but these errors were encountered: