Front page | perl.perl5.porters |
Postings from October 2011
Re: Black smoke
From: Brian Fraser
October 23, 2011 16:55
Re: Black smoke
Message ID: CA+nL+nZFKqJaX70huoCQ+4eWP-Qs2+KPmy3RqdpjMYTMYtQE_g@mail.gmail.com
On Sun, Oct 23, 2011 at 5:29 PM, Father Chrysostomos <email@example.com>wrote:
> On Oct 20, 2011, at 12:07 PM, Brian Fraser wrote:
> On Thu, Oct 20, 2011 at 11:11 AM, Father Chrysostomos <firstname.lastname@example.org>wrote:
>> Brian Fraser wrote:
>> > This was fixed by c49a809bf96a6f9d6e8f048bb237650dc7f6ccf9
>> > But now I'm thinking that wasn't the right fix. The previous version
>> > would've handled this:
>> > HvAUX(hv)->xhv_name_count *= -2;
>> > HvENAME_get(hv);
>> > Whereas the new version could end up crashing. I'm not sure if that
>> > acceptable breakage, but if it isn't, does anyone have any objections to
>> > restoring the previous version, but turning the macro into a function?
>> Why are you multiplying xhv_name_count? That does not happen currently.
>> Or did you mean a simple assignment, with xhv_name null? The HvENAME_get
>> macro checks that xhv_name is non-null before doing anything, so I do not
>> understand your concern.
> Heh, this is what happens when you write mails at 4 am. Sorry for the messy
> stream-of-consciousness; Here's the issue:
> That commit changed the HvENAME* macros. Before it would call the
> HvENAME_HEK_NN() macro twice, first to check that the returned value wasn't
> NULL, and then a second time to actually use that value.
> However, now that first call is gone, replaced with a xhv_name_count != -1
> check, because only for -1 does HvENAME_HEK_NN explicitly return a null
> value. And for the core, that's true enough.
> But given an XS module that does this:
> HvAUX(hv)->xhv_name_count *= -2;
> Then perl will start reading into unallocated memory. *Nothing* should be
> messing with xhv_name_count except hv.c. It’s certainly not public.
> (Neither is HvAUX.)
> PerlIO_printf(Perl_debug_log, "[%s]\n", HvENAME_get(hv));
> Then it's conceivable that HvENAME_get and friends could crash because of
> these three lines in HvENAME_HEK_NN:
> Even if HvENAME_get were changed to account, see above.
> HvAUX(hv)->xhv_name_count > 0 ? HvAUX(hv)->xhv_name_u.xhvnameu_names : \
> HvAUX(hv)->xhv_name_count < -1 ? HvAUX(hv)->xhv_name_u.xhvnameu_names :
> HvAUX(hv)->xhv_name_count == -1 ? NULL : \
> If before changing xhv_name_count we would've gotten xhvnameu_names, now
> wewhatever is in xhvnameu_names (or viceversa), which might be NULL [*].
> And since xhv_name_count != -1, we don't check for that, so the macros go on
> to happily try HEK_KEY() on it and possibly make everything explode, whereas
> the previous implementation would've handled it gracefully and returned a
> [*] And if that can't happen then this worry is entirely for naught.
> Should've probably asked that first.
> xhvnameu_names can be null, but only when xhv_name_count is positive.
> Other elements of xhvnameu_names are always non-null. If they are ever
> null, that is a bug.
Those are good enough reasons for me to stop worrying :) Thanks Father C!