develooper Front page | perl.perl5.porters | Postings from March 2013

SV_CHECK_THINKFIRST() needs to be rethought and further COW woes....

Thread Next
From:
demerphq
Date:
March 25, 2013 09:14
Subject:
SV_CHECK_THINKFIRST() needs to be rethought and further COW woes....
Message ID:
CANgJU+VoKy7b4+uptQaHqtbReNjVp6ArZiosU+vfx_UkwPnWkw@mail.gmail.com
Following up on recent mails about HTML::Entities, supposedly
read-only keys in 5.14, SV_CHECK_THINKFIRST() and etc...

I think SV_CHECK_THINKFIRST() is broken for some definition of broken.

First, its name is *terrible*.  It is named after the wrong thing, and
as such does not from the name suggest that it has side effects, and
that in fact the side effects are why it is being called.

#define SV_CHECK_THINKFIRST(sv) if (SvTHINKFIRST(sv)) \
                                    sv_force_normal_flags(sv, 0)

It should be named after "sv_force_normal_flags" and called something
like SV_FORCE_NORMAL_FLAGS().

Second, I question the implementation in general.

For instance the original bug in HTML::Entities is that if I feed it a
key under 5.14 it improperly dies with the message "Can't inline
decode readonly string".

If I make the following patch:

@@ -616,8 +616,13 @@ decode_entities(...)
        for (i = 0; i < items; i++) {
            if (GIMME_V != G_VOID)
                ST(i) = sv_2mortal(newSVsv(ST(i)));
-           else if (SvREADONLY(ST(i)))
-               croak("Can't inline decode readonly string");
+           else {
+#ifdef SV_CHECK_THINKFIRST
+                SV_CHECK_THINKFIRST(ST(i));
+#endif
+                if (SvREADONLY(ST(i)))
+                   croak("Can't inline decode readonly string in
decode_entities()");
+            }

It fixes the problem with keys() as arguments (IOW, READONLY+FAKE).

However, it breaks the old behavior of

decode_entities("thing");

which SHOULD trigger the error, but instead execution never gets
there, instead it produces:

$ perl -Mblib -MHTML::Entities=decode_entities -e'decode_entities("foo")'
Modification of a read-only value attempted at -e line 1.

The value according to Devel::Peek is as follows:

SV = PV(0x15b6be8) at 0x15dd9a8
  REFCNT = 1
  FLAGS = (PADTMP,POK,READONLY,pPOK)
  PV = 0x15cf980 "foo"\0
  CUR = 3
  LEN = 8

I don't see how this behavior of SV_CHECK_THINKFIRST() can be sound.
We can't recommend to XS authors that they use it, as it means they
cant intercept this case and do something sensible with it.

This mechanism seems like it has a rushed, poorly thought through
aspect to it that is going to cause a lot problems down the road.

To make things worse btw, IMO we have a real issue with COW, in that
a) COW strings no longer have the READONLY flag set, code that wants
to do an inplace modification based on this flag not being set will
silently start modifying COW strings.

Lastly, I thought COW was disabled, but I see lots of code related to
it. Devel::Peek still reports keys() as being COW, etc.

I think we have a real mess on our hands here. I feel really pessimistic.

Yves




-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About