Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
p5pRT opened this issue Mar 31, 2017 · 7 comments
Closed

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

p5pRT opened this issue Mar 31, 2017 · 7 comments
Labels
BBC Blead Breaks CPAN - changes in blead broke a cpan module(s)

Comments

@p5pRT
Copy link

p5pRT commented Mar 31, 2017

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

Searchable as RT131083$

@p5pRT
Copy link
Author

p5pRT commented Mar 31, 2017

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
Copy link
Author

p5pRT commented Mar 31, 2017

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\(\)&#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
Copy link
Author

p5pRT commented Mar 31, 2017

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

@p5pRT
Copy link
Author

p5pRT commented Mar 31, 2017

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
Copy link
Author

p5pRT commented Mar 31, 2017

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
Copy link
Author

p5pRT commented Mar 31, 2017

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

@xenu xenu removed the Severity Low label Dec 29, 2021
@jkeenan jkeenan added the BBC Blead Breaks CPAN - changes in blead broke a cpan module(s) label Mar 15, 2022
@jkeenan
Copy link
Contributor

jkeenan commented Mar 15, 2022

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

@jkeenan jkeenan closed this as completed Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BBC Blead Breaks CPAN - changes in blead broke a cpan module(s)
Projects
None yet
Development

No branches or pull requests

3 participants