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
File::Copy::copy() "works" when given a directory as a from parameter #16419
Comments
From daver@activestate.comCreated by daver@activestate.comFile::Copy::copy() seems to expect that calling open $fh, '<', '/dir' or ... will fail. However, on HPUX this succeeds and reading from the filehandle Perl Info
|
From @jkeenanOn Tue, 13 Feb 2018 23:07:40 GMT, daver@activestate.com wrote:
As far as I can tell from reading "perldoc -f open" and "perldoc perlopentut", what happens when you call "open $fh, '<', 'something'" when 'something' is a directory is pretty much undefined. Hence, I would expect that when you call "copy $fh", you would get undefined behavior as well. Moreover, 'perldoc File::Copy' specifically guides you away from attempting to copy from filehandles: ##### Thank you very much. |
The RT System itself - Status changed from 'new' to 'open' |
From @autarchOn Wed, 14 Feb 2018 06:36:13 -0800, jkeenan wrote:
Right, which is why I think File::Copy should check if the from parameter is directory.
Which is not what my bug report was about. I'm talking about calling copy('/path/to/directory', 'foo') |
From @jkeenanOn Wed, 14 Feb 2018 15:24:05 GMT, autarch wrote:
In lib/File/Copy.pm, we have this code inside the definition of copy(): ##### my $from = shift; my $size; my $from_a_handle = (ref($from) If the first argument to copy() is a filehandle (albeit one, in this circumstance, opened to a directory rather than a file), then $from_a_handle is Perl-true and $from, when dumped, is a GLOB reference like this: ##### ... which, if fed to Data::Dumper, dumps to this: ##### Am I correct in thinking that, to achieve what you want, we would need to determine the "target" of '::$IN'? Is it possible to do that? (Note: Whatever we do about this, I think that, given the centrality of File::Copy in the core distribution, we should not attempt to rush any change into perl-5.28.) Thank you very much. -- |
From @autarchOn Wed, 14 Feb 2018 09:17:44 -0800, jkeenan wrote:
Obviously is File::Copy is given a handle as the from param there's nothing to check. Again, I'm talking about the case where the from parameter is a string and that strings refers to a directory. It seems like in that case it's worth bailing early and returning 0. |
From @jkeenanOn Wed, 14 Feb 2018 17:35:29 GMT, autarch wrote:
I tried implementing the bare minimum for what I think you were requesting. See patch attached. However, I got plenty of test failures. "Fixing" them implies revising long-held assumptions (perhaps implicit ones) about the behavior of copy(). I'm reluctant to proceed further. ##### Thank you very much. -- |
From @jkeenan0001-Prevent-first-argument-to-copy-from-being-a-director.patchFrom 1b332897594260fa49a8b2a549912ca244919c6b Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Wed, 14 Feb 2018 15:04:21 -0500
Subject: [PATCH] Prevent first argument to copy() from being a directory.
Per suggestion by Dave Rolsky in RT #132866.
But test failures suggest we need more thought about this.
---
lib/File/Copy.pm | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/lib/File/Copy.pm b/lib/File/Copy.pm
index b796451..2b4b0099 100644
--- a/lib/File/Copy.pm
+++ b/lib/File/Copy.pm
@@ -24,7 +24,7 @@ sub syscopy;
sub cp;
sub mv;
-$VERSION = '2.33';
+$VERSION = '2.34';
require Exporter;
@ISA = qw(Exporter);
@@ -84,6 +84,9 @@ sub copy {
|| UNIVERSAL::isa($from, 'GLOB')
|| UNIVERSAL::isa($from, 'IO::Handle'))
: (ref(\$from) eq 'GLOB'));
+ if (! $from_a_handle and -d $from) {
+ croak "First argument to copy() must be file or filehandle, not a directory";
+ }
my $to_a_handle = (ref($to)
? (ref($to) eq 'GLOB'
|| UNIVERSAL::isa($to, 'GLOB')
--
2.7.4
|
From @autarchOn Wed, 14 Feb 2018 12:07:11 -0800, jkeenan wrote:
copy() mostly calls carp() and then returns 0. I think the current code relies on open() returning false when given a directory: open $from_h, "<", $from or goto fail_open1; |
From @tonycozOn Wed, 14 Feb 2018 12:07:11 -0800, jkeenan wrote:
The problem is -d can modify $!, if that's preserved the tests pass: tony@mars:.../git/perl$ git diff Inline Patchdiff --git a/lib/File/Copy.pm b/lib/File/Copy.pm
index b796451e37..265636af60 100644
--- a/lib/File/Copy.pm
+++ b/lib/File/Copy.pm
@@ -89,7 +89,9 @@ sub copy {
|| UNIVERSAL::isa($to, 'GLOB')
|| UNIVERSAL::isa($to, 'IO::Handle'))
: (ref(\$to) eq 'GLOB'));
-
+ if (! $from_a_handle and do { local $!; -d $from }) {
+ croak "First argument to copy() must be file or filehandle, not a directory";
+ }
if (_eq($from, $to)) { # works for references, too
carp("'$from' and '$to' are identical (not copied)");
return 0;
though perhaps the local should be in a block to make the code less esoteric. (and the general change probably needs tests) Tony |
From @tonycozOn Wed, 14 Feb 2018 16:10:31 -0800, tonyc wrote:
Thinking about it, I'm not sure the test is correct. It was added as part of e55c0a8, presumably the extra tests Nicholas mentioned in https://rt-archive.perl.org/perl5/Ticket/Display.html?id=48078#txn-485730 The File::Copy documentation doesn't say anything about the value of $! on success. Tony |
Migrated from rt.perl.org#132866 (status was 'open')
Searchable as RT132866$
The text was updated successfully, but these errors were encountered: