Front page | perl.perl5.porters |
Postings from May 2007
Should SvEND(sv) assert SvPOKp(sv)? (was Re: [PATCH] Math::BigInt::FastCalc v0.15)
Thread Previous
|
Thread Next
From:
Nicholas Clark
Date:
May 28, 2007 13:19
Subject:
Should SvEND(sv) assert SvPOKp(sv)? (was Re: [PATCH] Math::BigInt::FastCalc v0.15)
Message ID:
20070528201934.GM18077@plum.flirble.org
On Mon, May 28, 2007 at 02:02:34PM +0000, Tels wrote:
> And this:
>
> end = SvPV(x, len);
> end += len;
> cur = SvEND(x);
> if (end != cur)
> {
> printf ("end: %p, cur: %p\n", cur, end);
> sv_dump(x);
> }
>
> produces this:
>
> # perl -Ilib -Iblib/arch t.pl
> end: 0xb16ac0, cur: 0x669a32
> SV = PVIV(0x9c7d20) at 0x786590
> REFCNT = 1
> FLAGS = (PADBUSY,PADMY,ROK,OVERLOAD)
> IV = 0
> RV = 0xb16ac0
> PV = 0xb16ac0 ""
> CUR = 0
> LEN = 0
> Argument "\0\0" isn't numeric in addition (+)
> at /usr/lib/perl5/5.8.8/Math/BigInt/Calc.pm line 281.
> end: 0xb16ac0, cur: 0x669a12
> SV = PVIV(0x9c7d20) at 0x786590
> REFCNT = 1
> FLAGS = (PADBUSY,PADMY,ROK,OVERLOAD)
> IV = 0
> RV = 0xb16ac0
> PV = 0xb16ac0 ""
> CUR = 0
> LEN = 0
> Argument "\0\0" isn't numeric in numeric eq (==)
> at /usr/lib/perl5/5.8.8/Math/BigInt/Calc.pm line 304.
>
> Hm. So I should have checked for SvPOK() before accessing SvEND() I guess.
Yes. Or, more correctly, I think, just not use it in the general case.
I wondered whether SvEND() should assert that SvPOKp() is set.
But to do so required 3 tweaks to sv.c, and all this (which is basically moving
SvPOK_only() to the line before SvEND())
So I'm not sure if it is a great idea, as it will mean that some non-buggy
code on CPAN will need tweaking. But in return it would catch misuse of
SvEND() in buggy code.
(note that my patch for sv.h is long because it has a lot of comments, and,
independently, a better debugging check for SvLEN())
Nicholas Clark
==== //depot/perl/ext/Encode/Encode.xs#127 - /home/nick/p4perl/perl/ext/Encode/Encode.xs ====
--- /tmp/tmp.68753.0 Mon May 28 21:11:28 2007
+++ /home/nick/p4perl/perl/ext/Encode/Encode.xs Mon May 28 20:06:20 2007
@@ -490,6 +490,7 @@ CODE:
*d++ = (U8)UTF8_EIGHT_BIT_LO(uv);
}
}
+ SvPOK_only(dst);
SvCUR_set(dst, d- (U8 *)SvPVX(dst));
*SvEND(dst) = '\0';
}
@@ -502,7 +503,6 @@ CODE:
}
SvCUR_set(src, slen);
}
- SvPOK_only(dst);
SvUTF8_off(dst);
ST(0) = sv_2mortal(dst);
XSRETURN(1);
==== //depot/perl/ext/Storable/Storable.xs#129 - /home/nick/p4perl/perl/ext/Storable/Storable.xs ====
--- /tmp/tmp.68753.2 Mon May 28 21:11:29 2007
+++ /home/nick/p4perl/perl/ext/Storable/Storable.xs Mon May 28 20:37:13 2007
@@ -4224,12 +4224,12 @@ static SV *retrieve_hook(pTHX_ stcxt_t *
GETMARK(len2);
frozen = NEWSV(10002, len2);
+ (void) SvPOK_only(frozen); /* Validates string pointer */
if (len2) {
SAFEREAD(SvPVX(frozen), len2, frozen);
SvCUR_set(frozen, len2);
*SvEND(frozen) = '\0';
}
- (void) SvPOK_only(frozen); /* Validates string pointer */
if (cxt->s_tainted) /* Is input source tainted? */
SvTAINT(frozen);
@@ -4812,10 +4812,10 @@ static SV *retrieve_lscalar(pTHX_ stcxt_
* this way, it's worth the hassle and risk.
*/
+ (void) SvPOK_only(sv); /* Validate string pointer */
SAFEREAD(SvPVX(sv), len, sv);
SvCUR_set(sv, len); /* Record C string length */
*SvEND(sv) = '\0'; /* Ensure it's null terminated anyway */
- (void) SvPOK_only(sv); /* Validate string pointer */
if (cxt->s_tainted) /* Is input source tainted? */
SvTAINT(sv); /* External data cannot be trusted */
@@ -4864,6 +4864,7 @@ static SV *retrieve_scalar(pTHX_ stcxt_t
sv_upgrade(sv, SVt_PV);
}
SvGROW(sv, 1);
+ (void) SvPOK_only(sv); /* Validate string pointer */
*SvEND(sv) = '\0'; /* Ensure it's null terminated anyway */
TRACEME(("ok (retrieve_scalar empty at 0x%"UVxf")", PTR2UV(sv)));
} else {
@@ -4873,13 +4874,13 @@ static SV *retrieve_scalar(pTHX_ stcxt_t
* work done by sv_setpv. Since we're going to allocate lots of scalars
* this way, it's worth the hassle and risk.
*/
+ (void) SvPOK_only(sv); /* Validate string pointer */
SAFEREAD(SvPVX(sv), len, sv);
SvCUR_set(sv, len); /* Record C string length */
*SvEND(sv) = '\0'; /* Ensure it's null terminated anyway */
TRACEME(("small scalar len %d '%s'", len, SvPVX(sv)));
}
- (void) SvPOK_only(sv); /* Validate string pointer */
if (cxt->s_tainted) /* Is input source tainted? */
SvTAINT(sv); /* External data cannot be trusted */
==== //depot/perl/pp_sys.c#539 - /home/nick/p4perl/perl/pp_sys.c ====
--- /tmp/tmp.68753.3 Mon May 28 21:11:29 2007
+++ /home/nick/p4perl/perl/pp_sys.c Mon May 28 19:27:42 2007
@@ -1738,8 +1738,8 @@ PP(pp_sysread)
goto say_undef;
}
SvCUR_set(read_target, count+(buffer - SvPVX_const(read_target)));
- *SvEND(read_target) = '\0';
(void)SvPOK_only(read_target);
+ *SvEND(read_target) = '\0';
if (fp_utf8 && !IN_BYTES) {
/* Look at utf8 we got back and count the characters */
const char *bend = buffer + count;
==== //depot/perl/sv.h#319 - /home/nick/p4perl/perl/sv.h ====
--- /tmp/tmp.68753.4 Mon May 28 21:11:29 2007
+++ /home/nick/p4perl/perl/sv.h Mon May 28 19:35:36 2007
@@ -1234,7 +1234,7 @@ the scalar's value cannot change unless
# endif
# define SvCUR(sv) (0 + ((XPV*) SvANY(sv))->xpv_cur)
# define SvLEN(sv) (0 + ((XPV*) SvANY(sv))->xpv_len)
-# define SvEND(sv) ((sv)->sv_u.svu_pv + ((XPV*)SvANY(sv))->xpv_cur)
+# define SvEND(sv) (SvPVX(sv) + SvCUR(sv))
# ifdef DEBUGGING
# define SvMAGIC(sv) (0 + *(assert(SvTYPE(sv) >= SVt_PVMG), &((XPVMG*) SvANY(sv))->xmg_u.xmg_magic))
@@ -1244,9 +1244,6 @@ the scalar's value cannot change unless
# define SvSTASH(sv) (0 + ((XPVMG*) SvANY(sv))->xmg_stash)
# endif
#else
-# define SvLEN(sv) ((XPV*) SvANY(sv))->xpv_len
-# define SvEND(sv) ((sv)->sv_u.svu_pv + ((XPV*)SvANY(sv))->xpv_cur)
-
# if defined (DEBUGGING) && defined(__GNUC__) && !defined(PERL_GCC_BRACE_GROUPS_FORBIDDEN)
/* These get expanded inside other macros that already use a variable _sv */
# define SvPVX(sv) \
@@ -1265,6 +1262,27 @@ the scalar's value cannot change unless
assert(!isGV_with_GP(_svi)); \
&(((XPV*) SvANY(_svi))->xpv_cur); \
}))
+# define SvLEN(sv) \
+ (*({ SV *const _svi = (SV *) (sv); \
+ assert(SvTYPE(_svi) >= SVt_PV); \
+ assert(SvTYPE(_svi) != SVt_PVAV); \
+ assert(SvTYPE(_svi) != SVt_PVHV); \
+ &(((XPV*) SvANY(_svi))->xpv_len); \
+ }))
+/* This is quite deliberate - make sure we textually evaluate sv exactly twice,
+ because all the other variants of this macro do the same. If we evaluated it
+ only once when we assign it to the local variable, then people developing
+ under -DDEBUGGING won't see their bugs caused by expected multiple side
+ effects.
+ Also, we on't assert SvPOKp(sv) for SvEND(), and not for SvCUR() or SvPVX(),
+ because some other code hijacks them for its own purposes. Whereas SvEND()
+ can't be meaningful unless SvPVX() points to a block of memory for which
+ SvCUR() records the length. */
+# define SvEND(sv) \
+ ({ SV *const zlonk = (SV *) (sv); \
+ assert(SvPOKp(sv)); \
+ (SvPVX(zlonk) + SvCUR(zlonk)); \
+ })
# define SvIVX(sv) \
(*({ SV *const _svi = (SV *) (sv); \
assert(SvTYPE(_svi) == SVt_IV || SvTYPE(_svi) >= SVt_PVIV); \
@@ -1308,6 +1326,8 @@ the scalar's value cannot change unless
# else
# define SvPVX(sv) ((sv)->sv_u.svu_pv)
# define SvCUR(sv) ((XPV*) SvANY(sv))->xpv_cur
+# define SvLEN(sv) ((XPV*) SvANY(sv))->xpv_len
+# define SvEND(sv) (SvPVX(sv) + SvCUR(sv))
# define SvIVX(sv) ((XPVIV*) SvANY(sv))->xiv_iv
# define SvUVX(sv) ((XPVUV*) SvANY(sv))->xuv_uv
# define SvNVX(sv) ((XPVNV*) SvANY(sv))->xnv_u.xnv_nv
Thread Previous
|
Thread Next