File::moveFileTo deletes file in some cases

MacOSX and iOS
1

This is a critical bug in my opinion, because users can lose data because of this. It already happened in our case. Luckily the user had a backup of the file.

When the source and the destination are the same physical file but the path string is not identical, then JUCE deletes the file.

Here is the JUCE code:

bool File::moveFileTo (const File& newFile) const
{
    if (newFile.fullPath == fullPath) // this is the problem line
        return true;

    if (! exists())
        return false;

   #if ! NAMES_ARE_CASE_SENSITIVE
    if (*this != newFile)
   #endif
        if (! newFile.deleteFile()) // this deletes the source in this case
            return false;

    return moveInternal (newFile);
}

The string-compare-check if two directories/files are the same is too simplified in my opinion.

Paths can contain symbolic links and other things. JUCE needs to compare the real path to see if the two files are the same.

Here is another thread about the same problem:

2

Mixing up paths and files is tricky. If you must use juce::File, canonicalize your paths before creating instances.

This implementation is wrong for other reasons and you should not rely on it to move files. It will break if:

  • newFile is on a different filesystem.
  • newFile is on a filesystem with different case sensitivity then was checked at compile time (this cannot be done at compile time, although this doesn’t really matter for the check and is super pedantic)
  • The paths contains any symlinks, like you mention
  • There’s also a toctou bug here if you try and move to/from the same paths concurrently

Also, it’s incorrect to delete the new file before attempting to rename in POSIX. I’m not sure the windows specific code, but POSIX requires rename to handle this atomically and correctly (and leaves the new file in place if it exists and the call fails for any other reason, which prevents the data loss you’re reporting).

I believe you can replace this entire thing with a call to std::filesystem::rename but I’m not sure about the edge cases (which you probably don’t need to care about)

1 Like
3

I want to use JUCE for this. Some file operations are platform-specific. Things like File::moveFileTo access the file system anyway. I would expect that this works out of the box like POSIX std::filesystem::rename.

I can understand that File::getRelativePathFrom() only works with strings and in this case it is probably not smart to access the file system. For this case, it would be great to have something like getRealpath() to prepare a path as POSIX realpath does.

4

Like I said, if you must use juce::File, canonicalize the path before constructing the file (eg: use std::filesystem::path for your paths, and std::filesystem::canonical to get the canonical absolute path). You can convert an std::filesystem::path to a utf8 string or C string to give to juce::File.

juce::File predates std::filesystem which “just works” correctly. fwiw, it’s not POSIX. The libc/POSIX APIs have some issues that juce::File also fails to deal with.

I think this should be fixed, but just offering some workarounds (and highlighting other issues with that function). A correct crossplatform implementation would:

  • Canonicalize paths to verify if they refer to the same file
  • Rely on the implementation of rename to handle atomically updating the new file to avoid the toctou issue.
  • Fallback on copying the file if the rename fails because they’re on a different filesystem (this is actually a problem if you try and move a file out of a temporary directory on many linux distros)
1 Like
5

Thanks. That’s a good workaround. Looks like it also removes the symlinks.