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

Panic if 'use strict' and forget 'my' #10135

Closed
p5pRT opened this issue Feb 7, 2010 · 14 comments
Closed

Panic if 'use strict' and forget 'my' #10135

p5pRT opened this issue Feb 7, 2010 · 14 comments

Comments

@p5pRT
Copy link

p5pRT commented Feb 7, 2010

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

Searchable as RT72590$

@p5pRT
Copy link
Author

p5pRT commented Feb 7, 2010

From @khwilliamson

This is a bug report for perl from khw@​khw-desktop.nonet,
generated with the help of perlbug 1.39 running under perl 5.11.4.


This program causes a panic in blead. It is a regression from 5.10.0​:

use strict;
use charnames '​:full';
$x = qr/\N{LATIN CAPITAL LETTER A}/;



Flags​:
  category=core
  severity=medium


Site configuration information for perl 5.11.4​:

Configured by khw at Sat Jan 23 13​:32​:28 MST 2010.

Summary of my perl5 (revision 5 version 11 subversion 4) configuration​:
  Commit id​: d0a7635
  Platform​:
  osname=linux, osvers=2.6.27-16-generic, archname=i686-linux
  uname='linux khw-desktop 2.6.27-16-generic #1 smp tue dec 1
17​:56​:54 utc 2009 i686 gnulinux '
  config_args='-s -d -Dprefix=/home/khw/blead -Dusedevel
-D'optimize=-g3' -A'optimize=-g3' -A'optimize=-O0''
  hint=recommended, useposix=true, d_sigaction=define
  useithreads=undef, usemultiplicity=undef
  useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
  use64bitint=undef, use64bitall=undef, uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='cc', ccflags ='-DDEBUGGING -fno-strict-aliasing -pipe
-fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE
-D_FILE_OFFSET_BITS=64',
  optimize='-O0 -g3',
  cppflags='-DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector
-I/usr/local/include'
  ccversion='', gccversion='4.3.2', 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 =' -fstack-protector -L/usr/local/lib'
  libpth=/usr/local/lib /lib /usr/lib
  libs=-lnsl -ldl -lm -lcrypt -lutil -lc
  perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
  libc=/lib/libc-2.8.90.so, so=so, useshrplib=false, libperl=libperl.a
  gnulibc_version='2.8.90'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
  cccdlflags='-fPIC', lddlflags='-shared -g3 -g3 -O0 -L/usr/local/lib
-fstack-protector'

Locally applied patches​:


@​INC for perl 5.11.4​:
  /home/khw/blead/lib/perl5/site_perl/5.11.4/i686-linux
  /home/khw/blead/lib/perl5/site_perl/5.11.4
  /home/khw/blead/lib/perl5/5.11.4/i686-linux
  /home/khw/blead/lib/perl5/5.11.4
  /home/khw/blead/lib/perl5/site_perl
  .


Environment for perl 5.11.4​:
  HOME=/home/khw
  LANG=en_US.UTF-8
  LANGUAGE (unset)
  LD_LIBRARY_PATH (unset)
  LOGDIR (unset)

PATH=/home/khw/bin​:/home/khw/print/bin​:/bin​:/usr/local/sbin​:/usr/local/bin​:/usr/sbin​:/usr/bin​:/sbin​:/usr/games​:/opt/real/RealPlayer​:/home/khw/cxoffice/bin
  PERL_BADLANG (unset)
  SHELL=/bin/ksh

@p5pRT
Copy link
Author

p5pRT commented Feb 7, 2010

From @iabyn

On Sat, Feb 06, 2010 at 04​:36​:46PM -0800, karl williamson wrote​:

-----------------------------------------------------------------
This program causes a panic in blead. It is a regression from 5.10.0​:

use strict;
use charnames '​:full';
$x = qr/\N{LATIN CAPITAL LETTER A}/;
-----------------------------------------------------------------

Does this still happen with your \N{...} patch applied?

--
A major Starfleet emergency breaks out near the Enterprise, but
fortunately some other ships in the area are able to deal with it to
everyone's satisfaction.
  -- Things That Never Happen in "Star Trek" #13

@p5pRT
Copy link
Author

p5pRT commented Feb 7, 2010

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

@p5pRT
Copy link
Author

