Bleadperl v5.25.10-81-gd69c43040e breaks JV/App-PDF-Link-0.18.tar.gz

p5pRT commented 7 years ago

Migrated from rt.perl.org#131083 (status was 'open')

Searchable as RT131083$

p5pRT commented 7 years ago

From @andk

bisect

d69c43040e4967294b1195ecfdc4acf0f74b5958 is the first bad commit commit d69c43040e4967294b1195ecfdc4acf0f74b5958 Author: David Mitchell \davem@iabyn\.com Date: Wed Mar 15 14:35:59 2017 +0000

Perl_do_vecget(): change offset arg to STRLEN type

rt-cpan

https://rt.cpan.org/Ticket/Display.html?id=120801

perl -V

Summary of my perl5 (revision 5 version 25 subversion 11) configuration: Commit id: d69c43040e4967294b1195ecfdc4acf0f74b5958 Platform: osname=linux osvers=4.9.0-2-amd64 archname=x86_64-linux-thread-multi-ld uname='linux k93msid 4.9.0-2-amd64 #1 smp debian 4.9.13-1 (2017-02-27) x86_64 gnulinux ' config_args='-Dprefix=/home/sand/src/perl/repoperls/installed-perls/host/k93msid/v5.25.10-81-gd69c43040e/5ea4 -Dmyhostname=k93msid -Dinstallusrbinperl=n -Uversiononly -Dusedevel -des -Ui_db -Dlibswanted=cl pthread socket inet nsl gdbm dbm malloc dl ld sun m crypt sec util c cposix posix ucb BSD gdbm_compat -Duseithreads -Duselongdouble -DDEBUGGING=-g' hint=recommended useposix=true d_sigaction=define useithreads=define usemultiplicity=define use64bitint=define use64bitall=define uselongdouble=define usemymalloc=n default_inc_excludes_dot=define bincompat5005=undef Compiler: cc='cc' ccflags ='-D_REENTRANT -D_GNU_SOURCE -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64' optimize='-O2 -g' cppflags='-D_REENTRANT -D_GNU_SOURCE -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include' ccversion='' gccversion='6.3.0 20170316' gccosandvers='' intsize=4 longsize=8 ptrsize=8 doublesize=8 byteorder=12345678 doublekind=3 d_longlong=define longlongsize=8 d_longdbl=define longdblsize=16 longdblkind=3 ivtype='long' ivsize=8 nvtype='long double' nvsize=16 Off_t='off_t' lseeksize=8 alignbytes=16 prototype=define Linker and Libraries: ld='cc' ldflags =' -fstack-protector-strong -L/usr/local/lib' libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/6/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc libc=libc-2.24.so so=so useshrplib=false libperl=libperl.a gnulibc_version='2.24' Dynamic Linking: dlsrc=dl_dlopen.xs dlext=so d_dlsymun=undef ccdlflags='-Wl\,-E' cccdlflags='-fPIC' lddlflags='-shared -O2 -g -L/usr/local/lib -fstack-protector-strong'

Characteristics of this binary (from libperl): Compile-time options: HAS_TIMES MULTIPLICITY PERLIO_LAYERS PERL_COPY_ON_WRITE PERL_DONT_CREATE_GVSV PERL_IMPLICIT_CONTEXT PERL_MALLOC_WRAP PERL_OP_PARENT PERL_PRESERVE_IVUV PERL_USE_DEVEL USE_64_BIT_ALL USE_64_BIT_INT USE_ITHREADS USE_LARGE_FILES USE_LOCALE USE_LOCALE_COLLATE USE_LOCALE_CTYPE USE_LOCALE_NUMERIC USE_LOCALE_TIME USE_LONG_DOUBLE USE_PERLIO USE_PERL_ATOF USE_REENTRANT_API Built under linux Compiled at Mar 30 2017 20:46:41 @INC: /home/sand/src/perl/repoperls/installed-perls/host/k93msid/v5.25.10-81-gd69c43040e/5ea4/lib/site_perl/5.25.11/x86_64-linux-thread-multi-ld /home/sand/src/perl/repoperls/installed-perls/host/k93msid/v5.25.10-81-gd69c43040e/5ea4/lib/site_perl/5.25.11 /home/sand/src/perl/repoperls/installed-perls/host/k93msid/v5.25.10-81-gd69c43040e/5ea4/lib/5.25.11/x86_64-linux-thread-multi-ld /home/sand/src/perl/repoperls/installed-perls/host/k93msid/v5.25.10-81-gd69c43040e/5ea4/lib/5.25.11

