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 breaks SREZIC/Tk-804.032.tar.gz #14202

Closed
p5pRT opened this issue Nov 2, 2014 · 10 comments
Labels

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Nov 2, 2014

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

Searchable as RT123103$

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 2, 2014

From @eserte

This is a bug report for perl from slaven@​rezic.de,
generated with the help of perlbug 1.40 running under perl 5.21.5.


Almost all Tk tests fail. A simple test case is

  $ perl5.21.5 -Mblib -MTk -MTk​::ColorEditor -e 'tkinit->ColorEditor()'
  Modification of a read-only value attempted at /home/e/eserte/src/CPAN.local/Tk/blib/lib/Tk/Widget.pm line 1219.
  at /home/e/eserte/src/CPAN.local/Tk/blib/lib/Tk/Widget.pm line 203.

Bisect says​:

a623f89 is the first bad commit
commit a623f89
Author​: Father Chrysostomos <sprout@​cpan.org>
Date​: Fri Sep 19 23​:12​:48 2014 -0700

  Implement the bipolar read-only system
 
  This fixes bugs related to Hash​::Util​::unlock accidentally unlocking
  internal scalars (e.g., that returned by undef()) and allowing them to
  be modified.
 
  Internal read-only values are now marked by two flags, the regular
  read-only flag, and the new ‘protected’ flag.
 
  Before this SvREADONLY served two purposes​:
 
  1) The code would use it to protect things that must not be modi-
  fied, ever (except when the core sees fit to do so).
  2) Hash​::Util and everybody else would use it to make this unmodifia-
  ble temporarily when requested by the user.
 
  Internals​::SvREADONLY serves the latter purpose and only flips the
  read-only flag, so things that need to stay read-only will remain so,
  because of the ‘other’ read-only flag, that CPAN doesn’t know about.
  (If you are a CPAN author, do not read this.)

Another possible victim​: TOBYINK/Acme-Futuristic-Perl-0.001.tar.gz
A sample fail report here​:
http​://www.cpantesters.org/cpan/report/39d793a4-614c-11e4-ba7c-ee3de0bfc7aa (which
also has a "Modification of a read-only value attempted" error)

Regards,
  Slaven



Flags​:
  category=core
  severity=high


Site configuration information for perl 5.21.5​:

Configured by eserte at Sun Oct 26 13​:16​:39 CET 2014.

Summary of my perl5 (revision 5 version 21 subversion 5) configuration​:
 
  Platform​:
  osname=freebsd, osvers=9.2-release, archname=amd64-freebsd
  uname='freebsd cvrsnica.herceg.de 9.2-release freebsd 9.2-release #0 r255898​: thu sep 26 22​:50​:31 utc 2013 root@​bake.isc.freebsd.org​:usrobjusrsrcsysgeneric amd64 '
  config_args='-D useshrplib=true -Dprefix=/usr/perl5.21.5p -Dusemymalloc=n -D cc=ccache cc -D usedevel=define -Dgccansipedantic -de'
  hint=recommended, useposix=true, d_sigaction=define
  useithreads=undef, usemultiplicity=undef
  use64bitint=define, use64bitall=define, uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='ccache cc', ccflags ='-DHAS_FPSETMASK -DHAS_FLOATINGPOINT_H -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_FORTIFY_SOURCE=2',
  optimize='-O2 -pipe',
  cppflags='-DHAS_FPSETMASK -DHAS_FLOATINGPOINT_H -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
  ccversion='', gccversion='4.2.1 20070831 patched [FreeBSD]', gccosandvers=''
  intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
  d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16, longdblkind=3
  ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
  alignbytes=8, prototype=define
  Linker and Libraries​:
  ld='ccache cc', ldflags ='-pthread -Wl,-E -fstack-protector -L/usr/local/lib'
  libpth=/usr/lib /usr/local/lib /usr/include/gcc/4.2 /usr/lib
  libs=-lgdbm -lm -lcrypt -lutil -lc
  perllibs=-lm -lcrypt -lutil -lc
  libc=, so=so, useshrplib=true, libperl=libperl.so
  gnulibc_version=''
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags=' -Wl,-R/usr/perl5.21.5p/lib/5.21.5/amd64-freebsd/CORE'
  cccdlflags='-DPIC -fPIC', lddlflags='-shared -L/usr/local/lib -fstack-protector'


@​INC for perl 5.21.5​:
  /usr/perl5.21.5p/lib/site_perl/5.21.5/amd64-freebsd
  /usr/perl5.21.5p/lib/site_perl/5.21.5
  /usr/perl5.21.5p/lib/5.21.5/amd64-freebsd
  /usr/perl5.21.5p/lib/5.21.5
  .


Environment for perl 5.21.5​:
  HOME=/home/e/eserte
  LANG (unset)
  LANGUAGE (unset)
  LC_ALL=de_DE.UTF-8
  LD_LIBRARY_PATH (unset)
  LOGDIR (unset)
  PATH=/usr/local/bin​:/usr/bin​:/bin​:/usr/local/sbin​:/usr/sbin​:/sbin​:/home/e/eserte/bin/freebsd9.1​:/home/e/eserte/bin/sh​:/home/e/eserte/bin​:/usr/games​:/home/e/eserte/devel
  PERLDOC=-MPod​::Perldoc​::ToTextOverstrike
  PERL_BADLANG (unset)
  PERL_HTML_DISPLAY_CLASS=HTML​::Display​::Mozilla
  SHELL=/usr/local/bin/zsh

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 2, 2014

From @cpansprout

On Sun Nov 02 10​:51​:47 2014, slaven@​rezic.de wrote​:

This is a bug report for perl from slaven@​rezic.de,
generated with the help of perlbug 1.40 running under perl 5.21.5.

-----------------------------------------------------------------
Almost all Tk tests fail. A simple test case is

$ perl5.21.5 -Mblib -MTk -MTk​::ColorEditor -e 'tkinit->ColorEditor()'
Modification of a read-only value attempted at
/home/e/eserte/src/CPAN.local/Tk/blib/lib/Tk/Widget.pm line 1219.
at /home/e/eserte/src/CPAN.local/Tk/blib/lib/Tk/Widget.pm line 203.

Bisect says​:

a623f89 is the first bad commit
commit a623f89
Author​: Father Chrysostomos <sprout@​cpan.org>
Date​: Fri Sep 19 23​:12​:48 2014 -0700

Implement the bipolar read-only system

I see that Tk’s objGlue.c does SvREADONLY_off in three places. Would you please do some testing to find out which one is problematic. One at a time, please change those to​:

  SvREADONLY_off(sv), SvFLAGS(sv) &=~ SVf_PROTECT;

and see which ones need to be adjusted to make tests pass. I am assuming it is the FixBuggyUTF8String function, which, if I understand https://rt.cpan.org/Ticket/Display.html?id=41436 correctly, can just be skipped on newer perls.

(Whatever you do, please don’t turn off SVf_PROTECT, as that would defeat its purpose. Let’s get to the bottom of this and see whether it is Tk or bleadperl that needs adjustment.)

Another possible victim​: TOBYINK/Acme-Futuristic-Perl-0.001.tar.gz
A sample fail report here​:
http​://www.cpantesters.org/cpan/report/39d793a4-614c-11e4-ba7c-
ee3de0bfc7aa (which
also has a "Modification of a read-only value attempted" error)

=head1 DESCRIPTION

Sets the C<< $] >> and C<< $^V >> variables to 7.

That’s exactly the kind of thing the new flag was intended to protect against. :-)

Maybe the new flag is too prevalent. It was mainly intended to protect values like &PL_sv_no (which !1 returns) and &PL_sv_undef (which undef returns), the first argument to DESTROY, and also constants in the op tree. Modifying those (which could happen by accident) can lead to hangs, crashes, or erratic behaviour.

For built-in variables, we probably don’t need to be so strict. That said, this is an Acme module, and what it is doing *will* lead to erratic behaviour.

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 2, 2014

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

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 3, 2014

From @eserte

Dana Ned 02. Stu 2014, 13​:51​:23, sprout reče​:

On Sun Nov 02 10​:51​:47 2014, slaven@​rezic.de wrote​:

This is a bug report for perl from slaven@​rezic.de,
generated with the help of perlbug 1.40 running under perl 5.21.5.

-----------------------------------------------------------------
Almost all Tk tests fail. A simple test case is

$ perl5.21.5 -Mblib -MTk -MTk​::ColorEditor -e 'tkinit->ColorEditor()'
Modification of a read-only value attempted at
/home/e/eserte/src/CPAN.local/Tk/blib/lib/Tk/Widget.pm line 1219.
at /home/e/eserte/src/CPAN.local/Tk/blib/lib/Tk/Widget.pm line 203.

