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

.pmc not loaded if @INC has a trailing slash #13694

Closed
p5pRT opened this issue Mar 26, 2014 · 13 comments
Closed

.pmc not loaded if @INC has a trailing slash #13694

p5pRT opened this issue Mar 26, 2014 · 13 comments

Comments

@p5pRT
Copy link

p5pRT commented Mar 26, 2014

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

Searchable as RT121512$

@p5pRT
Copy link
Author

p5pRT commented Mar 26, 2014

From @schwern

Created by @schwern

If the @​INC directory where a .pm and .pmc are located has a trailing
slash, require will fail to load the .pmc. I suspect this bug was
added in eb70bb4

$ pwd
/Users/schwern/tmp

$ perl -I ~/tmp -wle 'use Foo; print Foo​::test()'
pmc

$ perl -I ~/tmp/ -wle 'use Foo; print Foo​::test()'
pm

$ cat Foo.pm
package Foo;

sub test { "pm" }

=pod

=head1 NAME

Foo - this is the pm

=cut

1;

$ cat Foo.pmc
package Foo;

sub test { "pmc" }

=pod

=head1 NAME

Foo - this is the pmc

=cut

1;

This bug appears in...
5.18.1
5.18.2
5.19.10

This bug does not appear in...
5.10.1
5.12.5
5.14.4
5.16.3

Perl Info

Flags:
    category=core
    severity=medium

Site configuration information for perl 5.18.2:

Configured by schwern at Thu Mar  6 14:40:23 PST 2014.

Summary of my perl5 (revision 5 version 18 subversion 2) configuration:

  Platform:
    osname=darwin, osvers=13.1.0, archname=darwin-2level
    uname='darwin windhund.local 13.1.0 darwin kernel version 13.1.0:
thu jan 16 19:40:37 pst 2014; root:xnu-2422.90.20~2release_x86_64 x86_64
i386 macbookpro8,1 darwin '
    config_args='-de
-Dprefix=/Users/schwern/perl5/perlbrew/perls/perl-5.18.2
-Aeval:scriptdir=/Users/schwern/perl5/perlbrew/perls/perl-5.18.2/bin'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=undef, usemultiplicity=undef
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-fno-common -DPERL_DARWIN -fno-strict-aliasing
-pipe -fstack-protector -I/opt/local/include',
    optimize='-O3',
    cppflags='-fno-common -DPERL_DARWIN -fno-strict-aliasing -pipe
-fstack-protector -I/opt/local/include'
    ccversion='', gccversion='4.2.1 Compatible Apple LLVM 5.0
(clang-500.2.79)', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t',
lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='env MACOSX_DEPLOYMENT_TARGET=10.3 cc', ldflags ='
-fstack-protector -L/opt/local/lib'
    libpth=/opt/local/lib /usr/lib
    libs=-lgdbm -ldbm -ldl -lm -lutil -lc
    perllibs=-ldl -lm -lutil -lc
    libc=, so=dylib, useshrplib=false, libperl=libperl.a
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=bundle, d_dlsymun=undef, ccdlflags=' '
    cccdlflags=' ', lddlflags=' -bundle -undefined dynamic_lookup
-L/opt/local/lib -fstack-protector'

Locally applied patches:



@INC for perl 5.18.2:

/Users/schwern/perl5/perlbrew/perls/perl-5.18.2/lib/site_perl/5.18.2/darwin-2level
    /Users/schwern/perl5/perlbrew/perls/perl-5.18.2/lib/site_perl/5.18.2
    /Users/schwern/perl5/perlbrew/perls/perl-5.18.2/lib/5.18.2/darwin-2level
    /Users/schwern/perl5/perlbrew/perls/perl-5.18.2/lib/5.18.2
    .


Environment for perl 5.18.2:
    DYLD_LIBRARY_PATH (unset)
    HOME=/Users/schwern
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)

