rm: rm -f should not fail

[jlduran]

Fix an inverted logic bug, so the -f flag is always honored.

When trying to forcefully (-f) remove a nonexistent file on an msdosfs mount, an error is thrown:

# rm -f /boot/efi/foo*
rm: /boot/efi/foo*: Invalid argument

---

It looks like it might be another issue underneath, and maybe other logic "bugs" in the file. At any rate, a more thorough explanation of when rm -f can fail (non-POSIX-compliant, etc.). Technically, only the penultimate case is needed to fix the issue.
This pull request can be taken as a bug report, should it be the case.

[stesser]

The reason for the observed behavior is that rm -f /boot/efi/foo* passes a literal * to msdos_lookup().
This is an illegal character on file systems that originate in MS-DOS, and therefor EINVAL is returned.
The same behavior would be observed for any other file name that contains characters not allowed on that file system.
Therefore, checking for EINVAL in addition to ENOENT might be the correct fix to have rm -f not print a warning.
OTOH, this is actually a case of an invalid command (for this particular file system).

The msdos file system could be modified to accept *and ? in file names as valid but never matching on lookups (and continue to reject them for file creation or renaming). That would cause the warning to suppressed for this particular command (with foo* as the filename pattern), but would still warn for attempt to lookup names that cannot exist on such file systems.

[jlduran]

@stesser basically, either:

if (!fflag || (errno != ENOENT && errno != EINVAL)) {

Or better, modify the msdos file system to accept * and ?. The latter sounds like a better approach, although I have no idea how to do it. I'll have to look it up with it over the weekend. Thank you.

[stesser]

I can modify the msdos file system to return ENOENT for invalid file names, since I know that code after fixing a few long standing bugs:

diff --git a/sys/fs/msdosfs/msdosfs_lookup.c b/sys/fs/msdosfs/msdosfs_lookup.c
index 2a90339d0878..9478196846d2 100644
--- a/sys/fs/msdosfs/msdosfs_lookup.c
+++ b/sys/fs/msdosfs/msdosfs_lookup.c
@@ -88,9 +88,13 @@ msdosfs_lookup_checker(struct msdosfsmount *pmp, struct vnode *dvp,
 int
 msdosfs_lookup(struct vop_cachedlookup_args *ap)
 {
+       int error;
 
-       return (msdosfs_lookup_ino(ap->a_dvp, ap->a_vpp, ap->a_cnp, NULL,
-           NULL));
+       error = msdosfs_lookup_ino(ap->a_dvp, ap->a_vpp, ap->a_cnp, NULL,
+           NULL);
+       if (error == EINVAL)
+               error = ENOENT;
+       return (error);
 }
 
 struct deget_dotdot {

The result is as expected, rm -f succeeds without reporting an error:

# rm -f /boot/efi/foo*
# echo $?
0

But this patch does also affect, e.g., ls -l:

# ls -l /boot/efi/foo*
ls: /boot/efi/foo*: No such file or directory

Without the patch, the result has been ls: /boot/efi/foo*: Invalid argument, and the behavior with the patch is similar to what would be returned on UFS.

But another side effect is that mv also returns No such file or directoryinstead of Invalid argument when the target file name is invalid:

# touch /boot/efi/A.DAT
# mv /boot/efi/A.DAT /boot/efi/B*.DAT
mv: rename /boot/efi/A.DAT to /boot/efi/B*.DAT: No such file or directory

This is unexpected and might break existing code that uses rename() or scripts that check the result of mv!
On UFS, renaming to B*.DATwould work and would result in a file with an asterisk in its name (as it is valid in filenames on UFS).

[jlduran]

Thank you! At a first glance, I like it (even for mv)!

[jlduran]

I have updated the commits accordingly, to let this soak in/gather more feedback. Nobody wants breaking changes (even if they are for a greater good).

The tests have no real relevance, and should be dropped. They are here just for convenience:

/usr/tests/bin # kyua debug rm/rm_test:f_flag_msdosfs ; \
kyua debug ls/ls_tests:l_flag_nonexistent_msdosfs ; \
kyua debug mv/mv_test:wildcard_msdosfs

Thank you!

[jlduran]

Note to self:

The rm and the ls tests pass using NetBSD, OpenBSD, macOS and Linux.

The mv test:

On NetBSD:

# touch /boot/efi/A.DAT
# mv /boot/efi/a.dat /boot/efi/B*.DAT
# ls /boot/efi
b~1.dat

on OpenBSD/macOS:

# touch /boot/efi/A.DAT
# mv /boot/efi/A.dat /boot/efi/B*.DAT
# ls /boot/efi
B*.DAT

On Linux (Debian):

# touch /boot/efi/A.DAT
# mv /boot/efi/A.DAT /boot/efi/B*.DAT
mv: cannot move '/boot/efi/A.DAT' to a subdirectory of itself, '/boot/efi/B*.DAT'

[bsdimp]

So what's the status of this? Ready?

[jlduran]

Let's wait for @stesser , otherwise, please close it, and I'll try to document somehow that this is something that can happen on MS-DOS mounts on FreeBSD.

[stesser]

Let's wait for @stesser , otherwise, please close it, and I'll try to document somehow that this is something that can happen on MS-DOS mounts on FreeBSD.

See my comment above:

But another side effect is that mv also returns No such file or directory instead of Invalid argument when the target file name is invalid:

# touch /boot/efi/A.DAT
# mv /boot/efi/A.DAT /boot/efi/B*.DAT
mv: rename /boot/efi/A.DAT to /boot/efi/B*.DAT: No such file or directory

This is caused by namei() being called with the target filename and that invokes the patched msdosfs_lookup() and thus gets and returns the changed error code.

Seems that different operating systems behave very different in that case (as you showed in your comment above), this might not be an issue. I doubt that it will cause existing scripts or programs to fail in unepected ways, but if they did, it might be hard to identify this change as the cause.

But since the change affects namei() and functions that call it, more vnode operations will return ENOENT instead of ```EINVAL`` with this patch (when given an invalid MSDOS file name on an MSDOSFS file system).

The rename(2) man page mentions both EINVAL and ENOENT, but neither description mentions the case of an invalid target file name:

[ENOENT] A component of the from path does not exist, or a path prefix of to does not exist.

[EINVAL] The from argument is a parent directory of to, or an attempt is made to rename ‘.’ or ‘..’.

Maybe the case of a target name not allowed on some file system should be added, e.g. one of:

[EINVAL] The from argument is a parent directory of to, or an attempt is made to rename ‘.’ or ‘..’, or the target filename is invalid.

[ENOENT] A component of the from path does not exist, a path prefix of to does not exist, or the target filename is invalid.

If changing the return code of rename() is acceptable for this specific error case, I'll commit the patch to -CURRENT, but I'm not sure whether POLA allows merging to -STABLE.

[jlduran]

Since GitHub is not (at the moment) a bug-reporting platform, I propose moving this conversation to bug report 281033.

The bug report contains the three disposable tests with the desired output.

Thank you for elucidating on the topic!

[jlduran]

For completeness, this was landed as 0b2c159. Thank you!

[stesser]

The patch that landed was better than what I had proposed.
But the man-pages of neither rename nor open mention invalid target file names as cause of EINVAL (since they date back to a time when there were no invalid characters in Unix filenames).
I'll suggest man-page updates to address these error cases.

[jlduran]

It is a valid and reasonable request. I’ll come up with a patch. Thanks!

[jlduran]

This is a first draft. I just want to avoid using the word invalid to describe a condition in ENOENT:

rename(2):

[EINVAL] The from argument is a parent directory of to, or an attempt is made to rename . or .. [, or a component of the to path is invalid.]

open(2):

[ENOENT] A component of the from path does not exist, or a path prefix of to does not exist. [or a component of the to path does not exist.]

Thoughts? I based my words taking a cue from open from The Open Group Issue 8.