Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.85k stars 524 forks source link

File::Copy::copy does not set $! on error due to identical files #22144

Open vinc17fr opened 2 weeks ago

vinc17fr commented 2 weeks ago

Module: File::Copy

Description When File::Copy::copy returns 0 (error) due to identical files, it does not set $!.

Steps to Reproduce perl -MFile::Copy -e '$! = 0; File::Copy::copy("/dev/null","/dev/null") or die "Copy failed: $! (\$! = ".int($!).")";' which gives:

'/dev/null' and '/dev/null' are identical (not copied) at -e line 1.
Copy failed:  ($! = 0) at -e line 1.

Expected behavior $! should be different from 0 (perhaps 1) as the module documents:

All functions return 1 on success, 0 on failure. $! will be set if an error was encountered.

The bug is still present in https://github.com/Perl/perl5/blob/1edc2b4ee3077fc72d6364edfe0281ea10aab252/lib/File/Copy.pm#L91-L94 (there are 3 occurrences of the issue).

Note: I found that this issue was mentioned in latexmk 4.85.

jkeenan commented 2 weeks ago

FWIW, this behavior has been in place since 754f2cd0b96 back in 2005. See the discussion in https://github.com/Perl/perl5/issues/8009 (which started off life as rt.perl.org#36507).

vinc17fr commented 2 weeks ago

Well, this commit had a return 1 in one case and a return 0 in the other case, while the current code has return 0 (meaning a failure) in the 3 cases. In #8009, there was no mention of $! (I suspect that it was completely forgotten).

mauke commented 2 weeks ago

$! should be different from 0 (perhaps 1)

$! isn't just a random variable. It is an interface to the system's errno and strerror(errno). Blindly setting it to some number doesn't sound like a great idea. We'd have to find an error code that matches this case somewhat.

My potential candidates would be EEXIST ("File exists"), EPERM ("Operation not permitted"), or the generic EINVAL ("Invalid argument"). Of these, EPERM is probably the least appropriate (our case is not a permission issue), but EEXIST seems strangely compelling (POSIX description: "An existing file was mentioned in an inappropriate context").

Jake-perl commented 2 weeks ago

From this snippit:

if (_eq($from, $to)) { # works for references, too carp("'$from' and '$to' are identical (not copied)"); return 0; }

The issue here is that while the code correctly identifies identical files and returns 0, it doesn't set $! to indicate the specific error condition. By setting it to an appropriate error code like EPERM for example, you can confirm that the error condition is properly communicated to the caller.

vinc17fr commented 2 weeks ago

I initially thought that EPERM could be used to say that the operation is not permitted by the API. But POSIX actually clarifies this as "An attempt was made to perform an operation limited to processes with appropriate privileges or to the owner of a file or other resource." However, EINVAL could be OK; here, this is actually a pair of arguments that is invalid.

guest20 commented 2 weeks ago

What would the caller do in this situation? How would one recover from (a correctly signalled) "I didn't copy it because it's already there"?

It's almost like asking for copy at the same place as the original is nonsense which makes it sound very EINVALy

vinc17fr commented 2 weeks ago

It depends on the caller. For latexmk, "it's a non-error", thus the current version does nothing special. That said, when an error is detected with copy, it just outputs a warning and goes on. So, what would change with $! being non-zero is that an additional warning would be displayed with the error message associated with it.

sisyphus commented 2 weeks ago

How would one recover from (a correctly signalled) "I didn't copy it because it's already there"?

Interestingly (though perhaps of little importance) there's no guarantee that "it's already there":

> perl -MFile::Copy -e "File::Copy::copy('non-existent-file','non-existent-file') or die 'Copy failed:';"
'non-existent-file' and 'non-existent-file' are identical (not copied) at -e line 1.
Copy failed: at -e line 1.
vinc17fr commented 2 weeks ago

I would regard this as a bug, because this is inconsistent with the behavior on equivalent forms (such as ./foo and foo). If the file exists, these equivalent forms are correctly detected as being the same file, but not if the file does not exist:

qaa% touch foo
qaa% perl -MFile::Copy -e "File::Copy::copy('./foo','foo') or die"
'./foo' and 'foo' are identical (not copied) at -e line 1.
Died at -e line 1.
qaa% rm foo
qaa% perl -MFile::Copy -e "File::Copy::copy('./foo','foo') or die"
Died at -e line 1.
guest20 commented 2 weeks ago

If you ever look too close at a bug you'll see there are other bugs hiding behind it