PATH=/Users/schwern/perl5/perlbrew/bin:/Users/schwern/perl5/perlbrew/perls/perl-5.18.2/bin:/Users/schwern/bin:/opt/local/libexec/gnubin:/opt/local/bin:/opt/local/sbin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/usr/local/MacGPG2/bin
    PERLBREW_BASHRC_VERSION=0.67
    PERLBREW_HOME=/Users/schwern/.perlbrew
    PERLBREW_MANPATH=/Users/schwern/perl5/perlbrew/perls/perl-5.18.2/man

PERLBREW_PATH=/Users/schwern/perl5/perlbrew/bin:/Users/schwern/perl5/perlbrew/perls/perl-5.18.2/bin
    PERLBREW_PERL=perl-5.18.2
    PERLBREW_ROOT=/Users/schwern/perl5/perlbrew
    PERLBREW_VERSION=0.67
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Mar 29, 2014

From @iabyn

On Wed, Mar 26, 2014 at 03​:23​:06PM -0700, Michael G Schwern wrote​:

If the @​INC directory where a .pm and .pmc are located has a trailing
slash, require will fail to load the .pmc. I suspect this bug was
added in eb70bb4

A bisect comes up with this​:

commit 6b0bdd7
Author​: Matthew Horsfall (alh) <wolfsage@​gmail.com>
AuthorDate​: Thu Dec 27 10​:38​:08 2012 -0500
Commit​: Father Chrysostomos <sprout@​cpan.org>
CommitDate​: Sun Feb 10 12​:19​:15 2013 -0800

  RT-116192 - If a directory in @​INC already has a trailing '/', don't add another.

--
Nothing ventured, nothing lost.

@p5pRT
Copy link
Author

p5pRT commented Mar 29, 2014

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

@p5pRT
Copy link
Author

p5pRT commented Mar 31, 2014

From @wolfsage

On Sat, Mar 29, 2014 at 3​:28 PM, Dave Mitchell <davem@​iabyn.com> wrote​:

On Wed, Mar 26, 2014 at 03​:23​:06PM -0700, Michael G Schwern wrote​:

If the @​INC directory where a .pm and .pmc are located has a trailing
slash, require will fail to load the .pmc. I suspect this bug was
added in eb70bb4

A bisect comes up with this​:

commit 6b0bdd7
Author​: Matthew Horsfall (alh) <wolfsage@​gmail.com>
AuthorDate​: Thu Dec 27 10​:38​:08 2012 -0500
Commit​: Father Chrysostomos <sprout@​cpan.org>
CommitDate​: Sun Feb 10 12​:19​:15 2013 -0800

RT\-116192 \- If a directory in @&#8203;INC already has a trailing '/'\, don't add another\.

My apologies! Patch attached.

The original code always assumed a '/' would be appended, so it set
the length of the resulting SV to be +1.

With my earlier patch, this wasn't the case if the SV already ended in
a '/', so later checks on the SV that were based off the length of the
SV would be wrong.

New tests included.

-- Matthew Horsfall (alh)

@p5pRT
Copy link
Author

p5pRT commented Mar 31, 2014

From @wolfsage

0001-RT-121512-Allow-I-dir-with-trailing-slash-to-find-.p.patch
From 4f86a010d75c228240961c2e480bae9a51653237 Mon Sep 17 00:00:00 2001
From: "Matthew Horsfall (alh)" <wolfsage@gmail.com>
Date: Mon, 31 Mar 2014 08:25:33 -0400
Subject: [PATCH] RT-121512 - Allow -I/dir/ with trailing slash to find .pmc
 files.

6b0bdd7f2041803dc3ec72b53d28052705861967 updated -I to not add a
trailing '/' if one was already present, but failed to update the
length of the resulting SV to account for the lack of another character.
This caused later checks based off of that length to fail (in this case,
seeing if the last 3 characters of the string are .pm).
---
 MANIFEST          |    2 ++
 pp_ctl.c          |    5 ++++-
 t/run/flib/t2.pm  |    5 +++++
 t/run/flib/t2.pmc |    5 +++++
 t/run/switchM.t   |   10 +++++++++-
 5 files changed, 25 insertions(+), 2 deletions(-)
 create mode 100644 t/run/flib/t2.pm
 create mode 100644 t/run/flib/t2.pmc

