string corruption with lvalue sub #6838

p5pRT · 2003-10-13T17:28:48Z

Migrated from rt.perl.org#24200 (status was 'resolved')

Searchable as RT24200$

p5pRT · 2003-10-13T17:28:48Z

From mconst@csua.berkeley.edu

Created by mconst@csua.berkeley.edu

Under perl v5.8.1 on my i386 Debian Linux system, this script:

$foo = "a";
sub bar: lvalue { substr $foo, 0 }
bar = "XXX";
print bar, "\n";

produces the output "X". I would have expected it to output "XXX",
like it does if you make the calls to substr directly instead of
through the lvalue function.

I get the same behaviour with perl v5.6.1 on an i386 FreeBSD system.

If you run the program under a debugging perl, you can actually see
the value change -- substr returns the string I expect, but by the
time bar returns, the value is different:

(./bugtest:2) substr
Pad 0x81c0be0[0x81c1950] sv: 3 sv=0x81c0c1c

STACK 0: MAIN
CX 0: BLOCK => *
retop=const
CX 1: SUB => PVLV("XXX"\0)

(./bugtest:2) leavesublv

STACK 0: MAIN
CX 0: BLOCK => * <T>PVNV("X"\0)

Am I doing something wrong?

Perl Info


Flags:
    category=core
    severity=low

Site configuration information for perl v5.8.1:

Configured by Debian Project at Tue Sep 30 21:35:53 EST 2003.

Summary of my perl5 (revision 5.0 version 8 subversion 1) configuration:
  Platform:
    osname=linux, osvers=2.4.22-xfs+ti1211, archname=i386-linux-thread-multi
    uname='linux kosh 2.4.22-xfs+ti1211 #1 wed sep 24 21:17:59 est 2003 i686 gnulinux '
    config_args='-Dusethreads -Duselargefiles -Dccflags=-DDEBIAN -Dcccdlflags=-fPIC -Darchname=i386-linux -Dprefix=/usr -Dprivlib=/usr/share/perl/5.8.1 -Darchlib=/usr/lib/perl/5.8.1 -Dvendorprefix=/usr -Dvendorlib=/usr/share/perl5 -Dvendorarch=/usr/lib/perl5 -Dsiteprefix=/usr/local -Dsitelib=/usr/local/share/perl/5.8.1 -Dsitearch=/usr/local/lib/perl/5.8.1 -Dman1dir=/usr/share/man/man1 -Dman3dir=/usr/share/man/man3 -Dsiteman1dir=/usr/local/man/man1 -Dsiteman3dir=/usr/local/man/man3 -Dman1ext=1 -Dman3ext=3perl -Dpager=/usr/bin/sensible-pager -Uafs -Ud_csh -Uusesfio -Uusenm -Duseshrplib -Dlibperl=libperl.so.5.8.1 -Dd_dosuid -des'
    hint=recommended, useposix=true, d_sigaction=define
    usethreads=define use5005threads=undef useithreads=define usemultiplicity=define
    useperlio=define d_sfio=undef uselargefiles=define usesocks=undef
    use64bitint=undef use64bitall=undef uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS -DDEBIAN -fno-strict-aliasing -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O3',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS -DDEBIAN -fno-strict-aliasing -I/usr/local/include'
    ccversion='', gccversion='3.3.2 20030908 (Debian prerelease)', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib
    libs=-lgdbm -ldb -ldl -lm -lpthread -lc -lcrypt
    perllibs=-ldl -lm -lpthread -lc -lcrypt
    libc=/lib/libc-2.3.2.so, so=so, useshrplib=true, libperl=libperl.so.5.8.1
    gnulibc_version='2.3.2'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-rdynamic'
    cccdlflags='-fPIC', lddlflags='-shared -L/usr/local/lib'

Locally applied patches:
    


@INC for perl v5.8.1:
    /etc/perl
    /usr/local/lib/perl/5.8.1
    /usr/local/share/perl/5.8.1
    /usr/lib/perl5
    /usr/share/perl5
    /usr/lib/perl/5.8.1
    /usr/share/perl/5.8.1
    /usr/local/lib/site_perl
    .


