kill off the deprecated chdir("") behavior #14729
Comments
From @rjbsThe use of chdir("") or chdir(undef) to mean chdir() has been deprecated since commit ff01919 ... +Using chdir("") or chdir(undef) instead of explicit chdir() is Thirteen years is long enough. Let's make this fatal in perl v5.23.early. -- |
From @khwilliamsonOn 06/01/2015 05:15 PM, Ricardo SIGNES (via RT) wrote:
+1 |
|
The RT System itself - Status changed from 'new' to 'open' |
From @tonycozOn Mon, Jun 01, 2015 at 04:15:13PM -0700, Ricardo SIGNES wrote:
Fatal as in failing (returning false), or as in raising an exception? Tony |
From @epaThe normal idiom for using chdir() is chdir($d) or die "cannot chdir to So if it fails, the error is expected to be in $!. But that corresponds to This suggests that if chdir(undef) is to be disallowed, it should throw an -- |
From @tonycozOn Tue, Jun 02, 2015 at 10:21:28AM +0000, Ed Avis wrote:
We set errno ($!) in similar situations in other I/O functions. Using EINVAL or ENOENT would make sense here. Tony |
From @rjbsTony, Thanks for asking, because I had not given this enough thought beyond "deprecated so now fatal." Here's my train of thought, roughly: I had thought it should be fatal because it's a totally bogus input, and we really want to signal that the command has failed. This was a really shallow thought. :( Previously, on "chdir undef" or "chdir ''" the user got a warning and *maybe* it changed directory. It could have failed for whatever reason, like ENOENT. If they cared, they were already checking. If they don't, they don't. The issue is this: reasonably, if you say chdir("") that is "" relative to cwd, which is cwd. So don't chdir and remain in cwd. Then you'd return true. If a user's program has changed behavior now, from "" meaning HOME, they may not notice. Sure, they probably should have fixed the code during those thirteen years of warnings, but we don't have to be punitive. I think we should stringify the first argument and, if it's an empty string, return false and signal EINVAL. That might not be the best behavior — I think ideally it should act like chdir('.') — but I think this is safer. This also implies that chdir(undef) should warn about uninit-as-string, like mkdir does, which I think is right and proper. Any other thoughts? -- |
From @rjbsOn Tue Jun 02 15:34:03 2015, rjbs wrote:
Yes, thanks. Although you might think that chdir("") should be relative to cwd, that's not what the standard C library does. It returns in error and sets errno to ENOENT, so so can we. Thanks to Walter Mankowski for mentioning this and inspiring me to actually write some C to look into it. -- |
From waltman@mawode.comOn Tue, Jun 02, 2015 at 03:48:46PM -0700, Ricardo SIGNES via RT wrote:
Glad I could help. The logic, I suppose, is that there's no directory Walt |
From @rjbs* Walt Mankowski <waltman@mawode.com> [2015-06-02T18:59:51]
I guess, but there's no empty string directory under /tmp, but chdir("/tmp/") Well, we need not worry to much about it! The answer is still the same: return -- |
From @tonycozOn Tue Jun 02 15:48:45 2015, rjbs wrote:
Patch. Tested on Linux and Win32/amd64. Tony |
From @tonycoz0001-perl-125305-chdir-no-longer-behaves-like-chdir.patchFrom 8acfce0771b8340dd0fc9301a5b8d975854b34ed Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Wed, 3 Jun 2015 10:28:23 +1000
Subject: [PATCH] [perl #125305] chdir("") no longer behaves like chdir()
---
pod/perldiag.pod | 10 ----------
pp_sys.c | 5 +----
t/op/chdir.t | 30 ++++++------------------------
3 files changed, 7 insertions(+), 38 deletions(-)
diff --git a/pod/perldiag.pod b/pod/perldiag.pod
index 47c23eb..0adabf5 100644
--- a/pod/perldiag.pod
+++ b/pod/perldiag.pod
@@ -6658,16 +6658,6 @@ one. This doesn't make sense. Perl will continue, assuming a Unicode
happens to be ISO-8859-1 (Latin1) where this message is spurious and can
be ignored.
-=item Use of chdir('') or chdir(undef) as chdir() deprecated
-
-(D deprecated) chdir() with no arguments is documented to change to
-$ENV{HOME} or $ENV{LOGDIR}. chdir(undef) and chdir('') share this
-behavior, but that has been deprecated. In future versions they
-will simply fail.
-
-Be careful to check that what you pass to chdir() is defined and not
-blank, else you might find yourself in your home directory.
-
=item Use of /c modifier is meaningless in s///
(W regexp) You used the /c modifier in a substitution. The /c
diff --git a/pp_sys.c b/pp_sys.c
index 7c20f52..38eb295 100644
--- a/pp_sys.c
+++ b/pp_sys.c
@@ -3577,8 +3577,7 @@ PP(pp_chdir)
else if (!(gv = MAYBE_DEREF_GV(sv)))
tmps = SvPV_nomg_const_nolen(sv);
}
-
- if( !gv && (!tmps || !*tmps) ) {
+ else {
HV * const table = GvHVn(PL_envgv);
SV **svp;
@@ -3589,8 +3588,6 @@ PP(pp_chdir)
#endif
)
{
- if( MAXARG == 1 )
- deprecate("chdir('') or chdir(undef) as chdir()");
tmps = SvPV_nolen_const(*svp);
}
else {
diff --git a/t/op/chdir.t b/t/op/chdir.t
index 813b0ed..84159fe 100644
--- a/t/op/chdir.t
+++ b/t/op/chdir.t
@@ -10,10 +10,11 @@ BEGIN {
# possibilities into @INC.
unshift @INC, qw(t . lib ../lib);
require "./test.pl";
- plan(tests => 48);
+ plan(tests => 42);
}
use Config;
+use Errno qw(ENOENT);
my $IsVMS = $^O eq 'VMS';
@@ -150,29 +151,10 @@ sub check_env {
my $warning = '';
local $SIG{__WARN__} = sub { $warning .= join '', @_ };
-
-
- # Check the deprecated chdir(undef) feature.
-#line 64
- ok( chdir(undef), "chdir(undef) w/ only \$ENV{$key} set" );
- is( abs_path, $ENV{$key}, ' abs_path() agrees' );
- is( $warning, <<WARNING, ' got uninit & deprecation warning' );
-Use of uninitialized value in chdir at $0 line 64.
-Use of chdir('') or chdir(undef) as chdir() is deprecated at $0 line 64.
-WARNING
-
- chdir($Cwd);
-
- # Ditto chdir('').
- $warning = '';
-#line 76
- ok( chdir(''), "chdir('') w/ only \$ENV{$key} set" );
- is( abs_path, $ENV{$key}, ' abs_path() agrees' );
- is( $warning, <<WARNING, ' got deprecation warning' );
-Use of chdir('') or chdir(undef) as chdir() is deprecated at $0 line 76.
-WARNING
-
- chdir($Cwd);
+ $! = 0;
+ ok(!chdir(''), "chdir('') no longer implied chdir()");
+ is($!+0, ENOENT, 'check $! set appropriately');
+ is($warning, '', 'should no longer warn about deprecation');
}
}
--
1.7.10.4
|
From @tonycozOn Tue Jun 02 18:00:42 2015, tonyc wrote:
Applied as b4929cb. Tony |
|
@tonycoz - Status changed from 'open' to 'pending release' |
From @iabynOn Tue, Jun 16, 2015 at 06:19:08PM -0700, Tony Cook via RT wrote:
Coverity has spotted a problem with this commit. The code chdir FOO where FOO (a bareword) is clearly supposed to be an open filehandle, but $ perl5200 -we'chdir FOO' and then actually chdir to $HOME or whatever. Now it does the OS system call with a null pointer, i.e. chdir(NULL), Some options: 1. add a new croak text along the lines of 2. re-use an existing croak text: 3. make it equivalent to chdir('') 4. make it equivalent to chdir(undef) -- |
From @tonycozOn Wed, Jun 24, 2015 at 10:52:51AM +0100, Dave Mitchell wrote:
It should fail and warn in the same way as chdir to any other closed $ ./perl -we 'opendir my $dh, "t"; closedir $dh; chdir $dh; print "no croak\n"' With the attached patch: tony@mars:.../git/perl$ ./perl -we 'chdir FOO' Tony |
From @tonycoz0001-perl-125305-handle-chdir-to-closed-handle-correctly.patchFrom 8523bf5596b184defec1d787c3882ad5f0de22a5 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Thu, 25 Jun 2015 13:58:57 +1000
Subject: [PATCH] [perl #125305] handle chdir to closed handle correctly
---
pod/perldiag.pod | 4 ++++
pp_sys.c | 10 ++++++++++
t/op/chdir.t | 13 ++++++++++---
3 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/pod/perldiag.pod b/pod/perldiag.pod
index 1d53e5d..807ed8d 100644
--- a/pod/perldiag.pod
+++ b/pod/perldiag.pod
@@ -1559,6 +1559,10 @@ defined in the C<:alias> import argument to C<use charnames>, but they
could be defined by a translator installed into C<$^H{charnames}>.
See L<charnames/CUSTOM ALIASES>.
+=item chdir() on unopened filehandle %s
+
+(W unopened) You tried chdir() on a filehandle that was never opened.
+
=item \C no longer supported in regex; marked by S<<-- HERE> in m/%s/
(F) The \C character class used to allow a match of single byte within a
diff --git a/pp_sys.c b/pp_sys.c
index 6770063..61fb5a0 100644
--- a/pp_sys.c
+++ b/pp_sys.c
@@ -3594,6 +3594,16 @@ PP(pp_chdir)
SV * const sv = POPs;
if (PL_op->op_flags & OPf_SPECIAL) {
gv = gv_fetchsv(sv, 0, SVt_PVIO);
+ if (!gv) {
+ if (ckWARN(WARN_UNOPENED)) {
+ Perl_warner(aTHX_ packWARN(WARN_UNOPENED),
+ "chdir() on unopened filehandle %" SVf, sv);
+ }
+ SETERRNO(EBADF,RMS_IFI);
+ PUSHi(0);
+ TAINT_PROPER("chdir");
+ RETURN;
+ }
}
else if (!(gv = MAYBE_DEREF_GV(sv)))
tmps = SvPV_nomg_const_nolen(sv);
diff --git a/t/op/chdir.t b/t/op/chdir.t
index 84159fe..ed4a4af 100644
--- a/t/op/chdir.t
+++ b/t/op/chdir.t
@@ -10,11 +10,11 @@ BEGIN {
# possibilities into @INC.
unshift @INC, qw(t . lib ../lib);
require "./test.pl";
- plan(tests => 42);
+ plan(tests => 47);
}
use Config;
-use Errno qw(ENOENT);
+use Errno qw(ENOENT EBADF);
my $IsVMS = $^O eq 'VMS';
@@ -68,7 +68,7 @@ SKIP: {
$Cwd = abs_path;
SKIP: {
- skip("no fchdir", 16) unless $has_fchdir;
+ skip("no fchdir", 21) unless $has_fchdir;
my $has_dirfd = ($Config{d_dirfd} || $Config{d_dir_dd_fd} || "") eq "define";
ok(opendir(my $dh, "."), "opendir .");
ok(open(my $fh, "<", "op"), "open op");
@@ -121,6 +121,13 @@ SKIP: {
ok(closedir(H), "closedir");
ok(chdir(H), "fchdir to base");
ok(-f "cond.t", "verify that we are in 'base'");
+ ok(close(H), "close");
+ $! = 0;
+ ok(!chdir(H), "check we can't chdir to closed handle");
+ is(0+$!, EBADF, 'check $! set appropriately');
+ $! = 0;
+ ok(!chdir(NEVEROPENED), "check we can't chdir to never opened handle");
+ is(0+$!, EBADF, 'check $! set appropriately');
chdir ".." or die $!;
}
--
1.7.10.4
|
From @iabynOn Thu, Jun 25, 2015 at 01:59:31PM +1000, Tony Cook wrote:
ah yes. Agreed. Patch looks good to me. -- |
From @tonycozOn Thu Jun 25 00:39:38 2015, davem wrote:
Applied as 6012112. Tony |
From @khwilliamsonThank you for submitting this report. You have helped make Perl better. Perl 5.24.0 may be downloaded via https://metacpan.org/release/RJBS/perl-5.24.0 |
|
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#125305 (status was 'resolved')
Searchable as RT125305$
The text was updated successfully, but these errors were encountered: