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

Combination of File::Spec->catdir() and do FILE problematic #15976

Closed
p5pRT opened this issue May 12, 2017 · 13 comments
Closed

Combination of File::Spec->catdir() and do FILE problematic #15976

p5pRT opened this issue May 12, 2017 · 13 comments

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented May 12, 2017

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

Searchable as RT131296$

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 12, 2017

From @jkeenan

Created by jkeenan@zareason.(none)

See https://rt.cpan.org/Ticket/Display.html?id=121633.

The patch attached brings Time-HiRes's Makefile.PL into conformance with
no-dot-by-default-in-@​INC. It also explicitly demonstrates how
File​::Spec->catfile('.', @​other_args) works.

Perl Info

Flags:
     category=library
     severity=medium
     module=File::Spec

Site configuration information for perl 5.24.1:

Configured by jkeenan at Sat Jan 14 20:56:26 EST 2017.

Summary of my perl5 (revision 5 version 24 subversion 1) configuration:

   Platform:
     osname=linux, osvers=4.4.0-59-generic, archname=x86_64-linux
     uname='linux zareason 4.4.0-59-generic #80-ubuntu smp fri jan 6 
17:47:47 utc 2017 x86_64 x86_64 x86_64 gnulinux '
     config_args='-de 
-Dprefix=/home/jkeenan/perl5/perlbrew/perls/perl-5.24.1 
-Aeval:scriptdir=/home/jkeenan/perl5/perlbrew/perls/perl-5.24.1/bin'
     hint=recommended, useposix=true, d_sigaction=define
     useithreads=undef, usemultiplicity=undef
     use64bitint=define, use64bitall=define, uselongdouble=undef
     usemymalloc=n, bincompat5005=undef
   Compiler:
     cc='cc', ccflags ='-fwrapv -fno-strict-aliasing -pipe 
-fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE 
-D_FILE_OFFSET_BITS=64',
     optimize='-O2',
     cppflags='-fwrapv -fno-strict-aliasing -pipe 
-fstack-protector-strong -I/usr/local/include'
     ccversion='', gccversion='5.4.0 20160609', 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='double', nvsize=8, Off_t='off_t', 
lseeksize=8
     alignbytes=8, 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/5/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 /lib64 /usr/lib64
     libs=-lpthread -lnsl -ldb -ldl -lm -lcrypt -lutil -lc
     perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
     libc=libc-2.23.so, so=so, useshrplib=false, libperl=libperl.a
     gnulibc_version='2.23'
   Dynamic Linking:
     dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
     cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib 
-fstack-protector-strong'



@INC for perl 5.24.1:
 
/home/jkeenan/perl5/perlbrew/perls/perl-5.24.1/lib/site_perl/5.24.1/x86_64-linux
     /home/jkeenan/perl5/perlbrew/perls/perl-5.24.1/lib/site_perl/5.24.1
     /home/jkeenan/perl5/perlbrew/perls/perl-5.24.1/lib/5.24.1/x86_64-linux
     /home/jkeenan/perl5/perlbrew/perls/perl-5.24.1/lib/5.24.1


Environment for perl 5.24.1:
     HOME=/home/jkeenan
     LANG=en_US.UTF-8
     LANGUAGE=en_US
     LD_LIBRARY_PATH (unset)
     LOGDIR (unset)
 
PATH=/home/jkeenan/perl5/perlbrew/bin:/home/jkeenan/perl5/perlbrew/perls/perl-5.24.1/bin:/usr/lib/ccache:/usr/local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/home/jkeenan/bin:/home/jkeenan/bin/perl:/home/jkeenan/bin/shell
     PERLBREW_BASHRC_VERSION=0.78
     PERLBREW_HOME=/home/jkeenan/.perlbrew
     PERLBREW_MANPATH=/home/jkeenan/perl5/perlbrew/perls/perl-5.24.1/man
 
