Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upAssert fail in Perl_sv_2pv_flags: '(vec%0,0,0)=0' #15370
Comments
This comment has been minimized.
This comment has been minimized.
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 (albeit with an expected warning). On debug builds, this returns an assert fail. dcollins@nightshade64:~/perl$ ./perl -Ilib -e '(vec%0,0,0)=0' Debugging tool output is below. A git bisect was performed and reported the following, which appears to be when the assert was initially added. 217f6fa is the first bad commit sv.c: Assert that sv_[ivp]v are not passed aggregates The lack of assertions can hide bugs. See 32a6097 for instance :100644 100644 3ac0a2bce83bc12406be1fa22acefa8f8a2014c7 daa87f00081d96c8d15f94f02c32cd85e8af0266 M sv.c **GDB** dcollins@nightshade64:~/perldebug$ gdb --args ./miniperl -Ilib -e '((vec%0,0,0)=0)' 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): |
This comment has been minimized.
This comment has been minimized.
From @cpansproutOn Thu May 26 18:52:05 2016, dcollinsn@gmail.com wrote:
That assertion was added specifically to find bugs like this: $ perl -we '(vec%0,0,1)=0' That output is nonsensical. Thank you for finding one. :-) -- Father Chrysostomos |
This comment has been minimized.
This comment has been minimized.
The RT System itself - Status changed from 'new' to 'open' |
This comment has been minimized.
This comment has been minimized.
From @cpansproutOn Thu May 26 21:38:50 2016, sprout wrote:
A related issue is that prototype("CORE::vec") returns '$$$', but vec’s first argument must be a valid lvalue, so the prototype is wrong: $ perl -we '(vec $_+1,0,1)=0' It’s prototype currently is effectively ‘\[$@%*]$$’, but it should probably be ‘\$$$’. Also, those ‘Can't modify’ messages cite the wrong op (list/scalar assignment instead of vec). -- Father Chrysostomos |
This comment has been minimized.
This comment has been minimized.
From @cpansproutOn Thu May 26 21:58:20 2016, sprout wrote:
It’s more complicated than that. vec does accept $_+1 if vec itself is in rvalue context, so its actual behaviour cannot be represented perfectly with a prototype.
That’s actually a clue to the problem. Scalar assignment dies at compile time, but list assignment goes on to do the wrong thing (and fail an assertion) at run time, because vec’s own type of lvalue context is being propagated, whereas it probably should not be. $ perl -we 'vec(%a,0,1)=0' vec is not the only problematic operator. $ ./perl -we '(substr %a,0,1)=0' The second one dies at compile time; the first one does the wrong thing at run time. Same problem. -- Father Chrysostomos |
This comment has been minimized.
This comment has been minimized.
From @cpansproutOn Fri May 27 17:55:20 2016, sprout wrote:
Should assignment to substr(keys) be allowed? $ ./perl -le 'substr(keys %h,1) = 3; print "ok"' (The logic in op.c:op_lvalue_flags is really broken.) Currently assignment to substr(keys) actually does something: $ ./perl -le 'substr(keys %h,1) = 3; use Devel::Peek; Dump %h' (See the MAX line, indicating the highest bucket index.) I just want to know if I should go ahead and break that, which would be the easiest way to fix the other bugs in this ticket. -- Father Chrysostomos |
This comment has been minimized.
This comment has been minimized.
From @cpansproutOn Thu Jun 09 07:03:06 2016, sprout wrote:
Duh. Ignore the ‘ok’. (Shouldn’t reduce scripts *after* copying and pasting them!) -- Father Chrysostomos |
This comment has been minimized.
This comment has been minimized.
From @ikegamiOn Thu, Jun 9, 2016 at 10:03 AM, Father Chrysostomos via RT <
Is there a reason it shouldn't? substr(keys)=... isn't too useful, but vec(substr)=... might be useful (I'm assuming substr/keys/vec/$#a are interchangeable in that question, as |
This comment has been minimized.
This comment has been minimized.
From @cpansproutOn Thu Jun 09 13:31:21 2016, ikegami@adaelis.com wrote:
The fix I have in my head would allow vec(substr)=....
keys() is extra extra special. keys() assignment only works in scalar assignment, not list assignment. The whole thing is a kludge, really. :-) -- Father Chrysostomos |
This comment has been minimized.
This comment has been minimized.
From @cpansproutOn Thu Jun 09 18:21:57 2016, sprout wrote:
Hrm, I did not realize this, but foreach(scalar keys %h) gives a magic keys scalar as do foo(scalar keys %h), \scalar keys %h, and grep $_, scalar keys %h. (keys %h) = is not permitted though. Maybe it’s because that is list context. That seems the only logical reason. Only a scalar-context keys can give a magic assignable scalar. I guess I need to make substr(keys)= work. Now that I understand this code better, it shouldn’t be too hard. -- Father Chrysostomos |
This comment has been minimized.
This comment has been minimized.
From @cpansproutOn Thu Jun 09 18:31:45 2016, sprout wrote:
keys(%h) works in some lvalue scalar contexts, but not others: $ ./perl -Ilib -e 'keys(%h).="00"' keys has its own list of exceptions for the lvalue type. If I were to make it use the same logic as other ops that make a distinction between scalar and list lvalue context (by calling S_scalar_mod_type), then it would be easier not to break it when fixing other bugs, and I could stop worrying about it. The .= and read examples above would start working, which would be good (though no sane code would do that) since it would be more predictable. -- Father Chrysostomos |
This comment has been minimized.
This comment has been minimized.
From @iabynOn Thu, Jun 09, 2016 at 10:51:12PM -0700, Father Chrysostomos via RT wrote:
So would substr(keys %h, ...) = foo; be equivalent to: my $k = keys %h; -- |
This comment has been minimized.
This comment has been minimized.
From @cpansproutOn Fri Jun 10 01:22:52 2016, davem wrote:
It already is. :-) -- Father Chrysostomos |
This comment has been minimized.
This comment has been minimized.
From @demerphqOn 10 June 2016 at 17:31, Father Chrysostomos via RT
So when or if we switch our hash implementation to something where I really feel like this goes in the wrong direction. We should be Yves |
This comment has been minimized.
This comment has been minimized.
From @cpansproutOn Fri Jun 10 10:23:31 2016, demerphq wrote:
Aha. I thought you were going to speak up along those lines. Until we actually remove keys assignment, I have no intention of accidentally breaking it. By making it share code with other ops, instead of having its own special cases (and this sharing has the side effect of allowing .= etc.), I can stop worrying about it and get on with fixing other bugs. As for what keys(%h)= should do if we want to change that, I have no idea. Nor do I have much interest. All I am doing is changing the code surrounding the *syntax*, not the actual implementation of keys lvalues, in order to make it easier to fix other syntax bugs, such as those discussed earlier in this ticket. Make sense? -- Father Chrysostomos |
This comment has been minimized.
This comment has been minimized.
From @cpansproutOn Fri Jun 10 13:06:29 2016, sprout wrote, in response to Yves Orton:
I do hope you respond soon. I have a branch waiting to be merged (smoke-me/128260), but I won’t merge it if there are still objections. -- Father Chrysostomos |
This comment has been minimized.
This comment has been minimized.
From @ap* demerphq <demerphq@gmail.com> [2016-06-10 19:24]:
If assignment to keys() really ought to become a no-op, then it should
I think it’s going in the right direction as well as the wrong direction However, I disagree with the premise. The specific notion of “allocate Regards, |
This comment has been minimized.
This comment has been minimized.
From @demerphqSorry. Yes i understand now. Please go ahead, i just get grumpy about this On Fri Jun 10 13:06:29 2016, sprout wrote, in response to Yves Orton:
I do hope you respond soon. I have a branch waiting to be merged -- Father Chrysostomos via perlbug: queue: perl5 status: open |
This comment has been minimized.
This comment has been minimized.
From @demerphqOn 11 Jun 2016 05:35, "Aristotle Pagaltzis" <pagaltzis@gmx.de> wrote:
While i have sympathy about the point about back compat and guarding i If things were done carefully you would be able to write new style code conf_hash (%hash, min_keys => 1000, some_new_prop => 5); Which on older perls would be the same as assigning 1000 to keys, and on
While i concede that some of the algorithms we might choose could have such Making all this explicit and future back compat proof seems to me to be I will try to find time soon to start a fresh thread that summarizes my Yves |
This comment has been minimized.
This comment has been minimized.
From @cpansproutOn Sat Jun 11 01:29:31 2016, demerphq wrote:
Now pushed. e4fc708 makes keys consistent with other ops with regard to list vs scalar lvalue context. 1a3e972 is a refactoring to prepare for the following commit. 79409ac fixes the assertion failure originally reported in this ticket, making it a compile-time error, and also fixes the wording in the error messages to say ‘in vec’ and ‘in substr’.
In fact, it has never really made much sense. keys %h = keys %h ought to be a no-op, but keys assignment specifies *buckets* whereas keys returns *keys*. If keys %h = 200 simply hinted that there would be about 200 keys soon, it would not be an abstraction violation, would it? -- Father Chrysostomos |
This comment has been minimized.
This comment has been minimized.
@cpansprout - Status changed from 'open' to 'resolved' |
This comment has been minimized.
This comment has been minimized.
@cpansprout - Status changed from 'resolved' to 'pending release' |
This comment has been minimized.
This comment has been minimized.
From @demerphqOn 11 June 2016 at 16:27, Father Chrysostomos via RT
Yeah, I agree. Syntactically it is very odd.
Effectively that is what it does. With our current implementation it And I suppose you are right, if you think of it as a hint of that sort I guess we don't have to get rid of it, we can just add something new Yves -- |
This comment has been minimized.
This comment has been minimized.
From @cpansproutOn Sat Jun 11 07:27:38 2016, sprout wrote:
Except I fell into the same trap as the original bug, and that is to allow an HV or AV to pass to SV-only functions, by still propagating the outer lvalue context for \substr %h. I fixed that in 33a1032. -- Father Chrysostomos |
This comment has been minimized.
This comment has been minimized.
From @khwilliamsonThank you for filing this report. You have helped make Perl better. With the release today of Perl 5.26.0, this and 210 other issues have been Perl 5.26.0 may be downloaded via: If you find that the problem persists, feel free to reopen this ticket. |
This comment has been minimized.
This comment has been minimized.
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#128260 (status was 'resolved')
Searchable as RT128260$