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

[p5pRT]

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

Searchable as RT131083$

[p5pRT]

From @andk

bisect

---

d69c430 is the first bad commit
commit d69c430
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​: d69c430
  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]

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
------
d69c430 is the first bad commit
commit d69c430
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

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]

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

[p5pRT]

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 1b92e69
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]

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 --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]

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]

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