p5pRT commented Feb 7, 2010

From @nwc10

On Sat, Feb 06, 2010 at 04​:36​:46PM -0800, karl williamson wrote​:

This program causes a panic in blead. It is a regression from 5.10.0​:

use strict;
use charnames '​:full';
$x = qr/\N{LATIN CAPITAL LETTER A}/;

Specifically, this panic​:

panic​: top_env
perl​: perl.c​:542​: perl_destruct​: Assertion `PL_scopestack_ix == 1' failed.
Aborted

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Feb 8, 2010

From @khwilliamson

Dave Mitchell wrote​:

On Sat, Feb 06, 2010 at 04​:36​:46PM -0800, karl williamson wrote​:

-----------------------------------------------------------------
This program causes a panic in blead. It is a regression from 5.10.0​:

use strict;
use charnames '​:full';
$x = qr/\N{LATIN CAPITAL LETTER A}/;
-----------------------------------------------------------------

Does this still happen with your \N{...} patch applied?

No panic, but it still doesn't work properly​:

Use of uninitialized value $txt in pattern match (m//) at
lib/charnames.pm line 119.
Unknown charname 'LATIN CAPITAL LETTER A' at lib/unicore/Name.pl line 5
Execution of ../blead/testpanic.pl aborted due to compilation errors.

$txt should have been initialized in the statement at line 113​:
  $txt = do "unicore/Name.pl" unless $txt;

I did change Name.pl to include a closure to fix a bug in the future.
When I comment all that out, there is a slight change in the output to
includethe proper error message​:

Use of uninitialized value $txt in pattern match (m//) at
lib/charnames.pm line 119.
Unknown charname 'LATIN CAPITAL LETTER A' at lib/unicore/Name.pl line 5
Global symbol "$x" requires explicit package name at
../blead/testpanic.pl line 5.
Execution of ../blead/testpanic.pl aborted due to compilation errors.

valgrind shows no problems.

My guess now is that because of the missing 'my', something doesn't get
initialized that is needed for charnames to work, so the solution would
be to find that, and if so, do the initialization or skip charnames.
I'm not sure if charnames use is necessary for this bug.

So, is this a 5.12 stopper?

@p5pRT
Copy link
Author

p5pRT commented Feb 8, 2010

From @rgarcia

On 8 February 2010 05​:08, karl williamson <public@​khwilliamson.com> wrote​:

Dave Mitchell wrote​:

On Sat, Feb 06, 2010 at 04​:36​:46PM -0800, karl williamson wrote​:

-----------------------------------------------------------------
This program causes a panic in blead.  It is a regression from 5.10.0​:

use strict;
use charnames '​:full';
$x = qr/\N{LATIN CAPITAL LETTER A}/;
-----------------------------------------------------------------

Does this still happen with your \N{...} patch applied?

No panic, but it still doesn't work properly​:

Use of uninitialized value $txt in pattern match (m//) at lib/charnames.pm
line 119.
Unknown charname 'LATIN CAPITAL LETTER A' at lib/unicore/Name.pl line 5
Execution of ../blead/testpanic.pl aborted due to compilation errors.

$txt should have been initialized in the statement at line 113​:
   $txt = do "unicore/Name.pl" unless $txt;

I did change Name.pl to include a closure to fix a bug in the future. When I
comment all that out, there is a slight change in the output to includethe
proper error message​:

Use of uninitialized value $txt in pattern match (m//) at lib/charnames.pm
line 119.
Unknown charname 'LATIN CAPITAL LETTER A' at lib/unicore/Name.pl line 5
Global symbol "$x" requires explicit package name at ../blead/testpanic.pl
line 5.
Execution of ../blead/testpanic.pl aborted due to compilation errors.

valgrind shows no problems.

My guess now is that because of the missing 'my', something doesn't get
initialized that is needed for charnames to work, so the solution would be
to find that, and if so, do the initialization or skip charnames. I'm not
sure if charnames use is necessary for this bug.

I agree with the analysis.

So, is this a 5.12 stopper?

This is severely hampering error reporting. And having perl segfault
when you make a strictness mistake gives a bad impression. I would
vote for it being a blocker, yes.

@p5pRT
Copy link
Author

p5pRT commented Feb 8, 2010

From @khwilliamson

Rafael Garcia-Suarez wrote​:

On 8 February 2010 05​:08, karl williamson <public@​khwilliamson.com> wrote​:

Dave Mitchell wrote​:

On Sat, Feb 06, 2010 at 04​:36​:46PM -0800, karl williamson wrote​:

-----------------------------------------------------------------
This program causes a panic in blead. It is a regression from 5.10.0​:

use strict;
use charnames '​:full';
$x = qr/\N{LATIN CAPITAL LETTER A}/;
-----------------------------------------------------------------
Does this still happen with your \N{...} patch applied?

No panic, but it still doesn't work properly​:

Use of uninitialized value $txt in pattern match (m//) at lib/charnames.pm
line 119.
Unknown charname 'LATIN CAPITAL LETTER A' at lib/unicore/Name.pl line 5
Execution of ../blead/testpanic.pl aborted due to compilation errors.

$txt should have been initialized in the statement at line 113​:
$txt = do "unicore/Name.pl" unless $txt;

I did change Name.pl to include a closure to fix a bug in the future. When I
comment all that out, there is a slight change in the output to includethe
proper error message​:

Use of uninitialized value $txt in pattern match (m//) at lib/charnames.pm
line 119.
Unknown charname 'LATIN CAPITAL LETTER A' at lib/unicore/Name.pl line 5
Global symbol "$x" requires explicit package name at ../blead/testpanic.pl
line 5.
Execution of ../blead/testpanic.pl aborted due to compilation errors.

valgrind shows no problems.

My guess now is that because of the missing 'my', something doesn't get
initialized that is needed for charnames to work, so the solution would be
to find that, and if so, do the initialization or skip charnames. I'm not
sure if charnames use is necessary for this bug.

I should have phrased the above sentence​: "I'm not sure if the bug is
confined to charnames use"

I agree with the analysis.

So, is this a 5.12 stopper?

This is severely hampering error reporting. And having perl segfault
when you make a strictness mistake gives a bad impression. I would
vote for it being a blocker, yes.

@p5pRT
Copy link
Author

p5pRT commented Feb 9, 2010

From @obra

This is severely hampering error reporting. And having perl segfault
when you make a strictness mistake gives a bad impression. I would
vote for it being a blocker, yes.

Done. We now have 7 blockers.

@p5pRT
Copy link
Author

p5pRT commented Feb 11, 2010

From @rgarcia

On 9 February 2010 17​:11, Jesse Vincent <jesse@​fsck.com> wrote​:

This is severely hampering error reporting. And having perl segfault
when you make a strictness mistake gives a bad impression. I would
vote for it being a blocker, yes.

Done. We now have 7 blockers.

Fixed (or worked around) by :

commit 78c4a74
Author​: Rafael Garcia-Suarez <rgs@​consttype.org>
Date​: Thu Feb 11 10​:32​:01 2010 +0100

  [perl #72590] Panic if 'use strict' and forget 'my'

  That bug happens when we detect a compilation error in the statement
  being parsed, and when the continuation of the parsing of that same
  statement needs to load the file unicore/Name.pl via charnames.pm.
  In that case perl gets confused, fails to parse Name.pl because
  the parser is already in error, and also fails to properly rewind
  to a normal error-reporting state.

  This patch does not attempt to fix the whole error-reporting process;
  instead, it simply prevents perl from trying to load charnames if it has
  already recorded a parse error. So, in a way, it hides the bug under
  the carpet. However, this is a safe fix, suitable for a code-freeze
  stage.

Inline Patch
diff --git a/regcomp.c b/regcomp.c
index ecea32d..0b27364 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -6722,6 +6722,9 @@ S_reg_namedseq(pTHX_ RExC_state_t *pRExC_state, UV *valuep
         if ( he_str ) {
             sv_str = HeVAL(he_str);
             cached = 1;
+       } else if (PL_parser && PL_parser->error_count > 0) {
+           /* Don't attempt to load charnames if we're already in error */
+           vFAIL("Too many errors, cannot continue parsing");
         } else {
             dSP ;

@p5pRT
Copy link
Author

p5pRT commented Feb 11, 2010

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

@p5pRT p5pRT closed this as completed Feb 11, 2010
@p5pRT
Copy link
Author

p5pRT commented Feb 11, 2010

From @ikegami

On Thu, Feb 11, 2010 at 4​:38 AM, Rafael Garcia-Suarez <rgs@​consttype.org>wrote​:

+ } else if (PL_parser && PL_parser->error_count > 0) {
+ /* Don't attempt to load charnames if we're already in error */
+ vFAIL("Too many errors, cannot continue parsing");

Since any error is too many errors, isn't that message misleading?

@p5pRT
Copy link
Author

p5pRT commented Feb 11, 2010

From @rgarcia

On 11 February 2010 19​:53, Eric Brine <ikegami@​adaelis.com> wrote​:

On Thu, Feb 11, 2010 at 4​:38 AM, Rafael Garcia-Suarez <rgs@​consttype.org>
wrote​:

+       } else if (PL_parser && PL_parser->error_count > 0) {
+           /* Don't attempt to load charnames if we're already in error
*/
+           vFAIL("Too many errors, cannot continue parsing");

Since any error is too many errors, isn't that message misleading?

Possibly, but perl can report many errors at once​:

$ perl -Mstrict -e '$x,$y'
Global symbol "$x" requires explicit package name at -e line 1.
Global symbol "$y" requires explicit package name at -e line 1.
Execution of -e aborted due to compilation errors.

The message points out that it has already reported one (or many)
errors, and that it is bailing out.

@p5pRT
Copy link
Author

p5pRT commented Feb 14, 2010

From @khwilliamson

Rafael Garcia-Suarez via RT wrote​:

According to our records, your request regarding
"Panic if 'use strict' and forget 'my'"
has been resolved.

If you have any further questions or concerns, please respond to this message.

For other topics, please create a new ticket.

Please don't feel obligated to say "Thanks" or "Kudos" or "I owe you a beer" -- if you respond to this message it will reopen the ticket. If you must, please send email directly to the person who handled your ticket, and not to the tracking system.

<URL​: http​://rt.perl.org/rt3/Ticket/Display.html?id=72590 >

Note that this patch only affects the regex use of \N, not the double
quoted use done by the lexer. That doesn't cause a panic, but it does
generate error messages from within Name.pl.

But, if we move the \N processing entirely to the lexer for 5.12, the
regcomp.c patch will become irrelevant, and the code needed should be in
toke.c. I already have the code ready to do that

@p5pRT
Copy link
Author

p5pRT commented Feb 15, 2010

From @rgarcia

On 14 February 2010 04​:53, karl williamson <public@​khwilliamson.com> wrote​:

Note that this patch only affects the regex use of \N, not the double quoted
use done by the lexer.  That doesn't cause a panic, but it does generate
error messages from within Name.pl.

That's because unicore/Name.pl fails to load, actually, and doesn't
report error correctly​:

$ bleadperl -Mstrict -Mcharnames=​:full -e '$x="\N{LATIN CAPITAL LETTER A}"'
Use of uninitialized value $txt in pattern match (m//) at
/home/rafael/bleadperl/lib/charnames.pm line 119.
Unknown charname 'LATIN CAPITAL LETTER A' at
/home/rafael/bleadperl/lib/unicore/Name.pl line 1
Execution of -e aborted due to compilation errors.

With the small patch below, we have a "nicer" message :
Couldn't load Name.pl
Propagated at -e line 1, within string
Execution of -e aborted due to compilation errors.

--- a/lib/charnames.pm
+++ b/lib/charnames.pm
@​@​ -111,6 +111,7 @​@​ sub charnames
  ## Lines look like​:
  ## "0052\t\tLATIN CAPITAL LETTER R\n"
  $txt = do "unicore/Name.pl" unless $txt;
+ die "Couldn't load Name.pl\n" unless $txt;

  ## @​off will hold the index into the code/name string of the start and
  ## end of the name as we find it.

While it's better to check for error, in this case, the load failure
is due to some stack corruption while recovering on error, so I'd
rather leave it as is for now, until this is implemented :

But, if we move the \N processing entirely to the lexer for 5.12, the
regcomp.c patch will become irrelevant, and the code needed should be in
toke.c.  I already have the code ready to do that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant