Bug #47637 for PathTools: File::Spec->rel2abs() incorrect inside a symlinked dir.

Tue Jul 07 01:19:35 2009 mschwern [...] cpan.org - Ticket created

Subject:

File::Spec->rel2abs() incorrect inside a symlinked dir.

I've been noticing things failing on OS X because paths are in /private/var rather than /var. My CPAN build directory is inside /var. /var is really a symlink to /private/var, this is a normal OS X thing. When in /var, File::Spec->rel2abs() will use /private/var as its base, cwd() will use /var/local and getcwd() will report /private/var. $ perl -wle 'use Cwd; print getcwd' /private/var/local $ perl -wle 'use Cwd; print cwd' /var/local $ perl -wle 'use File::Spec; print File::Spec->rel2abs(".")' /private/var/local This is because getcwd() walks up the directory tree while cwd() will look at $PWD. The File::Spec->rel2abs docs state: If $base is not present or '', then Cwd::cwd() is used. But the code uses getcwd().

Tue Jul 07 01:39:20 2009 mschwern [...] cpan.org - Correspondence added

RT-Send-CC:

"steven.pritchard", steve [...] silug.org

Attached is a patch. It adds a test and changes File::Spec->_cwd to use cwd() as documented. I traced the change to getcwd() back through the history... - File::Spec on Unix now uses Cwd::getcwd() rather than Cwd::cwd() to get the current directory because I guess someone on p5p thought it was more appropriate. And the upstream blead change... commit 7241d76ad1edf173ef75728426b7546b242afbdd Author: Abhijit Menon-Sen <ams@wiw.org> Date: Wed Aug 8 16:28:40 2007 +0000 From #43633: Cwd::cwd() use in File::Spec::Unix use causes unnecessary fork( ) p4raw-id: //depot/perl@31686 diff --git a/lib/File/Spec/Unix.pm b/lib/File/Spec/Unix.pm index 5674731..98a24dd 100644 --- a/lib/File/Spec/Unix.pm +++ b/lib/File/Spec/Unix.pm @@ -475,7 +475,7 @@ L<File::Spec> # File::Spec subclasses use this. sub _cwd { require Cwd; - Cwd::cwd(); + Cwd::getcwd(); } Which was an optimization but it broke stuff so I guess we have to revert it. http://rt.perl.org/rt3/Public/Bug/Display.html?id=43633

From 04fcc2cb19da66b5bb4d0c66a0712eaa46fb6966 Mon Sep 17 00:00:00 2001 From: Michael G. Schwern <schwern@pobox.com> Date: Mon, 6 Jul 2009 22:38:23 -0700 Subject: [PATCH] Fix for rel2abs() when inside a symlink, like on OS X --- Changes | 7 +++++++ MANIFEST | 1 + lib/File/Spec/Unix.pm | 2 +- t/rel2abs_vs_symlink.t | 34 ++++++++++++++++++++++++++++++++++ 4 files changed, 43 insertions(+), 1 deletions(-) create mode 100644 t/rel2abs_vs_symlink.t diff --git a/Changes b/Changes index 29d0113..52f0612 100644 --- a/Changes +++ b/Changes @@ -1,5 +1,12 @@ Revision history for Perl distribution PathTools. +Next + Bug Fix + * Revert optimization introduced by [rt.perl.org 43633] which broke + File::Spec->rel2abs() when under a symlink [rt.cpan.org 47637]. + Broke in 3.2501 + + 3.30 - Sun May 10 10:55:00 2009 - Promote to stable release. diff --git a/MANIFEST b/MANIFEST index a1cf764..f43e1aa 100644 --- a/MANIFEST +++ b/MANIFEST @@ -25,6 +25,7 @@ t/lib/Test/More.pm t/lib/Test/Simple.pm t/lib/Test/Tutorial.pod t/rel2abs2rel.t +t/rel2abs_vs_symlink.t t/Spec.t t/taint.t t/tmpdir.t diff --git a/lib/File/Spec/Unix.pm b/lib/File/Spec/Unix.pm index 8fd2320..f3042df 100644 --- a/lib/File/Spec/Unix.pm +++ b/lib/File/Spec/Unix.pm @@ -480,7 +480,7 @@ L<File::Spec> # File::Spec subclasses use this. sub _cwd { require Cwd; - Cwd::getcwd(); + Cwd::cwd(); } diff --git a/t/rel2abs_vs_symlink.t b/t/rel2abs_vs_symlink.t new file mode 100644 index 0000000..cfbde01 --- /dev/null +++ b/t/rel2abs_vs_symlink.t @@ -0,0 +1,34 @@ +#!/usr/bin/perl -w + +# Test that rel2abs() works correctly when the process is under a symlink +# See [rt.cpan.org 47637] + +use strict; + +use File::Path; +use File::Spec; + +# Do this to simulate already being inside a symlinked directory +# and having $ENV{PWD} set. +use Cwd qw(chdir); + +use Test::More; + +plan skip_all => "needs symlink()" if !eval { symlink("", ""); 1 }; + +plan tests => 1; + +my $real_dir = "for_rel2abs_test"; +my $symlink = "link_for_rel2abs_test"; +mkdir $real_dir or die "Can't make $real_dir: $!"; +END { rmtree $real_dir } + +symlink $real_dir, $symlink or die "Can't symlink $real_dir => $symlink: $!"; +END { unlink $symlink } + +chdir $symlink or die "Can't chdir into $symlink: $!"; + +like( File::Spec->rel2abs("."), qr/$symlink/ ); + +# So the unlinking works +chdir ".."; -- 1.6.2.4

Tue Jul 07 01:39:22 2009 mschwern [...] cpan.org - Status changed from 'new' to 'open'

Tue Jul 07 18:39:16 2009 steve [...] silug.org - Correspondence added

On Tue Jul 07 01:39:20 2009, MSCHWERN wrote: Show quoted text

> From #43633: Cwd::cwd() use in File::Spec::Unix use causes > unnecessary fork( > ) > > p4raw-id: //depot/perl@31686 > > diff --git a/lib/File/Spec/Unix.pm b/lib/File/Spec/Unix.pm > index 5674731..98a24dd 100644 > --- a/lib/File/Spec/Unix.pm > +++ b/lib/File/Spec/Unix.pm > @@ -475,7 +475,7 @@ L<File::Spec> > # File::Spec subclasses use this. > sub _cwd { > require Cwd; > - Cwd::cwd(); > + Cwd::getcwd(); > } > > Which was an optimization but it broke stuff so I guess we have to > revert it. > http://rt.perl.org/rt3/Public/Bug/Display.html?id=43633

There has to be a better fix. This causes an unnecessary fork() and exec() of /bin/pwd for every use of FileHandle. The performance penalty is insane.

Tue Jul 07 19:56:58 2009 schwern [...] pobox.com - Correspondence added

CC:	mschwern [...] cpan.org
Subject:	Re: [rt.cpan.org #47637] File::Spec->rel2abs() incorrect inside a symlinked dir.
Date:	Tue, 07 Jul 2009 16:56:34 -0700
To:	bug-PathTools [...] rt.cpan.org
From:	Michael G Schwern <schwern [...] pobox.com>

Steven Pritchard via RT wrote: Show quoted text

> There has to be a better fix. This causes an unnecessary fork() and > exec() of /bin/pwd for every use of FileHandle. The performance penalty > is insane.

Can you provide a benchmark? Maybe we can fix FileHandle or IO::File. I see only calls to File::Spec->catfile and only in IO::Dir. IO::File unnecessarily uses File::Spec. FileHandle->new makes no call to File::Spec::Unix->_cwd. I see 5.8.8's IO::File had a call to rel2abs, but 5.10.0 does not. The log says this was fixed in IO 1.13_01. -- You know what the chain of command is? It's the chain I go get and beat you with 'til you understand who's in ruttin' command here. -- Jayne Cobb, "Firefly"

Tue Jul 07 20:07:18 2009 kenahoo [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #47637] File::Spec->rel2abs() incorrect inside a symlinked dir.
Date:	Tue, 7 Jul 2009 19:06:58 -0500
To:	bug-PathTools [...] rt.cpan.org
From:	Ken Williams <kenahoo [...] gmail.com>

On Tue, Jul 7, 2009 at 5:39 PM, Steven Pritchard via RT<bug-PathTools@rt.cpan.org> wrote: Show quoted text

> There has to be a better fix. This causes an unnecessary fork() and > exec() of /bin/pwd for every use of FileHandle. The performance penalty > is insane.

For that matter, the entire Cwd module is insane. For instance, last time I benchmarked the Module::Build test suite, the biggest consumer was cwd() and/or friends. I think we need to have a serious look at what OS-level utilities do (e.g. the source of `pwd`) and copy it.

Mon Nov 30 23:49:55 2009 mschwern [...] cpan.org - Correspondence added

Seeing as how the original reason for the optimization (FileHandle/IO::File) has gone away, could this patch be applied? It would make testing modules inside /var (which is really a symlink to /private/var) on OS X not fail.

Tue Dec 01 10:44:39 2009 kwilliams [...] cpan.org - Correspondence added

Done, thanks for your persistence. Checked in as revision 13621. Any chance you (under the aegis of P5P user) want to make a release?

Tue Dec 01 10:44:41 2009 kwilliams [...] cpan.org - Status changed from 'open' to 'resolved'

Tue Dec 01 11:07:09 2009 smueller [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #47637] File::Spec->rel2abs() incorrect inside a symlinked dir.
Date:	Tue, 01 Dec 2009 16:46:59 +0100
To:	bug-PathTools [...] rt.cpan.org
From:	Steffen Mueller <smueller [...] cpan.org>

Ken Williams via RT wrote: Show quoted text

> Queue: PathTools > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=47637 > > > Done, thanks for your persistence. Checked in as revision 13621. > > Any chance you (under the aegis of P5P user) want to make a release?

I could cut a release tonight. Schwern, do you think this would be appropriate for 5.12, i.e. an important enough bug fix to bother Jesse about? Cheers, Steffen

Tue Dec 01 11:07:09 2009 The RT System itself - Status changed from 'resolved' to 'open'

Tue Dec 01 15:48:31 2009 schwern [...] pobox.com - Correspondence added

Subject:	Re: [rt.cpan.org #47637] File::Spec->rel2abs() incorrect inside a symlinked dir.
Date:	Tue, 01 Dec 2009 12:47:50 -0800
To:	bug-PathTools [...] rt.cpan.org
From:	Michael G Schwern <schwern [...] pobox.com>

Steffen Mueller via RT wrote: Show quoted text

> Schwern, do you think this would be appropriate for 5.12, i.e. an > important enough bug fix to bother Jesse about?

It would be nice, but we've been living with this bug for a while so there's no urgency. -- 184. When operating a military vehicle I may *not* attempt something "I saw in a cartoon". -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army http://skippyslist.com/list/

Wed Dec 02 04:33:48 2009 smueller [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #47637] File::Spec->rel2abs() incorrect inside a symlinked dir.
Date:	Wed, 02 Dec 2009 10:33:26 +0100
To:	bug-PathTools [...] rt.cpan.org
From:	Steffen Mueller <smueller [...] cpan.org>

Hi, Michael G Schwern via RT wrote: Show quoted text

> Queue: PathTools > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=47637 > > > Steffen Mueller via RT wrote:

>> Schwern, do you think this would be appropriate for 5.12, i.e. an >> important enough bug fix to bother Jesse about?

> > It would be nice, but we've been living with this bug for a while so there's > no urgency.

I'm getting test failure on a rather boring linux box without symlinks involved in the build path: ERL_DL_NONLAZY=1 /usr/bin/perl "-MExtUtils::Command::MM" "-e" "test_harness(0, 'blib/lib', 'blib/arch')" t/*.t t/abs_path.t ............ 1/16 stat(dne): No such file or directory at t/abs_path.t line 65 stat(dne/sub_dne): No such file or directory at t/abs_path.t line 70 t/abs_path.t ............ ok t/crossplatform.t ....... ok t/cwd.t ................. ok t/Functions.t ........... ok t/rel2abs2rel.t ......... ok t/rel2abs_vs_symlink.t .. 1/1 # Failed test at t/rel2abs_vs_symlink.t line 31. # '/home/tsee/perl/PathTools/trunk/for_rel2abs_test' # doesn't match '(?-xism:link_for_rel2abs_test)' # Looks like you failed 1 test of 1. t/rel2abs_vs_symlink.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/1 subtests t/Spec.t ................ ok t/taint.t ............... ok t/tmpdir.t .............. ok t/win32.t ............... skipped: this is not win32 Cheers, Steffen

Wed Dec 02 06:20:44 2009 schwern [...] pobox.com - Correspondence added

Subject:	Re: [rt.cpan.org #47637] File::Spec->rel2abs() incorrect inside a symlinked dir.
Date:	Wed, 02 Dec 2009 03:20:08 -0800
To:	bug-PathTools [...] rt.cpan.org
From:	Michael G Schwern <schwern [...] pobox.com>

Steffen Mueller via RT wrote: Show quoted text

> I'm getting test failure on a rather boring linux box without symlinks > involved in the build path:

Can you figure out what *Cwd::cwd is pointing at? Simplest way is to check what \&Cwd::cwd is in the debugger. $ perl -dwle 'use Cwd; 1' Loading DB routines from perl5db.pl version 1.32 Editor support available. Enter h or `h h' for help, or `man perldebug' for more help. main::(-e:1): use Cwd; 1 DB<1> x \&Cwd::cwd 0 CODE(0x8a7244) -> &Cwd::_backtick_pwd in /usr/local/lib/perl5/5.10.1/darwin-thread-multi-64int-ld-2level/Cwd.pm:360-371 I can reproduce if I hard code that to the other option, Cwd::getcwd. You have no /bin/pwd? I'll look into what magic pwd does and see if we can make use of that. -- 151. The proper way to report to my Commander is "Specialist Schwarz, reporting as ordered, Sir" not "You can't prove a thing!" -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army http://skippyslist.com/list/

Wed Dec 02 07:59:23 2009 smueller [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #47637] File::Spec->rel2abs() incorrect inside a symlinked dir.
Date:	Wed, 02 Dec 2009 13:58:53 +0100
To:	bug-PathTools [...] rt.cpan.org
From:	Steffen Mueller <smueller [...] cpan.org>

Hi, Michael G Schwern via RT wrote: Show quoted text

> Queue: PathTools > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=47637 > > Can you figure out what *Cwd::cwd is pointing at? Simplest way is to check > what \&Cwd::cwd is in the debugger. > > $ perl -dwle 'use Cwd; 1' > > Loading DB routines from perl5db.pl version 1.32 > Editor support available. > > Enter h or `h h' for help, or `man perldebug' for more help. > > main::(-e:1): use Cwd; 1 > DB<1> x \&Cwd::cwd > 0 CODE(0x8a7244) > -> &Cwd::_backtick_pwd in > /usr/local/lib/perl5/5.10.1/darwin-thread-multi-64int-ld-2level/Cwd.pm:360-371

I felt at liberty to add a few -I's because I haven't installed the new version yet. By the way, this is an ubuntu 9.04 system perl 5.10.0. perl -Iblib/arch -Iblib/lib -dwle 'use Cwd; 1' Loading DB routines from perl5db.pl version 1.3 Editor support available. Enter h or `h h' for help, or `man perldebug' for more help. main::(-e:1): use Cwd; 1 DB<1> x \&Cwd::cwd 0 CODE(0x1713b98) -> &Cwd::_backtick_pwd in blib/lib/Cwd.pm:360-371 Show quoted text

> I can reproduce if I hard code that to the other option, Cwd::getcwd. You > have no /bin/pwd?

Umm. I have that. tsee@l3tsee:~/perl/PathTools/trunk$ which pwd /bin/pwd Show quoted text

> I'll look into what magic pwd does and see if we can make use of that.

I'm a little starved for good debugging time and don't have a good grasp of the issues involved in pwd stuff, but if I can test anything else for you, just let me know. Thanks for looking into this. Best regards, Steffen

Wed Dec 02 18:24:36 2009 schwern [...] pobox.com - Correspondence added

Subject:	Re: [rt.cpan.org #47637] File::Spec->rel2abs() incorrect inside a symlinked dir.
Date:	Wed, 02 Dec 2009 15:24:18 -0800
To:	bug-PathTools [...] rt.cpan.org
From:	Michael G Schwern <schwern [...] pobox.com>

Steffen Mueller via RT wrote: Show quoted text

> Loading DB routines from perl5db.pl version 1.3 > Editor support available. > > Enter h or `h h' for help, or `man perldebug' for more help. > > main::(-e:1): use Cwd; 1 > DB<1> x \&Cwd::cwd > 0 CODE(0x1713b98) > -> &Cwd::_backtick_pwd in blib/lib/Cwd.pm:360-371

Weird. /bin/pwd should be referencing PWD. Anyhow, its all screwed. As far as I can tell, the only way you can figure out that you're inside a symlink is by checking PWD and Perl doesn't keep that consistent. As soon as your program chdirs Cwd::_backtick_pwd() won't know its inside the symlink. So without a core change to chdir() to always track the cwd [1] Cwd consistently knowing its inside a symlink is not possible. Stepping back, the larger problem is not that Cwd isn't seeing the symlink, its that Cwd is inconsistent. Not only across versions but also across operating systems and across all the maddeningly different flavors of cwd functions. Code compares Cwd::cwd() with File::Spec->rel2abs() (using getcwd) and gets different results. Unfortunately, the function folks will tend most to reach for, cwd(), is also the potentially slowest and least consistent. For example, Path-Class 01-basic.t does this. Its an easy trap to fall into. So I'm not sure what to do, except that this patch should be reverted as it just shuffles the inconsistency around. [1] Which would make most of Cwd unnecessary, so perhaps it should be considered. -- 44. I am not the atheist chaplain. -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army http://skippyslist.com/list/

Thu Dec 03 05:00:17 2009 smueller [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #47637] File::Spec->rel2abs() incorrect inside a symlinked dir.
Date:	Thu, 03 Dec 2009 10:59:53 +0100
To:	bug-PathTools [...] rt.cpan.org
From:	Steffen Mueller <smueller [...] cpan.org>

Michael G Schwern via RT wrote: Show quoted text

> So I'm not sure what to do, except that this patch should be reverted as it > just shuffles the inconsistency around.

That's what you did, right? But then tests started failing, so we're in no-mans land. Or am I misunderstanding things? By the way: Jesse has agreed to absorb a PathTools into blead if the ONLY change is reverting the original change. Anything else will need another nod from him. Cheers, Steffen

Fri Dec 04 02:46:01 2009 schwern [...] pobox.com - Correspondence added

Subject:	Re: [rt.cpan.org #47637] File::Spec->rel2abs() incorrect inside a symlinked dir.
Date:	Thu, 03 Dec 2009 23:45:41 -0800
To:	bug-PathTools [...] rt.cpan.org
From:	Michael G Schwern <schwern [...] pobox.com>

Steffen Mueller via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=47637 > > > Michael G Schwern via RT wrote:

>> So I'm not sure what to do, except that this patch should be reverted as it >> just shuffles the inconsistency around.

> > That's what you did, right? But then tests started failing, so we're in > no-mans land. Or am I misunderstanding things?

Sorry, I meant revert my patch. Bring it back to what it was doing, using Cwd::getcwd. getcwd() is at least a bit more consistent than cwd(). -- But there's no sense crying over every mistake. You just keep on trying till you run out of cake. -- Jonathan Coulton, "Still Alive"

Fri Dec 04 03:42:39 2009 smueller [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #47637] File::Spec->rel2abs() incorrect inside a symlinked dir.
Date:	Fri, 04 Dec 2009 09:42:17 +0100
To:	bug-PathTools [...] rt.cpan.org
From:	Steffen Mueller <smueller [...] cpan.org>

Hi, Michael G Schwern via RT wrote: Show quoted text

> Queue: PathTools > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=47637 > > > Steffen Mueller via RT wrote:

>> <URL: https://rt.cpan.org/Ticket/Display.html?id=47637 > >> >> Michael G Schwern via RT wrote:

>>> So I'm not sure what to do, except that this patch should be reverted as it >>> just shuffles the inconsistency around.

>> That's what you did, right? But then tests started failing, so we're in >> no-mans land. Or am I misunderstanding things?

> > Sorry, I meant revert my patch. Bring it back to what it was doing, using > Cwd::getcwd. getcwd() is at least a bit more consistent than cwd().

Okay, I switched back to using getcwd() and still get the symlinks test failures. But then again, that is probably expected as you added those tests specifically with the getcwd->cwd change. Right? Should I remove the tests? Sorry for being a little confused about this. Cwd is one of those things I wouldn't want to be involved in any more than necessary :) Cheers, Steffen

Fri Dec 04 07:48:46 2009 schwern [...] pobox.com - Correspondence added

Subject:	Re: [rt.cpan.org #47637] File::Spec->rel2abs() incorrect inside a symlinked dir.
Date:	Fri, 04 Dec 2009 04:16:47 -0800
To:	bug-PathTools [...] rt.cpan.org
From:	Michael G Schwern <schwern [...] pobox.com>

Steffen Mueller via RT wrote: Show quoted text

> Okay, I switched back to using getcwd() and still get the symlinks test > failures. But then again, that is probably expected as you added those > tests specifically with the getcwd->cwd change. Right? Should I remove > the tests?

Yes, pull the whole patch, tests and all. -- 184. When operating a military vehicle I may *not* attempt something "I saw in a cartoon". -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army http://skippyslist.com/list/

Sat Nov 23 20:53:12 2013 jkeenan [...] cpan.org - Correspondence added

On Fri Dec 04 07:48:46 2009, schwern@pobox.com wrote: Show quoted text

> Steffen Mueller via RT wrote:

> > Okay, I switched back to using getcwd() and still get the symlinks test > > failures. But then again, that is probably expected as you added those > > tests specifically with the getcwd->cwd change. Right? Should I remove > > the tests?

> > Yes, pull the whole patch, tests and all. > >

Michael, Ken, Steffen: Discussion in this ticket appears to have petered out four years ago. Are there still issues which need to be addressed? Thank you very much. Jim Keenan

Sat Nov 23 22:29:17 2013 kwilliams [...] cpan.org - Correspondence added

I have less than no clue. I'd support closing this ticket.

Tue Nov 26 18:44:16 2013 jkeenan [...] cpan.org - Requestor JKEENAN added