Environment for perl v5.8.1:
    HOME=/home/mconst
    LANG=C
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/mconst/bin:/home/mconst/env/bin:/usr/local/bin:/usr/bin:/usr/sbin:/bin:/sbin:/usr/X11R6/bin:/usr/games
    PERL_BADLANG (unset)
    SHELL=/bin/tcsh

p5pRT · 2003-10-15T07:19:32Z

From @ysth

On Mon, Oct 13, 2003 at 05:28:49PM -0000, "mconst@csua.berkeley.edu (via RT)" <perlbug-followup@perl.org> wrote:

Under perl v5.8.1 on my i386 Debian Linux system, this script:
$foo = "a";
sub bar&#8203;: lvalue \{ substr $foo\, 0 \}
bar = "XXX";
print bar\, "\\n";
produces the output "X". I would have expected it to output "XXX",
like it does if you make the calls to substr directly instead of
through the lvalue function.

Looks like substr is expecting to always or never return an lvalue.
It gets confused if you first make it return a PVLV TARG and then
reuse the TARG not considering it is a PVLV.

This is a start:

Inline Patch

--- perl/pp.c.orig	2003-10-14 23:55:45.673918400 -0700
+++ perl/pp.c	2003-10-14 23:58:14.748276800 -0700
@@ -3079,6 +3079,8 @@
 		    SvREFCNT_dec(LvTARG(TARG));
 		LvTARG(TARG) = SvREFCNT_inc(sv);
 	    }
+        }
+        if (SvTYPE(TARG) == SVt_PVLV) {
 	    LvTARGOFF(TARG) = upos;
 	    LvTARGLEN(TARG) = urem;
 	}

End of Patch.

but it needs more. The non-lvalue but a PVLV case needs to handle
TARG REFCNT > 1. (Is that check in the right place in the lvalue
case? Its already done stuff to TARG at the point it decides to do
TARG = sv_newmortal(). And does it leak? Seems like it should be
decrementing the old TARGs refcnt before reassigning it.)

Can someone more familiar with TARG reuse take a look at this?

p5pRT · 2004-01-19T02:18:21Z

From @ysth

I repeat my plea for someone with knowlege about using TARG to look at
this. Should the rvalue case just create a new mortal sv to return if
TARG is a PVLV?

On Wed, Oct 15, 2003 at 12:18:16AM -0700, Yitzchak Scott-Thoennes <sthoenna@efn.org> wrote:

On Mon, Oct 13, 2003 at 05:28:49PM -0000, "mconst@csua.berkeley.edu (via RT)" <perlbug-followup@perl.org> wrote:
Under perl v5.8.1 on my i386 Debian Linux system, this script:
$foo = "a";
sub bar&#8203;: lvalue \{ substr $foo\, 0 \}
bar = "XXX";
print bar\, "\\n";
produces the output "X". I would have expected it to output "XXX",
like it does if you make the calls to substr directly instead of
through the lvalue function.
Looks like substr is expecting to always or never return an lvalue.
It gets confused if you first make it return a PVLV TARG and then
reuse the TARG not considering it is a PVLV.

This is a start:

--- perl/pp.c.orig 2003-10-14 23:55:45.673918400 -0700
+++ perl/pp.c 2003-10-14 23:58:14.748276800 -0700
@@ -3079,6 +3079,8 @@
SvREFCNT_dec(LvTARG(TARG));
LvTARG(TARG) = SvREFCNT_inc(sv);
}
+ }
+ if (SvTYPE(TARG) == SVt_PVLV) {
LvTARGOFF(TARG) = upos;
LvTARGLEN(TARG) = urem;
}
End of Patch.

but it needs more. The non-lvalue but a PVLV case needs to handle
TARG REFCNT > 1. (Is that check in the right place in the lvalue
case? Its already done stuff to TARG at the point it decides to do
TARG = sv_newmortal(). And does it leak? Seems like it should be
decrementing the old TARGs refcnt before reassigning it.)

Can someone more familiar with TARG reuse take a look at this?

p5pRT · 2004-01-24T01:06:30Z

From @iabyn

On Wed, Oct 15, 2003 at 12:18:16AM -0700, Yitzchak Scott-Thoennes wrote:

On Mon, Oct 13, 2003 at 05:28:49PM -0000, "mconst@csua.berkeley.edu (via RT)" <perlbug-followup@perl.org> wrote:
Under perl v5.8.1 on my i386 Debian Linux system, this script:
$foo = "a";
sub bar&#8203;: lvalue \{ substr $foo\, 0 \}
bar = "XXX";
print bar\, "\\n";
produces the output "X". I would have expected it to output "XXX",
like it does if you make the calls to substr directly instead of
through the lvalue function.
Looks like substr is expecting to always or never return an lvalue.
It gets confused if you first make it return a PVLV TARG and then
reuse the TARG not considering it is a PVLV.

This is a start:

--- perl/pp.c.orig 2003-10-14 23:55:45.673918400 -0700
+++ perl/pp.c 2003-10-14 23:58:14.748276800 -0700
@@ -3079,6 +3079,8 @@
SvREFCNT_dec(LvTARG(TARG));
LvTARG(TARG) = SvREFCNT_inc(sv);
}
+ }
+ if (SvTYPE(TARG) == SVt_PVLV) {
LvTARGOFF(TARG) = upos;
LvTARGLEN(TARG) = urem;
}
End of Patch.

but it needs more. The non-lvalue but a PVLV case needs to handle
TARG REFCNT > 1. (Is that check in the right place in the lvalue
case? Its already done stuff to TARG at the point it decides to do
TARG = sv_newmortal(). And does it leak? Seems like it should be
decrementing the old TARGs refcnt before reassigning it.)

Can someone more familiar with TARG reuse take a look at this?

I'm the person who added the TARG REFCNT > 1 test, so I claim to know a
bit about it. I propose the following patch, which simply says - if the
TARG is already of the right type, reuse it; otherwise just use a tmp
instead. I don't think there are any leaks associated with it.

Dave.

--
Thank God I'm an atheist.....

Inline Patch

--- ../22168.ORIG/pp.c	Sat Jan 17 18:31:44 2004
+++ pp.c	Sat Jan 24 00:49:58 2004
@@ -3027,6 +3027,19 @@ PP(pp_substr)
 	if (utf8_curlen)
 	    sv_pos_u2b(sv, &pos, &rem);
 	tmps += pos;
+	/* we either return a PV or an LV. If the TARG hasn't been used
+	 * before, or is of that type, reuse it; otherwise use a mortal
+	 * instead. Note that LVs can have an extended lifetime, so also
+	 * dont reuse if refcount > 1 (bug #20933) */
+	if (SvTYPE(TARG) > SVt_NULL) {
+	    if ( (SvTYPE(TARG) == SVt_PVLV)
+		    ? (!lvalue || SvREFCNT(TARG) > 1)
+		    : lvalue)
+	    {
+		TARG = sv_newmortal();
+	    }
+	}
+
 	sv_setpvn(TARG, tmps, rem);
 #ifdef USE_LOCALE_COLLATE
 	sv_unmagic(TARG, PERL_MAGIC_collxfrm);
@@ -3063,8 +3076,6 @@ PP(pp_substr)
 		    sv_setpvn(sv,"",0);	/* avoid lexical reincarnation */
 	    }
 
-	    if (SvREFCNT(TARG) > 1)	/* don't share the TARG (#20933) */
-		TARG = sv_newmortal();
 	    if (SvTYPE(TARG) < SVt_PVLV) {
 		sv_upgrade(TARG, SVt_PVLV);
 		sv_magic(TARG, Nullsv, PERL_MAGIC_substr, Nullch, 0);

p5pRT · 2004-01-25T10:38:39Z

From @ysth

On Sat, Jan 24, 2004 at 01:05:20AM +0000, Dave Mitchell <davem@fdisolutions.com> wrote:

On Wed, Oct 15, 2003 at 12:18:16AM -0700, Yitzchak Scott-Thoennes wrote:

but it needs more. The non-lvalue but a PVLV case needs to handle
TARG REFCNT > 1. (Is that check in the right place in the lvalue
case? Its already done stuff to TARG at the point it decides to do
TARG = sv_newmortal(). And does it leak? Seems like it should be
decrementing the old TARGs refcnt before reassigning it.)

Can someone more familiar with TARG reuse take a look at this?

I'm the person who added the TARG REFCNT > 1 test, so I claim to know a
bit about it. I propose the following patch, which simply says - if the
TARG is already of the right type, reuse it; otherwise just use a tmp
instead. I don't think there are any leaks associated with it.

Looks ok. Do you want to come up with tests for this or shall I?

p5pRT · 2004-01-25T12:12:21Z

From @iabyn

On Sun, Jan 25, 2004 at 02:37:49AM -0800, Yitzchak Scott-Thoennes wrote:

On Sat, Jan 24, 2004 at 01:05:20AM +0000, Dave Mitchell <davem@fdisolutions.com> wrote:

On Wed, Oct 15, 2003 at 12:18:16AM -0700, Yitzchak Scott-Thoennes wrote:

but it needs more. The non-lvalue but a PVLV case needs to handle
TARG REFCNT > 1. (Is that check in the right place in the lvalue
case? Its already done stuff to TARG at the point it decides to do
TARG = sv_newmortal(). And does it leak? Seems like it should be
decrementing the old TARGs refcnt before reassigning it.)

Can someone more familiar with TARG reuse take a look at this?

I'm the person who added the TARG REFCNT > 1 test, so I claim to know a
bit about it. I propose the following patch, which simply says - if the
TARG is already of the right type, reuse it; otherwise just use a tmp
instead. I don't think there are any leaks associated with it.

Looks ok. Do you want to come up with tests for this or shall I?

I'm happy for you to do so :-)

--
Little fly, thy summer's play my thoughtless hand
has terminated with extreme prejudice.
(with apologies to William Blake)

p5pRT · 2004-01-25T15:39:42Z

From @nwc10

On Sun, Jan 25, 2004 at 12:10:52PM +0000, Dave Mitchell wrote:

On Sun, Jan 25, 2004 at 02:37:49AM -0800, Yitzchak Scott-Thoennes wrote:

Looks ok. Do you want to come up with tests for this or shall I?

I'm happy for you to do so :-)

I think that in general having tests written by someone other than the patch
author is good. (Not that this rules out the patch author also writing tests)

Nicholas Clark

p5pRT · 2004-03-27T02:18:33Z

From @iabyn

On Sat, Jan 24, 2004 at 01:05:20AM +0000, Dave Mitchell wrote:

On Wed, Oct 15, 2003 at 12:18:16AM -0700, Yitzchak Scott-Thoennes wrote:
On Mon, Oct 13, 2003 at 05:28:49PM -0000, "mconst@csua.berkeley.edu (via RT)" <perlbug-followup@perl.org> wrote:
Under perl v5.8.1 on my i386 Debian Linux system, this script:
$foo = "a";
sub bar&#8203;: lvalue \{ substr $foo\, 0 \}
bar = "XXX";
print bar\, "\\n";
produces the output "X". I would have expected it to output "XXX",
like it does if you make the calls to substr directly instead of
through the lvalue function.
Looks like substr is expecting to always or never return an lvalue.
It gets confused if you first make it return a PVLV TARG and then
reuse the TARG not considering it is a PVLV.

This is a start:

--- perl/pp.c.orig 2003-10-14 23:55:45.673918400 -0700
+++ perl/pp.c 2003-10-14 23:58:14.748276800 -0700
@@ -3079,6 +3079,8 @@
SvREFCNT_dec(LvTARG(TARG));
LvTARG(TARG) = SvREFCNT_inc(sv);
}
+ }
+ if (SvTYPE(TARG) == SVt_PVLV) {
LvTARGOFF(TARG) = upos;
LvTARGLEN(TARG) = urem;
}
End of Patch.

but it needs more. The non-lvalue but a PVLV case needs to handle
TARG REFCNT > 1. (Is that check in the right place in the lvalue
case? Its already done stuff to TARG at the point it decides to do
TARG = sv_newmortal(). And does it leak? Seems like it should be
decrementing the old TARGs refcnt before reassigning it.)

Can someone more familiar with TARG reuse take a look at this?
I'm the person who added the TARG REFCNT > 1 test, so I claim to know a
bit about it. I propose the following patch, which simply says - if the
TARG is already of the right type, reuse it; otherwise just use a tmp
instead. I don't think there are any leaks associated with it.

I've now belatedly applied my patch to bleedperl, along with some tests.
Ironically the OP's code actually works now due to change #22414
that updates the substitution window in the substr LV; but the 2nd new
test below would still fail without the new patch - it being the
lvalue sometimes returning an LV and sometimes a PV.

Dave.
--
Counsellor Troi states something other than the blindingly obvious.
-- Things That Never Happen in "Star Trek" #16

Change 22599 by davem@davem-percy on 2004/03/27 01:54:09

[perl #24200] string corruption with lvalue sub
Depending on the context, the same substr OP may want to return
a PVLV or an LV on subsequent invcations. If TARG is the wrong
type, use a mortal instead.

Affected files ...

... //depot/perl/pp.c#411 edit
... //depot/perl/t/op/substr.t#28 edit

Differences ...

==== //depot/perl/pp.c#411 (text) ====

@@ -3038,6 +3038,19 @@
if (utf8_curlen)
sv_pos_u2b(sv, &pos, &rem);
tmps += pos;
+ /* we either return a PV or an LV. If the TARG hasn't been used
+ * before, or is of that type, reuse it; otherwise use a mortal
+ * instead. Note that LVs can have an extended lifetime, so also
+ * dont reuse if refcount > 1 (bug #20933) */
+ if (SvTYPE(TARG) > SVt_NULL) {
+ if ( (SvTYPE(TARG) == SVt_PVLV)
+ ? (!lvalue || SvREFCNT(TARG) > 1)
+ : lvalue)
+ {
+ TARG = sv_newmortal();
+ }
+ }
+
sv_setpvn(TARG, tmps, rem);
#ifdef USE_LOCALE_COLLATE
sv_unmagic(TARG, PERL_MAGIC_collxfrm);
@@ -3074,8 +3087,6 @@
sv_setpvn(sv,"",0); /* avoid lexical reincarnation */
}

- if (SvREFCNT(TARG) > 1) /* don't share the TARG (#20933) */
- TARG = sv_newmortal();
if (SvTYPE(TARG) < SVt_PVLV) {
sv_upgrade(TARG, SVt_PVLV);
sv_magic(TARG, Nullsv, PERL_MAGIC_substr, Nullch, 0);

==== //depot/perl/t/op/substr.t#28 (xtext) ====

@@ -1,6 +1,6 @@
#!./perl

-print "1..186\n";
+print "1..188\n";

#P = start of string Q = start of substr R = end of substr S = end of string

@@ -629,3 +629,14 @@
ok 186, $x eq 'aYYYYef';
}
}
+
+# [perl #24200] string corruption with lvalue sub
+
+{
+ my $foo = "a";
+ sub bar: lvalue { substr $foo, 0 }
+ bar = "XXX";
+ ok 187, bar eq 'XXX';
+ $foo = '123456789';
+ ok 188, bar eq '123456789';
+}

p5pRT · 2004-03-27T02:26:44Z

@iabyn - Status changed from 'open' to 'resolved'

p5pRT closed this as completed Mar 27, 2004

p5pRT added Severity Low distro-Linux type-core labels Oct 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

string corruption with lvalue sub #6838

string corruption with lvalue sub #6838

p5pRT commented Oct 13, 2003

p5pRT commented Oct 13, 2003

p5pRT commented Oct 15, 2003

p5pRT commented Jan 19, 2004

p5pRT commented Jan 24, 2004

p5pRT commented Jan 25, 2004

p5pRT commented Jan 25, 2004

p5pRT commented Jan 25, 2004

p5pRT commented Mar 27, 2004

p5pRT commented Mar 27, 2004

string corruption with lvalue sub #6838

string corruption with lvalue sub #6838

Comments

p5pRT commented Oct 13, 2003

p5pRT commented Oct 13, 2003

From mconst@csua.berkeley.edu

Created by mconst@csua.berkeley.edu

p5pRT commented Oct 15, 2003

From @ysth

p5pRT commented Jan 19, 2004

From @ysth

p5pRT commented Jan 24, 2004

From @iabyn

p5pRT commented Jan 25, 2004

From @ysth

p5pRT commented Jan 25, 2004

From @iabyn

p5pRT commented Jan 25, 2004

From @nwc10

p5pRT commented Mar 27, 2004

From @iabyn

p5pRT commented Mar 27, 2004