-- andreas

p5pRT commented 7 years ago

From @iabyn

On Thu\, Mar 30\, 2017 at 09:41:01PM -0700\, Andreas J. Koenig via RT wrote:

# New Ticket Created by (Andreas J. Koenig) # Please include the string: [perl #131083] # in the subject line of all future correspondence about this issue. # \<URL: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=131083 >

bisect ------ d69c43040e4967294b1195ecfdc4acf0f74b5958 is the first bad commit commit d69c43040e4967294b1195ecfdc4acf0f74b5958 Author: David Mitchell \davem@iabyn\.com Date: Wed Mar 15 14:35:59 2017 +0000
Perl\_do\_vecget&#8203;: change offset arg to STRLEN type
rt-cpan ------- https://rt.cpan.org/Ticket/Display.html?id=120801

That commit changed how vec() behaves in maybe-lvalue context with a negative offset. Consider these three cases:

(1) $x = vec($s\,-1\,8); (2) vec($s\,-1\,8) = 1; (3) f(vec($s\,-1\,8)) where (3a) sub f { $x = $_[0] } where (3b) sub f { $_[0] = 1 }

(1) is documented to return 0\, and this is unchanged (2) and (3b) are documented to croak "Negative offset to vec in lvalue context"\, and this is unchanged (except that in 3b the croak now happens just before f() is called\, rather than later during the assignment).

(3a) has changed. Previously it would set $x to 0\, now it croaks.

vec() in lvalue and maybe-lvalue context works by returning a SVt_PVLV SV whose xlvu_targoff field is set to the offset. If that SV is assigned to\, its set magic retrieves the offset from that field and croaks if it's negative. My commit changed it so that the negative check was done earlier\, when the SVt_PVLV was created.

The reason for the change is that xlvu_targoff is of type STRLEN which may be 32-bit\, while the offset is an IV or UV which can be 64-bit. Thus there was lots of opportunity for truncation and wrapping.

I guess I could change it so that only the minimum possible checking is done early (will the value fit in a STRELEN/SSize_t and if so croak)\, then leave the negative checking to be done by the LV's set magic.

In the longer term\, maybe change SVt_PVLV's targoff and targlen to be IV/UVs rather than STRLEN/SSize_t.

-- That he said that that that that is is is debatable\, is debatable.

p5pRT commented 7 years ago

The RT System itself - Status changed from 'new' to 'open'

p5pRT commented 7 years ago

From @iabyn

On Fri\, Mar 31\, 2017 at 11:37:54AM +0100\, Dave Mitchell wrote:

That commit changed how vec() behaves in maybe-lvalue context with a negative offset. Consider these three cases:

(1) $x = vec($s\,-1\,8); (2) vec($s\,-1\,8) = 1; (3) f(vec($s\,-1\,8)) where (3a) sub f { $x = $_[0] } where (3b) sub f { $_[0] = 1 }

(1) is documented to return 0\, and this is unchanged (2) and (3b) are documented to croak "Negative offset to vec in lvalue context"\, and this is unchanged (except that in 3b the croak now happens just before f() is called\, rather than later during the assignment).

(3a) has changed. Previously it would set $x to 0\, now it croaks.

vec() in lvalue and maybe-lvalue context works by returning a SVt_PVLV SV whose xlvu_targoff field is set to the offset. If that SV is assigned to\, its set magic retrieves the offset from that field and croaks if it's negative. My commit changed it so that the negative check was done earlier\, when the SVt_PVLV was created.

The reason for the change is that xlvu_targoff is of type STRLEN which may be 32-bit\, while the offset is an IV or UV which can be 64-bit. Thus there was lots of opportunity for truncation and wrapping.

I guess I could change it so that only the minimum possible checking is done early (will the value fit in a STRELEN/SSize_t and if so croak)\, then leave the negative checking to be done by the LV's set magic.

Instead I went for the approach of checking early\, but only flagging the result; then croaking later:

commit 1b92e6949b737e92f61827f9c92afce9218e30ba Author: David Mitchell \davem@iabyn\.com AuthorDate: Fri Mar 31 13:44:58 2017 +0100 Commit: David Mitchell \davem@iabyn\.com CommitDate: Fri Mar 31 14:13:24 2017 +0100

vec(): defer lvalue out-of-range croaking
RT #131083
Recent commits v5.25.10-81-gd69c430 and v5.25.10-82-g67dd6f3 added out-of-range/overflow checks for the offset arg of vec(). However in lvalue context\, these croaks now happen before the SVt_PVLV was created\, rather than when its set magic was called. This means that something like
sub f { $x = $_[0] } f(vec($s\, -1\, 8))
now croaks even though the out-of-range value never ended up getting used in lvalue context.
This commit fixes things by\, in pp_vec()\, rather than croaking\, just set flag bits in LvFLAGS() to indicate that the offset is -Ve / out-of-range.
Then in Perl_magic_getvec()\, return 0 if these flags are set\, and in Perl_magic_setvec() croak with a suitable error.

-- My get-up-and-go just got up and went.

p5pRT commented 7 years ago

From @ilmari

Dave Mitchell \davem@iabyn\.com writes:

This commit fixes things by\, in pp\_vec\(\)\, rather than croaking\, just set
flag bits in LvFLAGS\(\) to indicate that the offset is \-Ve / out\-of\-range\.

How about using named constants for the flag values\, like this?

Inline Patch