PERLBREW_PATH=/home/jkeenan/perl5/perlbrew/bin:/home/jkeenan/perl5/perlbrew/perls/perl-5.24.1/bin
     PERLBREW_PERL=perl-5.24.1
     PERLBREW_ROOT=/home/jkeenan/perl5/perlbrew
     PERLBREW_VERSION=0.78
     PERL_BADLANG (unset)
     PERL_WORKDIR=/home/jkeenan/gitwork/perl
     SHELL=/bin/bash

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 12, 2017

From @jkeenan

0001-Show-how-File-Spec-catfile-handles-leading.patch
From c0607ef1f1b523e71a423f1fc1a5debf4b6a6cf8 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Fri, 12 May 2017 08:58:56 -0400
Subject: [PATCH] Show how File::Spec->catfile handles leading '.'.

This implies need to modify code where, in:

        do $hints;

... $hints has been assigned a string composed by, e.g.,

        $hints = File::Spec->catfile('.', 'hints', 'Makefile.PL');
---
 dist/PathTools/t/Spec.t     | 2 ++
 dist/Time-HiRes/Makefile.PL | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/dist/PathTools/t/Spec.t b/dist/PathTools/t/Spec.t
index 0255bdb..8b9bcc7 100644
--- a/dist/PathTools/t/Spec.t
+++ b/dist/PathTools/t/Spec.t
@@ -61,6 +61,8 @@ my @tests = (
 [ "Unix->catfile('a', do { my \$x = 'b'.chr(0xaf); use utf8 (); utf8::upgrade(\$x); \$x })",   'a/b'.chr(0xaf)   ],
 ) : ()),
 [ "Unix->catfile(substr('foo', 2))", 'o' ],
+# https://rt.cpan.org/Ticket/Display.html?id=121633
+[ "Unix->catfile('.', 'hints', 'Makefile.PL')", 'hints/Makefile.PL' ],
 
 [ "Unix->splitpath('file')",            ',,file'            ],
 [ "Unix->splitpath('/d1/d2/d3/')",      ',/d1/d2/d3/,'      ],
diff --git a/dist/Time-HiRes/Makefile.PL b/dist/Time-HiRes/Makefile.PL
index ca4d4dc..66691fd 100644
--- a/dist/Time-HiRes/Makefile.PL
+++ b/dist/Time-HiRes/Makefile.PL
@@ -417,11 +417,11 @@ sub DEFINE {
 }
 
 sub init {
-    my $hints = File::Spec->catfile(".", "hints", "$^O.pl");
+    my $hints = File::Spec->catfile("hints", "$^O.pl");
     if (-f $hints) {
 	print "Using hints $hints...\n";
 	local $self;
-	do $hints;
+	do "./$hints";
 	if (exists $self->{LIBS}) {
 	    $LIBS = $self->{LIBS};
 	    print "Extra libraries: @$LIBS...\n";
-- 
2.7.4

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 14, 2017

From leejebay@hotmail.com

Configure and perl -V that showed this below.

CFLAGS='-m64 -mtune=nocona' ./Configure -Dprefix=/opt/tools -Dotherlibdirs=/opt/tools/testing/lib -d -A ccflags=-fPIC

Summary of my perl5 (revision 5 version 26 subversion 0) configuration​:
 
  Platform​:
  osname=linux
  osvers=3.13.0-105-generic
  archname=x86_64-linux
  uname='linux givengaindev 3.13.0-105-generic #152-ubuntu smp fri dec 2 15​:37​:11 utc 2016 x86_64 x86_64 x86_64 gnulinux '
  config_args='-Dprefix=/opt/tools -Dotherlibdirs=/opt/tools/testing/lib -d -A ccflags=-fPIC'
  hint=previous
  useposix=true
  d_sigaction=define
  useithreads=undef
  usemultiplicity=undef
  use64bitint=define
  use64bitall=define
  uselongdouble=undef
  usemymalloc=n
  default_inc_excludes_dot=define
  bincompat5005=undef
  Compiler​:
  cc='cc'
  ccflags ='-fPIC -fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -fPIC'
  optimize='-O2'
  cppflags='-fPIC -fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
  ccversion=''
  gccversion='4.8.4'
  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='double'
  nvsize=8
  Off_t='off_t'
  lseeksize=8
  alignbytes=8
  prototype=define
  Linker and Libraries​:
  ld='cc'
  ldflags =' -fstack-protector -L/usr/local/lib'
  libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/4.8/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 /usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/4.8/include-fixed /usr/include/x86_64-linux-gnu /usr/lib
  libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
  libc=libc-2.19.so
  so=so
  useshrplib=false
  libperl=libperl.a
  gnulibc_version='2.19'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs
  dlext=so
  d_dlsymun=undef
  ccdlflags='-Wl,-E'
  cccdlflags='-fPIC'
  lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector'

Characteristics of this binary (from libperl)​:
  Compile-time options​:
  HAS_TIMES
  PERLIO_LAYERS
  PERL_COPY_ON_WRITE
  PERL_DONT_CREATE_GVSV
  PERL_MALLOC_WRAP
  PERL_OP_PARENT
  PERL_PRESERVE_IVUV
  USE_64_BIT_ALL
  USE_64_BIT_INT
  USE_LARGE_FILES
  USE_LOCALE
  USE_LOCALE_COLLATE
  USE_LOCALE_CTYPE
  USE_LOCALE_NUMERIC
  USE_LOCALE_TIME
  USE_PERLIO
  USE_PERL_ATOF
  Locally applied patches​:
  RC1
  Built under linux
  Compiled at May 12 2017 12​:37​:32
  @​INC​:
  /opt/tools/lib/perl5/site_perl/5.26.0/x86_64-linux
  /opt/tools/lib/perl5/site_perl/5.26.0
  /opt/tools/lib/perl5/5.26.0/x86_64-linux
  /opt/tools/lib/perl5/5.26.0
  /opt/tools/lib/perl5/site_perl/5.24.0
  /opt/tools/lib/perl5/site_perl/5.16.3
  /opt/tools/lib/perl5/site_perl
  /opt/tools/testing/lib

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 14, 2017

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 14, 2017

From @Leont

On Fri, May 12, 2017 at 4​:34 PM, James E Keenan <perlbug-followup@​perl.org>
wrote​:

See https://rt.cpan.org/Ticket/Display.html?id=121633.

The patch attached brings Time-HiRes's Makefile.PL into conformance with
no-dot-by-default-in-@​INC. It also explicitly demonstrates how
File​::Spec->catfile('.', @​other_args) works.

I think this needs to be fixed before we put out an RC2

Leon

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 14, 2017

From @jkeenan

On Sun, 14 May 2017 14​:00​:19 GMT, LeonT wrote​:

On Fri, May 12, 2017 at 4​:34 PM, James E Keenan <perlbug-followup@​perl.org>
wrote​:

See https://rt.cpan.org/Ticket/Display.html?id=121633.

The patch attached brings Time-HiRes's Makefile.PL into conformance with
no-dot-by-default-in-@​INC. It also explicitly demonstrates how
File​::Spec->catfile('.', @​other_args) works.

I think this needs to be fixed before we put out an RC2

Leon

If you were to ask me to look at the following line of code​:

#####
my $hints = File​::Spec->catfile(".", "hints", "Makefile.PL");
#####

... and then state the content of $hints -- without looking at the documentation and without consulting all the tests for this syntax in dist/PathTools/t/Spec.t -- I would probably "off the top of my head" responded​:

#####
./hints/Makefile.PL
#####

If that were true, then the 'do $hints;' in dist/Time-HiRes/Makefile.PL would be perfectly fine.

But in reality, that line of code produces​:

#####
hints/Makefile.PL
#####

... in which case 'do $hints' would no longer be valid in the era of no-dot-by-default-in-@​INC. File​::Spec->catdir's behavior is plausible, but not necessarily what you would guess it to be.

AFAICT, this is the only place in the core distribution where the arguments to File​::Spec->catdir() start with '.' and where the result is used in 'do EXPR'.

There may well be lots of code on CPAN or in the wild that combines File​::Spec->catdir(".", @​other_args) and do EXPR. Perhaps someone could do a grep of CPAN to identify such cases. But I think that if my patch is applied we will no longer have a blocker to 5.26.0.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 15, 2017

From @jkeenan

On Sun, 14 May 2017 15​:09​:19 GMT, jkeenan wrote​:

On Sun, 14 May 2017 14​:00​:19 GMT, LeonT wrote​:

On Fri, May 12, 2017 at 4​:34 PM, James E Keenan <perlbug-
followup@​perl.org>
wrote​:

See https://rt.cpan.org/Ticket/Display.html?id=121633.

The patch attached brings Time-HiRes's Makefile.PL into conformance
with
no-dot-by-default-in-@​INC. It also explicitly demonstrates how
File​::Spec->catfile('.', @​other_args) works.

I think this needs to be fixed before we put out an RC2

Leon

If you were to ask me to look at the following line of code​:

#####
my $hints = File​::Spec->catfile(".", "hints", "Makefile.PL");
#####

... and then state the content of $hints -- without looking at the
documentation and without consulting all the tests for this syntax in
dist/PathTools/t/Spec.t -- I would probably "off the top of my head"
responded​:

#####
./hints/Makefile.PL
#####

If that were true, then the 'do $hints;' in dist/Time-
HiRes/Makefile.PL would be perfectly fine.

But in reality, that line of code produces​:

#####
hints/Makefile.PL
#####

... in which case 'do $hints' would no longer be valid in the era of
no-dot-by-default-in-@​INC. File​::Spec->catdir's behavior is
plausible, but not necessarily what you would guess it to be.

commit 8b69401
Author​: James E Keenan <jkeenan@​cpan.org>
AuthorDate​: Mon Mar 27 19​:31​:34 2017 -0400
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Fri Apr 7 14​:42​:24 2017 +0100

  fix cases where 'do file' should be 'do ./file'.
 
  These are some cases which weren't picked up by the test suite
  [ This is a subset of a set of fixes originally submitted by James as
  2 commits - DAPM]

Inline Patch
diff --git a/dist/Time-HiRes/Makefile.PL b/dist/Time-HiRes/Makefile.PL
index ccad6a3..ca4d4dc 100644
--- a/dist/Time-HiRes/Makefile.PL
+++ b/dist/Time-HiRes/Makefile.PL
@@ -417,7 +417,7 @@ sub DEFINE {
 }
 
 sub init {
-    my $hints = File::Spec->catfile("hints", "$^O.pl");
+    my $hints = File::Spec->catfile(".", "hints", "$^O.pl");
     if (-f $hints) {
[snip]

#####

Dave, apply my more recent patch as needed.

AFAICT, this is the only place in the core distribution where the
arguments to File​::Spec->catdir() start with '.' and where the result
is used in 'do EXPR'.

There may well be lots of code on CPAN or in the wild that combines
File​::Spec->catdir(".", @​other_args) and do EXPR. Perhaps someone
could do a grep of CPAN to identify such cases. But I think that if
my patch is applied we will no longer have a blocker to 5.26.0.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 15, 2017

From @jkeenan

On Sun, 14 May 2017 15​:09​:19 GMT, jkeenan wrote​:

On Sun, 14 May 2017 14​:00​:19 GMT, LeonT wrote​:

On Fri, May 12, 2017 at 4​:34 PM, James E Keenan <perlbug-
followup@​perl.org>
wrote​:

See https://rt.cpan.org/Ticket/Display.html?id=121633.

The patch attached brings Time-HiRes's Makefile.PL into conformance
with
no-dot-by-default-in-@​INC. It also explicitly demonstrates how
File​::Spec->catfile('.', @​other_args) works.

I think this needs to be fixed before we put out an RC2

Leon

If you were to ask me to look at the following line of code​:

#####
my $hints = File​::Spec->catfile(".", "hints", "Makefile.PL");
#####

... and then state the content of $hints -- without looking at the
documentation and without consulting all the tests for this syntax in
dist/PathTools/t/Spec.t -- I would probably "off the top of my head"
responded​:

#####
./hints/Makefile.PL
#####

If that were true, then the 'do $hints;' in dist/Time-
HiRes/Makefile.PL would be perfectly fine.

But in reality, that line of code produces​:

#####
hints/Makefile.PL
#####

... in which case 'do $hints' would no longer be valid in the era of
no-dot-by-default-in-@​INC. File​::Spec->catdir's behavior is
plausible, but not necessarily what you would guess it to be.

AFAICT, this is the only place in the core distribution where the
arguments to File​::Spec->catdir() start with '.' and where the result
is used in 'do EXPR'.

There may well be lots of code on CPAN or in the wild that combines
File​::Spec->catdir(".", @​other_args) and do EXPR. Perhaps someone
could do a grep of CPAN to identify such cases. But I think that if
my patch is applied we will no longer have a blocker to 5.26.0.

I grepped CPAN for files which contained 'catfile' and, within the next 10 lines of code, also matched '\bdo\b'. My program caught some false positives, but the only true positive I came up with was that in Time-HiRes's Makefile.PL -- the subject of this ticket.

Hence, once we correct that file, we no longer have a blocker for 5.26.0.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 16, 2017

From @xsawyerx

On 05/14/2017 03​:59 PM, Leon Timmermans wrote​:

On Fri, May 12, 2017 at 4​:34 PM, James E Keenan
<perlbug-followup@​perl.org <mailto​:perlbug-followup@​perl.org>> wrote​:

See https://rt.cpan.org/Ticket/Display.html?id=121633
\<https://rt.cpan.org/Ticket/Display.html?id=121633>.

The patch attached brings Time\-HiRes's Makefile\.PL into
conformance with
no\-dot\-by\-default\-in\-@&#8203;INC\.  It also explicitly demonstrates how
File&#8203;::Spec\->catfile\('\.'\, @&#8203;other\_args\) works\.

I think this needs to be fixed before we put out an RC2

I agree.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 22, 2017

From @iabyn

On Tue, May 16, 2017 at 11​:35​:52PM +0200, Sawyer X wrote​:

On 05/14/2017 03​:59 PM, Leon Timmermans wrote​:

On Fri, May 12, 2017 at 4​:34 PM, James E Keenan
<perlbug-followup@​perl.org <mailto​:perlbug-followup@​perl.org>> wrote​:

See https://rt.cpan.org/Ticket/Display.html?id=121633
\<https://rt.cpan.org/Ticket/Display.html?id=121633>.

The patch attached brings Time\-HiRes's Makefile\.PL into
conformance with
no\-dot\-by\-default\-in\-@&#8203;INC\.  It also explicitly demonstrates how
File&#8203;::Spec\->catfile\('\.'\, @&#8203;other\_args\) works\.

I think this needs to be fixed before we put out an RC2

I agree.

I've just pushed the Time​::HiRes part of James's patch as
v5.26.0-RC1-47-gba57084.
I think the File​::Spec part is best to leave for 5.27.x.

--
If life gives you lemons, you'll probably develop a citric acid allergy.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 18, 2017

From zefram@fysh.org

Time​::HiRes was fixed long ago. Is there any other action to take on
this ticket? I don't see any other specific distros mentioned, nor any
proposal for a more general approach to the issue.

-zefram

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 18, 2017

From @jkeenan

On Mon, 18 Dec 2017 03​:33​:08 GMT, zefram@​fysh.org wrote​:

Time​::HiRes was fixed long ago. Is there any other action to take on
this ticket? I don't see any other specific distros mentioned, nor any
proposal for a more general approach to the issue.

-zefram

In commit 9f6291f, I applied the PathTools part of my patch, which demonstrates how File​::Spec->catfile works when '.' is the first argument.

Marking ticket Resolved. Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 18, 2017

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

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

Successfully merging a pull request may close this issue.

None yet
1 participant