Fix more encryption issues on exFAT filesystem

CHANGELOG.md

@@ -6,6 +6,7 @@
 
 ### Fixed
 * <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
+* Fixed several causes of "decryption failed" exceptions that could happen when opening multiple encrypted Realm files in the same process while using Apple/linux and storing the Realms on an exFAT file system. ([#7156](https://github.com/realm/realm-core/issues/7156), since the beginning) 
 * Update existing std exceptions thrown by the Sync Client to use Realm exceptions. ([#6255](https://github.com/realm/realm-core/issues/6255), since v10.2.0)
 
 ### Breaking changes

src/realm/alloc_slab.cpp

@@ -827,12 +827,19 @@ ref_type SlabAlloc::attach_file(const std::string& path, Config& cfg, util::Writ
         if (REALM_UNLIKELY(cfg.read_only))
             throw InvalidDatabase("Read-only access to empty Realm file", path);
 
-        const char* data = reinterpret_cast<const char*>(&empty_file_header);
-        m_file.write(data, sizeof empty_file_header); // Throws
-
-        // Pre-alloc initial space
         size_t initial_size = page_size(); // m_initial_section_size;
+        // exFAT does not allocate a unique id for the file until it is non-empty. It must be
+        // valid at this point because File::get_unique_id() is used to distinguish
+        // mappings_for_file in the encryption layer. So the prealloc() is required before
+        // interacting with the encryption layer in File::write().
+        // Pre-alloc initial space
         m_file.prealloc(initial_size);     // Throws
+        // seek() back to the start of the file in preparation for writing the header
+        // This sequence of File operations is protected from races by
+        // DB::m_controlmutex, so we know we are the only ones operating on the file
+        m_file.seek(0);
+        const char* data = reinterpret_cast<const char*>(&empty_file_header);
+        m_file.write(data, sizeof empty_file_header); // Throws
 
         bool disable_sync = get_disable_sync_to_disk() || cfg.disable_sync;
         if (!disable_sync)

src/realm/group.cpp

@@ -967,6 +967,10 @@ void Group::write(File& file, const char* encryption_key, uint_fast64_t version_
 
     file.set_encryption_key(encryption_key);
 
+    // Force the file system to allocate a node so we get a stable unique id.
+    // See File::get_unique_id(). This is used to distinguish encrypted mappings.
+    file.resize(1);
+
     // The aim is that the buffer size should be at least 1/256 of needed size but less than 64 Mb
     constexpr size_t upper_bound = 64 * 1024 * 1024;
     size_t min_space = std::min(get_used_space() >> 8, upper_bound);

src/realm/util/file.cpp

@@ -572,6 +572,32 @@ void File::close() noexcept
 #endif
 }
 
+void File::close_static(FileDesc fd)
+{
+#ifdef _WIN32
+    if (!fd)
+        return;
+
+    if (!CloseHandle(fd))
+        throw std::system_error(GetLastError(), std::system_category(),
+                                "CloseHandle() failed from File::close_static()");
+#else
+    if (fd < 0)
+        return;
+
+    int ret = -1;
+    do {
+        ret = ::close(fd);
+    } while (ret == -1 && errno == EINTR);
+
+    if (ret != 0) {
+        int err = errno; // Eliminate any risk of clobbering
+        if (err == EBADF || err == EIO)
+            throw SystemError(err, "File::close_static() failed");
+    }
+#endif
+}
+
 size_t File::read_static(FileDesc fd, char* data, size_t size)
 {
 #ifdef _WIN32 // Windows version
@@ -1558,22 +1584,39 @@ bool File::compare(const std::string& path_1, const std::string& path_2)
     return true;
 }
 
-bool File::is_same_file_static(FileDesc f1, FileDesc f2)
+bool File::is_same_file_static(FileDesc f1, FileDesc f2, const std::string& path1, const std::string& path2)
 {
-    return get_unique_id(f1) == get_unique_id(f2);
+    return get_unique_id(f1, path1) == get_unique_id(f2, path2);
 }
 
 bool File::is_same_file(const File& f) const
 {
     REALM_ASSERT_RELEASE(is_attached());
     REALM_ASSERT_RELEASE(f.is_attached());
-    return is_same_file_static(m_fd, f.m_fd);
+    return is_same_file_static(m_fd, f.m_fd, m_path, f.m_path);
+}
+
+FileDesc File::dup_file_desc(FileDesc fd)
+{
+    FileDesc fd_duped;
+#ifdef _WIN32
+    if (!DuplicateHandle(GetCurrentProcess(), fd, GetCurrentProcess(), &fd_duped, 0, FALSE, DUPLICATE_SAME_ACCESS))
+        throw std::system_error(GetLastError(), std::system_category(), "DuplicateHandle() failed");
+#else
+    fd_duped = dup(fd);
+
+    if (fd_duped == -1) {
+        int err = errno; // Eliminate any risk of clobbering
+        throw std::system_error(err, std::system_category(), "dup() failed");
+    }
+#endif // conditonal on _WIN32
+    return fd_duped;
 }
 
 File::UniqueID File::get_unique_id() const
 {
     REALM_ASSERT_RELEASE(is_attached());
-    return File::get_unique_id(m_fd);
+    return File::get_unique_id(m_fd, m_path);
 }
 
 FileDesc File::get_descriptor() const
@@ -1597,37 +1640,57 @@ std::optional<File::UniqueID> File::get_unique_id(const std::string& path)
         throw SystemError(GetLastError(), "CreateFileW failed");
     }
 
-    return get_unique_id(fileHandle);
+    return get_unique_id(fileHandle, path);
 #else // POSIX version
     struct stat statbuf;
     if (::stat(path.c_str(), &statbuf) == 0) {
+        if (statbuf.st_size == 0) {
+            // On exFAT systems the inode and device are not populated
+            // until the file has been allocated some space. This has led
+            // to bugs where a unique id returned here was reused by different
+            // files. Returning `none` here assumes that the caller knows this
+            // and will create non-empty files.
+            return none;
+        }
         return File::UniqueID(statbuf.st_dev, statbuf.st_ino);
     }
     int err = errno; // Eliminate any risk of clobbering
     // File doesn't exist
     if (err == ENOENT)
         return none;
-    throw SystemError(err, format_errno("fstat() failed: %1", err));
+    throw SystemError(err, format_errno("fstat() failed: %1 for '%2'", err, path));
 #endif
 }
 
-File::UniqueID File::get_unique_id(FileDesc file)
+File::UniqueID File::get_unique_id(FileDesc file, const std::string& debug_path)
 {
 #ifdef _WIN32 // Windows version
     REALM_ASSERT(file != nullptr);
     File::UniqueID ret;
     if (GetFileInformationByHandleEx(file, FileIdInfo, &ret.id_info, sizeof(ret.id_info)) == 0) {
-        throw std::system_error(GetLastError(), std::system_category(), "GetFileInformationByHandleEx() failed");
+        throw std::system_error(GetLastError(), std::system_category(),
+                                util::format("GetFileInformationByHandleEx() failed for '%1'", debug_path));
     }
 
     return ret;
 #else // POSIX version
     REALM_ASSERT(file >= 0);
     struct stat statbuf;
     if (::fstat(file, &statbuf) == 0) {
+        // On exFAT systems the inode and device are not populated
+        // until the file has been allocated some space. This has led
+        // to bugs where a unique id returned here was reused by different
+        // files. The following check ensures that this is not happening
+        // anywhere else in future code.
+        if (statbuf.st_size == 0) {
+            throw FileAccessError(ErrorCodes::FileOperationFailed,
+                                  "Attempt to get unique id on an empty file. This could be due to an external "
+                                  "process modifying Realm files.",
+                                  debug_path);
+        }
         return UniqueID(statbuf.st_dev, statbuf.st_ino);
     }
-    throw std::system_error(errno, std::system_category(), "fstat() failed");
+    throw std::system_error(errno, std::system_category(), util::format("fstat() failed for '%1'", debug_path));
 #endif
 }
 

src/realm/util/file.hpp

@@ -176,6 +176,7 @@ class File {
     /// regardless of whether this instance currently is attached to
     /// an open file.
     void close() noexcept;
+    static void close_static(FileDesc fd); // throws
 
     /// Check whether this File instance is currently attached to an
     /// open file.
@@ -529,7 +530,9 @@ class File {
     /// Both instances have to be attached to open files. If they are
     /// not, this function has undefined behavior.
     bool is_same_file(const File&) const;
-    static bool is_same_file_static(FileDesc f1, FileDesc f2);
+    static bool is_same_file_static(FileDesc f1, FileDesc f2, const std::string& path1, const std::string& path2);
+
+    static FileDesc dup_file_desc(FileDesc fd);
 
     /// Resolve the specified path against the specified base directory.
     ///
@@ -612,10 +615,12 @@ class File {
     std::string get_path() const;
 
     // Return none if the file doesn't exist. Throws on other errors.
+    // If the file does exist but has a size of zero, the file may be resized
+    // to force the file system to allocate a unique id.
     static std::optional<UniqueID> get_unique_id(const std::string& path);
 
     // Return the unique id for the file descriptor. Throws if the underlying stat operation fails.
-    static UniqueID get_unique_id(FileDesc file);
+    static UniqueID get_unique_id(FileDesc file, const std::string& debug_path);
 
     template <class>
     class Map;

src/realm/util/file_mapper.cpp

@@ -25,7 +25,6 @@
 #else
 #include <cerrno>
 #include <sys/mman.h>
-#include <unistd.h>
 #endif
 
 #include <realm/exceptions.hpp>
@@ -60,11 +59,6 @@
 #include <dispatch/dispatch.h>
 #endif
 
-#if REALM_ANDROID
-#include <linux/unistd.h>
-#include <sys/syscall.h>
-#endif
-
 #endif // enable encryption
 
 namespace {
@@ -92,12 +86,7 @@ size_t round_up_to_page_size(size_t size) noexcept
 
 // A list of all of the active encrypted mappings for a single file
 struct mappings_for_file {
-#ifdef _WIN32
-    HANDLE handle;
-#else
-    dev_t device;
-    ino_t inode;
-#endif
+    File::UniqueID file_unique_id;
     std::shared_ptr<SharedFileInfo> info;
 };
 
@@ -483,19 +472,12 @@ mapping_and_addr* find_mapping_for_addr(void* addr, size_t size)
 SharedFileInfo* get_file_info_for_file(File& file)
 {
     LockGuard lock(mapping_mutex);
-#ifndef _WIN32
     File::UniqueID id = file.get_unique_id();
-#endif
     std::vector<mappings_for_file>::iterator it;
     for (it = mappings_by_file.begin(); it != mappings_by_file.end(); ++it) {
-#ifdef _WIN32
-        auto fd = file.get_descriptor();
-        if (File::is_same_file_static(it->handle, fd))
-            break;
-#else
-        if (it->inode == id.inode && it->device == id.device)
+        if (it->file_unique_id == id) {
             break;
-#endif
+        }
     }
     if (it == mappings_by_file.end())
         return nullptr;
@@ -507,30 +489,19 @@ SharedFileInfo* get_file_info_for_file(File& file)
 namespace {
 EncryptedFileMapping* add_mapping(void* addr, size_t size, const FileAttributes& file, size_t file_offset)
 {
-#ifndef _WIN32
-    struct stat st;
-
-    if (fstat(file.fd, &st)) {
-        int err = errno; // Eliminate any risk of clobbering
-        throw std::system_error(err, std::system_category(), "fstat() failed");
-    }
-#endif
-
     size_t fs = to_size_t(File::get_size_static(file.fd));
     if (fs > 0 && fs < page_size())
         throw DecryptionFailed();
 
     LockGuard lock(mapping_mutex);
 
+    File::UniqueID fuid = File::get_unique_id(file.fd, file.path);
+
     std::vector<mappings_for_file>::iterator it;
     for (it = mappings_by_file.begin(); it != mappings_by_file.end(); ++it) {
-#ifdef _WIN32
-        if (File::is_same_file_static(it->handle, file.fd))
+        if (it->file_unique_id == fuid) {
             break;
-#else
-        if (it->inode == st.st_ino && it->device == st.st_dev)
-            break;
-#endif
+        }
     }
 
     // Get the potential memory allocation out of the way so that mappings_by_addr.push_back can't throw
@@ -540,24 +511,8 @@ EncryptedFileMapping* add_mapping(void* addr, size_t size, const FileAttributes&
         mappings_by_file.reserve(mappings_by_file.size() + 1);
         mappings_for_file f;
         f.info = std::make_shared<SharedFileInfo>(reinterpret_cast<const uint8_t*>(file.encryption_key));
-
-        FileDesc fd_duped;
-#ifdef _WIN32
-        if (!DuplicateHandle(GetCurrentProcess(), file.fd, GetCurrentProcess(), &fd_duped, 0, FALSE,
-                             DUPLICATE_SAME_ACCESS))
-            throw std::system_error(GetLastError(), std::system_category(), "DuplicateHandle() failed");
-        f.info->fd = f.handle = fd_duped;
-#else
-        fd_duped = dup(file.fd);
-
-        if (fd_duped == -1) {
-            int err = errno; // Eliminate any risk of clobbering
-            throw std::system_error(err, std::system_category(), "dup() failed");
-        }
-        f.info->fd = fd_duped;
-        f.device = st.st_dev;
-        f.inode = st.st_ino;
-#endif // conditonal on _WIN32
+        f.info->fd = File::dup_file_desc(file.fd);
+        f.file_unique_id = fuid;
 
         mappings_by_file.push_back(f); // can't throw due to reserve() above
         it = mappings_by_file.end() - 1;
@@ -576,12 +531,7 @@ EncryptedFileMapping* add_mapping(void* addr, size_t size, const FileAttributes&
     }
     catch (...) {
         if (it->info->mappings.empty()) {
-#ifdef _WIN32
-            bool b = CloseHandle(it->info->fd);
-            REALM_ASSERT_RELEASE(b);
-#else
-            ::close(it->info->fd);
-#endif
+            File::close_static(it->info->fd);
             mappings_by_file.erase(it);
         }
         throw;
@@ -600,16 +550,7 @@ void remove_mapping(void* addr, size_t size)
 
     for (std::vector<mappings_for_file>::iterator it = mappings_by_file.begin(); it != mappings_by_file.end(); ++it) {
         if (it->info->mappings.empty()) {
-#ifdef _WIN32
-            if (!CloseHandle(it->info->fd))
-                throw std::system_error(GetLastError(), std::system_category(), "CloseHandle() failed");
-#else
-            if (::close(it->info->fd) != 0) {
-                int err = errno;                // Eliminate any risk of clobbering
-                if (err == EBADF || err == EIO) // FIXME: how do we handle EINTR?
-                    throw std::system_error(err, std::system_category(), "close() failed");
-            }
-#endif
+            File::close_static(it->info->fd);
             mappings_by_file.erase(it);
             break;
         }

test/util/spawned_process.cpp

@@ -144,6 +144,9 @@ std::unique_ptr<SpawnedProcess> spawn_process(const std::string& test_name, cons
     if (getenv("UNITTEST_ENCRYPT_ALL")) {
         env_vars.push_back("UNITTEST_ENCRYPT_ALL=1");
     }
+    if (getenv("TMPDIR")) {
+        env_vars.push_back(util::format("TMPDIR=%1", getenv("TMPDIR")));
+    }
 
 #if REALM_ANDROID || REALM_IOS
     // posix_spawn() is unavailable on Android, and not permitted on iOS