Bisect says​:

a623f89 is the first bad commit
commit a623f89
Author​: Father Chrysostomos <sprout@​cpan.org>
Date​: Fri Sep 19 23​:12​:48 2014 -0700

Implement the bipolar read-only system

I see that Tk’s objGlue.c does SvREADONLY_off in three places. Would
you please do some testing to find out which one is problematic. One
at a time, please change those to​:

SvREADONLY_off(sv), SvFLAGS(sv) &=~ SVf_PROTECT;

and see which ones need to be adjusted to make tests pass. I am
assuming it is the FixBuggyUTF8String function, which, if I understand
https://rt.cpan.org/Ticket/Display.html?id=41436 correctly, can just
be skipped on newer perls.

(Whatever you do, please don’t turn off SVf_PROTECT, as that would
defeat its purpose. Let’s get to the bottom of this and see whether
it is Tk or bleadperl that needs adjustment.)

It's caused by the SvREADONLY_off call in Tcl_ObjMagic(). If I patch the SvFLAGS as proposed, then the Tk test suite works again. The call in FixBuggyUTF8String() is not problematic (and actually I think it's best if I skip this function for perl >= 5.10). The flow in MaybeForceList() is actually reversed (first SvREADONLY_on, then SvREADONLY_off).

Regards,
  Slaven

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 4, 2014

From @cpansprout

On Mon Nov 03 12​:12​:46 2014, slaven@​rezic.de wrote​:

Dana Ned 02. Stu 2014, 13​:51​:23, sprout reče​:
It's caused by the SvREADONLY_off call in Tcl_ObjMagic(). If I patch
the SvFLAGS as proposed, then the Tk test suite works again.

Is there any easy way to determine the SV’s provenance?

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 4, 2014

From @cpansprout

On Mon Nov 03 12​:12​:46 2014, slaven@​rezic.de wrote​:

It's caused by the SvREADONLY_off call in Tcl_ObjMagic(). If I patch
the SvFLAGS as proposed, then the Tk test suite works again. The call
in FixBuggyUTF8String() is not problematic (and actually I think it's
best if I skip this function for perl >= 5.10). The flow in
MaybeForceList() is actually reversed (first SvREADONLY_on, then
SvREADONLY_off).

I see the problem. sv_magic is croaking if you try to apply ‘ext’ magic to a read-only SV. It shouldn’t be doing that. Perl can’t know whether the magic will try to modify the SV or not, so it should give the benefit of the doubt. I’ll fix that, but you will need some #ifdefs.

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 4, 2014

From @cpansprout

On Mon Nov 03 16​:45​:22 2014, sprout wrote​:

On Mon Nov 03 12​:12​:46 2014, slaven@​rezic.de wrote​:

It's caused by the SvREADONLY_off call in Tcl_ObjMagic(). If I patch
the SvFLAGS as proposed, then the Tk test suite works again. The call
in FixBuggyUTF8String() is not problematic (and actually I think it's
best if I skip this function for perl >= 5.10). The flow in
MaybeForceList() is actually reversed (first SvREADONLY_on, then
SvREADONLY_off).

I see the problem. sv_magic is croaking if you try to apply ‘ext’
magic to a read-only SV. It shouldn’t be doing that. Perl can’t know
whether the magic will try to modify the SV or not, so it should give
the benefit of the doubt. I’ll fix that, but you will need some
#ifdefs.

Er, no, you won’t need any #ifdefs.

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 4, 2014

From @cpansprout

I believe I have fixed this in 1d5686e and 8c995ab. If I’m wrong, just say something and I’ll reopen it.

--

Father Chrysostomos

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 4, 2014

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

@p5pRT p5pRT closed this Nov 4, 2014
@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 5, 2014

From @eserte

Dana Pon 03. Stu 2014, 20​:18​:14, sprout reče​:

I believe I have fixed this in 1d5686e and 8c995ab. If I’m wrong,
just say something and I’ll reopen it.

Looks good, I produced a Tk PASS report with a recent bleadperl​:
http​://www.cpantesters.org/cpan/report/48380202-64ae-11e4-a24e-84e2e0bfc7aa

Thanks!
  Slaven

@p5pRT p5pRT added the Severity Low label Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.