diff --git a/MANIFEST b/MANIFEST
index 70565d5..0794152 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -5444,6 +5444,8 @@ t/run/dtrace.pl			For dtrace.t
 t/run/dtrace.t			Test for DTrace probes
 t/run/exit.t			Test perl's exit status.
 t/run/flib/broken.pm		Bad .pm file for switchM.t
+t/run/flib/t2.pm		Test for .pmcs with -I/dir/
+t/run/flib/t2.pmc		Test for .pmcs with -I/dir/
 t/run/fresh_perl.t		Tests that require a fresh perl.
 t/run/locale.t		Tests related to locale handling
 t/run/mad.t			Test vs MAD environment
diff --git a/pp_ctl.c b/pp_ctl.c
index 3c643d7..1352d38 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -3954,6 +3954,7 @@ PP(pp_require)
 		  if (path_searchable) {
 		    const char *dir;
 		    STRLEN dirlen;
+		    U8 ext_len = 1;
 
 		    if (SvOK(dirsv)) {
 			dir = SvPV_nomg_const(dirsv, dirlen);
@@ -3999,13 +4000,15 @@ PP(pp_require)
 			/* Avoid '<dir>//<file>' */
 			if (!dirlen || *(tmp-1) != '/') {
 			    *tmp++ = '/';
+			} else {
+			    ext_len = 0;
 			}
 
 			/* name came from an SV, so it will have a '\0' at the
 			   end that we can copy as part of this memcpy().  */
 			memcpy(tmp, name, len + 1);
 
-			SvCUR_set(namesv, dirlen + len + 1);
+			SvCUR_set(namesv, dirlen + len + ext_len);
 			SvPOK_on(namesv);
 		    }
 #  endif
diff --git a/t/run/flib/t2.pm b/t/run/flib/t2.pm
new file mode 100644
index 0000000..172a14e
--- /dev/null
+++ b/t/run/flib/t2.pm
@@ -0,0 +1,5 @@
+package t2;
+
+sub id { "t2pm" }
+
+1;
diff --git a/t/run/flib/t2.pmc b/t/run/flib/t2.pmc
new file mode 100644
index 0000000..e3894bc
--- /dev/null
+++ b/t/run/flib/t2.pmc
@@ -0,0 +1,5 @@
+package t2;
+
+sub id { "t2pmc" }
+
+1;
diff --git a/t/run/switchM.t b/t/run/switchM.t
index 72e8908..6a75100 100644
--- a/t/run/switchM.t
+++ b/t/run/switchM.t
@@ -8,7 +8,7 @@ use strict;
 
 require './test.pl';
 
-plan(2);
+plan(4);
 
 like(runperl(switches => ['-Irun/flib', '-Mbroken'], stderr => 1),
      qr/^Global symbol "\$x" requires explicit package name at run\/flib\/broken.pm line 6\./,
@@ -17,3 +17,11 @@ like(runperl(switches => ['-Irun/flib', '-Mbroken'], stderr => 1),
 like(runperl(switches => ['-Irun/flib/', '-Mbroken'], stderr => 1),
      qr/^Global symbol "\$x" requires explicit package name at run\/flib\/broken.pm line 6\./,
      "Ensure -Irun/flib/ produces correct filename in warnings");
+
+like(runperl(switches => ['-Irun/flib', '-Mt2', '-e "print t2::id()"'], stderr => 1),
+     qr/^t2pmc$/,
+     "Ensure -Irun/flib loads pmc");
+
+like(runperl(switches => ['-Irun/flib/', '-Mt2', '-e "print t2::id()"'], stderr => 1),
+     qr/^t2pmc$/,
+     "Ensure -Irun/flib/ loads pmc");
-- 
1.7.9.5

@p5pRT
Copy link
Author

p5pRT commented Mar 31, 2014

From @tonycoz

On Mon Mar 31 05​:32​:55 2014, alh wrote​:

RT-116192 - If a directory in @​INC already has a trailing '/', don't
add another.

My apologies! Patch attached.

The original code always assumed a '/' would be appended, so it set
the length of the resulting SV to be +1.

With my earlier patch, this wasn't the case if the SV already ended in
a '/', so later checks on the SV that were based off the length of the
SV would be wrong.

New tests included.

I think this patch or something like it should be applied to blead and backported.

The only issue I have with the patch itself is it could be simpler, instead of adding extra variable, the if could simply ++dirlen when it bumps tmp.

Nothing else within the scope depends on dirlen being the length of the original @​INC entry.

Tony

@p5pRT
Copy link
Author

p5pRT commented Apr 1, 2014

From @wolfsage

On Mon, Mar 31, 2014 at 7​:28 PM, Tony Cook via RT
<perlbug-followup@​perl.org> wrote​:

I think this patch or something like it should be applied to blead and backported.

The only issue I have with the patch itself is it could be simpler, instead of adding extra variable, the if could simply ++dirlen when it bumps tmp.

Nothing else within the scope depends on dirlen being the length of the original @​INC entry.

Thanks for the review Tony. Attached a version with your suggested changes.

Side question (related to both of these patches) - is it bad that in
the case of dirs that already end with '/', we're calling SvGROW with
one more byte than we end up using? Or is there nothing wrong with
SvGROW to an arbitrary length and SvCUR_set to a smaller size?

Thanks,

-- Matthew Horsfall (alh)

@p5pRT
Copy link
Author

p5pRT commented Apr 1, 2014

From @wolfsage

0001-RT-121512-Allow-I-dir-with-trailing-slash-to-find-.p.patch
From 3277d1b90c2bf865b14f5bf27134f6c28b9d607f Mon Sep 17 00:00:00 2001
From: "Matthew Horsfall (alh)" <wolfsage@gmail.com>
Date: Mon, 31 Mar 2014 08:25:33 -0400
Subject: [PATCH] RT-121512 - Allow -I/dir/ with trailing slash to find .pmc
 files.

6b0bdd7f2041803dc3ec72b53d28052705861967 updated -I to not add a
trailing '/' if one was already present, but failed to update the
length of the resulting SV to account for the lack of another character.
This caused later checks based off of that length to fail (in this case,
seeing if the last 3 characters of the string are .pm).
---
 MANIFEST          |    2 ++
 pp_ctl.c          |    3 +++
 t/run/flib/t2.pm  |    5 +++++
 t/run/flib/t2.pmc |    5 +++++
 t/run/switchM.t   |   10 +++++++++-
 5 files changed, 24 insertions(+), 1 deletion(-)
 create mode 100644 t/run/flib/t2.pm
 create mode 100644 t/run/flib/t2.pmc

diff --git a/MANIFEST b/MANIFEST
index 70565d5..0794152 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -5444,6 +5444,8 @@ t/run/dtrace.pl			For dtrace.t
 t/run/dtrace.t			Test for DTrace probes
 t/run/exit.t			Test perl's exit status.
 t/run/flib/broken.pm		Bad .pm file for switchM.t
+t/run/flib/t2.pm		Test for .pmcs with -I/dir/
+t/run/flib/t2.pmc		Test for .pmcs with -I/dir/
 t/run/fresh_perl.t		Tests that require a fresh perl.
 t/run/locale.t		Tests related to locale handling
 t/run/mad.t			Test vs MAD environment
diff --git a/pp_ctl.c b/pp_ctl.c
index 3c643d7..e13e450 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -3999,6 +3999,9 @@ PP(pp_require)
 			/* Avoid '<dir>//<file>' */
 			if (!dirlen || *(tmp-1) != '/') {
 			    *tmp++ = '/';
+			} else {
+			    /* So SvCUR_set reports the correct length below */
+			    dirlen--;
 			}
 
 			/* name came from an SV, so it will have a '\0' at the
diff --git a/t/run/flib/t2.pm b/t/run/flib/t2.pm
new file mode 100644
index 0000000..172a14e
--- /dev/null
+++ b/t/run/flib/t2.pm
@@ -0,0 +1,5 @@
+package t2;
+
+sub id { "t2pm" }
+
+1;
diff --git a/t/run/flib/t2.pmc b/t/run/flib/t2.pmc
new file mode 100644
index 0000000..e3894bc
--- /dev/null
+++ b/t/run/flib/t2.pmc
@@ -0,0 +1,5 @@
+package t2;
+
+sub id { "t2pmc" }
+
+1;
diff --git a/t/run/switchM.t b/t/run/switchM.t
index 72e8908..6a75100 100644
--- a/t/run/switchM.t
+++ b/t/run/switchM.t
@@ -8,7 +8,7 @@ use strict;
 
 require './test.pl';
 
-plan(2);
+plan(4);
 
 like(runperl(switches => ['-Irun/flib', '-Mbroken'], stderr => 1),
      qr/^Global symbol "\$x" requires explicit package name at run\/flib\/broken.pm line 6\./,
@@ -17,3 +17,11 @@ like(runperl(switches => ['-Irun/flib', '-Mbroken'], stderr => 1),
 like(runperl(switches => ['-Irun/flib/', '-Mbroken'], stderr => 1),
      qr/^Global symbol "\$x" requires explicit package name at run\/flib\/broken.pm line 6\./,
      "Ensure -Irun/flib/ produces correct filename in warnings");
+
+like(runperl(switches => ['-Irun/flib', '-Mt2', '-e "print t2::id()"'], stderr => 1),
+     qr/^t2pmc$/,
+     "Ensure -Irun/flib loads pmc");
+
+like(runperl(switches => ['-Irun/flib/', '-Mt2', '-e "print t2::id()"'], stderr => 1),
+     qr/^t2pmc$/,
+     "Ensure -Irun/flib/ loads pmc");
-- 
1.7.9.5

@p5pRT
Copy link
Author

p5pRT commented Apr 2, 2014

From @tonycoz

On Tue Apr 01 09​:42​:26 2014, alh wrote​:

Side question (related to both of these patches) - is it bad that in
the case of dirs that already end with '/', we're calling SvGROW with
one more byte than we end up using? Or is there nothing wrong with
SvGROW to an arbitrary length and SvCUR_set to a smaller size?

That isn't a problem, SvGROW() will often allocate a bit more space anyway.

pp_sysread() provides one example where we grow to a length where perhaps not all that space is used.

Tony

@p5pRT
Copy link
Author

p5pRT commented Apr 7, 2014

From @wolfsage

On Tue Apr 01 21​:12​:13 2014, tonyc wrote​:

On Tue Apr 01 09​:42​:26 2014, alh wrote​:

Side question (related to both of these patches) - is it bad that in
the case of dirs that already end with '/', we're calling SvGROW with
one more byte than we end up using? Or is there nothing wrong with
SvGROW to an arbitrary length and SvCUR_set to a smaller size?

That isn't a problem, SvGROW() will often allocate a bit more space
anyway.

pp_sysread() provides one example where we grow to a length where
perhaps not all that space is used.

Tony

Alright, thanks. I've pushed this to blead.

Closing.

-- Matthew Horsfall (alh)

--
-- Matthew Horsfall (alh)

@p5pRT
Copy link
Author

p5pRT commented Apr 7, 2014

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

@p5pRT p5pRT closed this as completed Apr 7, 2014
@p5pRT
Copy link
Author

p5pRT commented Apr 7, 2014

From @wolfsage

Relevant commit​: 9fdd5a7

-- Matthew Horsfall (alh)

@p5pRT
Copy link
Author

p5pRT commented Apr 7, 2014

From [Unknown Contact. See original ticket]

Relevant commit​: 9fdd5a7

-- Matthew Horsfall (alh)

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

No branches or pull requests

1 participant