Replace some sv_2mortal() instances with more direct equivalents #23153
Conversation
| @@ -4189,7 +4189,8 @@ Perl_do_readline(pTHX) | |||
| } | |||
| else { | |||
| /* XXX on RC builds, push on stack rather than mortalize ? */ | |||
| sv = sv_2mortal(newSV(80)); | |||
| sv = newSV_type(SVt_PV); | |||
| sv_grow_fresh(sv, 81); | |||
There was a problem hiding this comment.
What end the life of the sv head now that mortal is missing?
Why 81 bytes when old was 80?
There was a problem hiding this comment.
What end the life of the sv head now that mortal is missing?
Thanks, it was meant to be newSV_type_mortal, I'll change that now.
Why 81 bytes when old was 80?
It's to keep the allocation size consistent: Perl_newSV calls sv_grow_fresh(sv, len + 1);.
There was a problem hiding this comment.
Why 81 bytes when old was 80?
It's to keep the allocation size consistent:
Perl_newSVcallssv_grow_fresh(sv, len + 1);.
Be very very careful and single step it line by line if your making assumptions about +1s and perl's PV/NewX API. This class of bugs already is sitting in stable and blead perl. "can't introduce an exploit" "safe than sorry" +1 +1, another 16 or 32 bytes just disappeared times a couple 1000. All these +1s and the PERL_STR_RNDUP macro are very fragile. Open PR with this class of bug
There was a problem hiding this comment.
update:
the off by 1 in
goes back to your commit at
64b4056614429bb6fc7d35118e19a220459358fd
I suggest you single step the C code and verify what integer from the interp is entering glibc.so func since you/nobody else (including me) understands this area of the SVPV API anymore. Short made up demo math of the details in #23153
80/8=10, aligned!
(81+7)&(~7)=88
(81+15)&(~15)=96
Atleast use bytes 82-95 on a PP C level if you paided for them from libc, but this 1+1+15 then !0xF mask over and ever. needs to be stopped.
There was a problem hiding this comment.
The +1 is done by the normal Perl_sv_grow() for the default COW build as well, sv_grow_fresh simply duplicates that.
That said, the exact value doesn't matter too much.
richardleach
force-pushed
the
unnecessary_sv2mortal
branch
from
89e85e9 to
981d0f7
Compare
A few commits making this sort of change:
sv_2mortal(newSVpvn_utf8(SvPVX(sv), namelen, do_utf8));->
newSVpvn_flags(SvPVX(sv), namelen, SVs_TEMP|do_utf8);newSVpvn_utf8(key, klen, TRUE);+sv_2mortal()->
newSVpvn_flags(key, klen, SVf_UTF8|SVs_TEMP);(AV *) sv_2mortal((SV *) newAV())->
MUTABLE_AV(newSV_type_mortal(SVt_PVAV));The changes are intended to be functionally equivalent but do the work
through one function call rather than two.