```diff diff --git a/doop.c b/doop.c index 18bc067d93..53ce47d454 100644 --- a/doop.c +++ b/doop.c @@ -920,8 +920,8 @@ Perl_do_vecset(pTHX_ SV *sv) /* some out-of-range errors have been deferred if/until the LV is * actually written to: f(vec($s,-1,8)) is not always fatal */ if (errflags) { - assert(!(errflags & ~(1|4))); - if (errflags & 1) + assert(!(errflags & ~(LVf_NEG_OFF|LVf_OUT_OF_RANGE))); + if (errflags & LVf_NEG_OFF) Perl_croak_nocontext("Negative offset to vec in lvalue context"); Perl_croak_nocontext("Out of memory!"); } diff --git a/mg.c b/mg.c index 969d183d6a..d5ab040134 100644 --- a/mg.c +++ b/mg.c @@ -2197,8 +2197,8 @@ Perl_magic_getsubstr(pTHX_ SV *sv, MAGIC *mg) const char * const tmps = SvPV_const(lsv,len); STRLEN offs = LvTARGOFF(sv); STRLEN rem = LvTARGLEN(sv); - const bool negoff = LvFLAGS(sv) & 1; - const bool negrem = LvFLAGS(sv) & 2; + const bool negoff = LvFLAGS(sv) & LVf_NEG_OFF; + const bool negrem = LvFLAGS(sv) & LVf_NEG_LEN; PERL_ARGS_ASSERT_MAGIC_GETSUBSTR; PERL_UNUSED_ARG(mg); @@ -2229,8 +2229,8 @@ Perl_magic_setsubstr(pTHX_ SV *sv, MAGIC *mg) SV * const lsv = LvTARG(sv); STRLEN lvoff = LvTARGOFF(sv); STRLEN lvlen = LvTARGLEN(sv); - const bool negoff = LvFLAGS(sv) & 1; - const bool neglen = LvFLAGS(sv) & 2; + const bool negoff = LvFLAGS(sv) & LVf_NEG_OFF; + const bool neglen = LvFLAGS(sv) & LVf_NEG_LEN; PERL_ARGS_ASSERT_MAGIC_SETSUBSTR; PERL_UNUSED_ARG(mg); @@ -2311,7 +2311,7 @@ Perl_magic_getvec(pTHX_ SV *sv, MAGIC *mg) PERL_UNUSED_ARG(mg); /* non-zero errflags implies deferred out-of-range condition */ - assert(!(errflags & ~(1|4))); + assert(!(errflags & ~(LVf_NEG_OFF|LVf_OUT_OF_RANGE))); sv_setuv(sv, errflags ? 0 : do_vecget(lsv, LvTARGOFF(sv), LvTARGLEN(sv))); return 0; diff --git a/pp.c b/pp.c index cc4cb59f7d..4ed6b850a3 100644 --- a/pp.c +++ b/pp.c @@ -3377,11 +3377,11 @@ PP(pp_substr) LvTARGOFF(ret) = pos1_is_uv || pos1_iv >= 0 ? (STRLEN)(UV)pos1_iv - : (LvFLAGS(ret) |= 1, (STRLEN)(UV)-pos1_iv); + : (LvFLAGS(ret) |= LVf_NEG_OFF, (STRLEN)(UV)-pos1_iv); LvTARGLEN(ret) = len_is_uv || len_iv > 0 ? (STRLEN)(UV)len_iv - : (LvFLAGS(ret) |= 2, (STRLEN)(UV)-len_iv); + : (LvFLAGS(ret) |= LVf_NEG_LEN, (STRLEN)(UV)-len_iv); PUSHs(ret); /* avoid SvSETMAGIC here */ RETURN; @@ -3488,12 +3488,12 @@ PP(pp_vec) /* avoid a large UV being wrapped to a negative value */ if (SvIOK_UV(offsetsv) && SvUVX(offsetsv) > (UV)IV_MAX) - errflags = 4; /* out of range */ + errflags = LVf_OUT_OF_RANGE; else if (iv < 0) - errflags = (1|4); /* negative offset, out of range */ + errflags = (LVf_NEG_OFF|LVf_OUT_OF_RANGE); #if PTRSIZE < IVSIZE else if (iv > Size_t_MAX) - errflags = 4; /* out of range */ + errflags = LVf_OUT_OF_RANGE; #endif else offset = (STRLEN)iv; diff --git a/sv.h b/sv.h index 5e9c5b6881..18e230516f 100644 --- a/sv.h +++ b/sv.h @@ -1402,6 +1402,10 @@ object type. Exposed to perl code via Internals::SvREADONLY(). #define LvTARGLEN(sv) ((XPVLV*) SvANY(sv))->xlv_targlen #define LvFLAGS(sv) ((XPVLV*) SvANY(sv))->xlv_flags +#define LVf_NEG_OFF 0x1 +#define LVf_NEG_LEN 0x2 +#define LVf_OUT_OF_RANGE 0x4 + #define IoIFP(sv) (sv)->sv_u.svu_fp #define IoOFP(sv) ((XPVIO*) SvANY(sv))->xio_ofp #define IoDIRP(sv) ((XPVIO*) SvANY(sv))->xio_dirp -- ```

2.11.0

-- - Twitter seems more influential [than blogs] in the 'gets reported in the mainstream press' sense at least. - Matt McLeod - That'd be because the content of a tweet is easier to condense down to a mainstream media article. - Calle Dybedahl

p5pRT commented 7 years ago

From @iabyn

On Fri\, Mar 31\, 2017 at 03:14:41PM +0100\, Dagfinn Ilmari Mannsåker wrote:

Dave Mitchell \davem@iabyn\.com writes:
This commit fixes things by\, in pp\_vec\, rather than croaking\, just set
flag bits in LvFLAGS to indicate that the offset is \-Ve / out\-of\-range\.
How about using named constants for the flag values\, like this?

I intend to after 5.26.0; but in this late stage of code freeze I'm trying to change as little as possible.

-- Please note that ash-trays are provided for the use of smokers\, whereas the floor is provided for the use of all patrons. -- Bill Royston

jkeenan commented 2 years ago

App-PDF-Link is doing fine on CPANtesters, so this ticket is closable.

Perl / perl5