From 36b18854503c03b8498dda9bb436fb9aaabad2b0 Mon Sep 17 00:00:00 2001 From: Nicola Cabiddu Date: Thu, 24 Aug 2023 15:14:53 +0100 Subject: [PATCH 1/5] throw an exception if File::unlock has failed --- CHANGELOG.md | 1 + src/realm/util/file.cpp | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e5550750d72..d7aa249ce09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ### Enhancements * (PR [#????](https://github.com/realm/realm-core/pull/????)) * Added support for server log messages that are enabled by sync protocol version 10. Appservices request id will be provided in a server log message in a future server release. ([PR #6476](https://github.com/realm/realm-core/pull/6476)) +* Throw an exception if `File::unlock` has failed, in order to inform the SDK that we are likely hitting some limitation on the OS filesystem, instead of crashing the application.([PR #6926](https://github.com/realm/realm-core/pull/6926)) ### Fixed * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) diff --git a/src/realm/util/file.cpp b/src/realm/util/file.cpp index 09c0f99f35b..611d65368f5 100644 --- a/src/realm/util/file.cpp +++ b/src/realm/util/file.cpp @@ -1098,7 +1098,10 @@ static void _unlock(int m_fd) do { r = flock(m_fd, LOCK_UN); } while (r != 0 && errno == EINTR); - REALM_ASSERT_RELEASE_EX(r == 0 && "File::unlock()", r, errno); + if (r && errno) { + throw RuntimeError(ErrorCodes::RuntimeError, + "File::unlock() has failed, this is likely due to some limitation in the OS file system."); + } } #endif From 2174e625bf93412b1539ddd297b7b1ef9377847c Mon Sep 17 00:00:00 2001 From: Nicola Cabiddu Date: Fri, 25 Aug 2023 10:29:12 +0100 Subject: [PATCH 2/5] code review --- src/realm/util/file.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/realm/util/file.cpp b/src/realm/util/file.cpp index 611d65368f5..d70cd98d43c 100644 --- a/src/realm/util/file.cpp +++ b/src/realm/util/file.cpp @@ -1098,9 +1098,15 @@ static void _unlock(int m_fd) do { r = flock(m_fd, LOCK_UN); } while (r != 0 && errno == EINTR); - if (r && errno) { - throw RuntimeError(ErrorCodes::RuntimeError, - "File::unlock() has failed, this is likely due to some limitation in the OS file system."); + if (r) { + std::string message = + "File::unlock() has failed, this is likely due to some limitation in the OS file system."; +#ifdef REALM_ANDROID + message += + " The most common cause is trying to use Realm on external storage. From Android API 30 this is no " + "longer supported. See https://developer.android.com/about/versions/11/privacy/storage for more details."; +#endif + throw SystemError(errno, message); } } #endif From bc7edfbdca5a0aff631e63b4af7d92cbf9c0fe8f Mon Sep 17 00:00:00 2001 From: Nicola Cabiddu Date: Thu, 31 Aug 2023 10:56:30 +0100 Subject: [PATCH 3/5] use same logic for all platforms for locking and unlocking the file --- CHANGELOG.md | 2 +- src/realm/util/file.cpp | 17 +++++++++-------- src/realm/util/file.hpp | 8 +++++++- 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1dbdd3a2907..0cf7706c7e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ ### Enhancements * (PR [#????](https://github.com/realm/realm-core/pull/????)) * Add a distinct error code for timeouts (SyncConnectTimeout) rather than using the same one as for less transient failures ([PR #6932](https://github.com/realm/realm-core/pull/6932)). -* Throw an exception if `File::unlock` has failed, in order to inform the SDK that we are likely hitting some limitation on the OS filesystem, instead of crashing the application.([PR #6926](https://github.com/realm/realm-core/pull/6926)) +* Throw an exception if `File::unlock` has failed, in order to inform the SDK that we are likely hitting some limitation on the OS filesystem, instead of crashing the application and use the same file locking logic for all the platforms.([PR #6926](https://github.com/realm/realm-core/pull/6926)) ### Fixed diff --git a/src/realm/util/file.cpp b/src/realm/util/file.cpp index d70cd98d43c..86ff68bf269 100644 --- a/src/realm/util/file.cpp +++ b/src/realm/util/file.cpp @@ -507,6 +507,7 @@ void File::open_internal(const std::string& path, AccessMode a, CreateMode c, in int fd = ::open(path.c_str(), flags2, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); if (0 <= fd) { m_fd = fd; + m_have_lock = false; if (success) *success = true; return; @@ -562,7 +563,8 @@ void File::close() noexcept if (m_fd < 0) return; - unlock(); + if (m_have_lock) + unlock(); int r = ::close(m_fd); REALM_ASSERT_RELEASE(r == 0); m_fd = -1; @@ -1212,9 +1214,9 @@ bool File::rw_lock(bool exclusive, bool non_blocking) bool File::lock(bool exclusive, bool non_blocking) { REALM_ASSERT_RELEASE(is_attached()); + REALM_ASSERT_RELEASE(!m_have_lock); #ifdef _WIN32 // Windows version - REALM_ASSERT_RELEASE(!m_have_lock); // Under Windows a file lock must be explicitely released before // the file is closed. It will eventually be released by the @@ -1259,8 +1261,10 @@ bool File::lock(bool exclusive, bool non_blocking) if (non_blocking) operation |= LOCK_NB; do { - if (flock(m_fd, operation) == 0) + if (flock(m_fd, operation) == 0) { + m_have_lock = true; return true; + } } while (errno == EINTR); int err = errno; // Eliminate any risk of clobbering if (err == EWOULDBLOCK) @@ -1271,10 +1275,10 @@ bool File::lock(bool exclusive, bool non_blocking) void File::unlock() noexcept { -#ifdef _WIN32 // Windows version if (!m_have_lock) return; +#ifdef _WIN32 // Windows version OVERLAPPED overlapped; overlapped.hEvent = 0; overlapped.OffsetHigh = 0; @@ -1285,11 +1289,8 @@ void File::unlock() noexcept REALM_ASSERT_RELEASE(r); m_have_lock = false; #else - // The Linux man page for flock() does not state explicitely that - // unlocking is idempotent, however, we will assume it since there - // is no mention of the error that would be reported if a - // non-locked file were unlocked. _unlock(m_fd); + m_have_lock = false; #endif } diff --git a/src/realm/util/file.hpp b/src/realm/util/file.hpp index 6c38f6f175c..b7314feb333 100644 --- a/src/realm/util/file.hpp +++ b/src/realm/util/file.hpp @@ -627,9 +627,9 @@ class File { class Streambuf; private: + bool m_have_lock = false; // Only valid when m_fd is not null #ifdef _WIN32 HANDLE m_fd = nullptr; - bool m_have_lock = false; // Only valid when m_fd is not null #else int m_fd = -1; #ifdef REALM_FILELOCK_EMULATION @@ -983,9 +983,12 @@ inline File::File(File&& f) noexcept #ifdef _WIN32 m_fd = f.m_fd; m_have_lock = f.m_have_lock; + f.m_have_lock = false; f.m_fd = nullptr; #else m_fd = f.m_fd; + m_have_lock = f.m_have_lock; + f.m_have_lock = false; #ifdef REALM_FILELOCK_EMULATION m_pipe_fd = f.m_pipe_fd; m_has_exclusive_lock = f.m_has_exclusive_lock; @@ -1003,10 +1006,13 @@ inline File& File::operator=(File&& f) noexcept #ifdef _WIN32 m_fd = f.m_fd; m_have_lock = f.m_have_lock; + f.m_have_lock = false; f.m_fd = nullptr; #else m_fd = f.m_fd; f.m_fd = -1; + m_have_lock = f.m_have_lock; + f.m_have_lock = false; #ifdef REALM_FILELOCK_EMULATION m_pipe_fd = f.m_pipe_fd; f.m_pipe_fd = -1; From cca4eb135be63fa2c03ab66aed8444a1ab518288 Mon Sep 17 00:00:00 2001 From: Nicola Cabiddu Date: Fri, 1 Sep 2023 12:03:29 +0100 Subject: [PATCH 4/5] code review --- src/realm/util/file.cpp | 4 +--- src/realm/util/file.hpp | 12 ++++-------- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/src/realm/util/file.cpp b/src/realm/util/file.cpp index 86ff68bf269..bfd517aeb5c 100644 --- a/src/realm/util/file.cpp +++ b/src/realm/util/file.cpp @@ -1285,13 +1285,11 @@ void File::unlock() noexcept overlapped.Offset = 0; overlapped.Pointer = 0; BOOL r = UnlockFileEx(m_fd, 0, 1, 0, &overlapped); - REALM_ASSERT_RELEASE(r); - m_have_lock = false; #else _unlock(m_fd); - m_have_lock = false; #endif + m_have_lock = false; } void File::rw_unlock() noexcept diff --git a/src/realm/util/file.hpp b/src/realm/util/file.hpp index b7314feb333..17538fdb54a 100644 --- a/src/realm/util/file.hpp +++ b/src/realm/util/file.hpp @@ -982,13 +982,9 @@ inline File::File(File&& f) noexcept { #ifdef _WIN32 m_fd = f.m_fd; - m_have_lock = f.m_have_lock; - f.m_have_lock = false; f.m_fd = nullptr; #else m_fd = f.m_fd; - m_have_lock = f.m_have_lock; - f.m_have_lock = false; #ifdef REALM_FILELOCK_EMULATION m_pipe_fd = f.m_pipe_fd; m_has_exclusive_lock = f.m_has_exclusive_lock; @@ -997,6 +993,8 @@ inline File::File(File&& f) noexcept #endif f.m_fd = -1; #endif + m_have_lock = f.m_have_lock; + f.m_have_lock = false; m_encryption_key = std::move(f.m_encryption_key); } @@ -1005,14 +1003,10 @@ inline File& File::operator=(File&& f) noexcept close(); #ifdef _WIN32 m_fd = f.m_fd; - m_have_lock = f.m_have_lock; - f.m_have_lock = false; f.m_fd = nullptr; #else m_fd = f.m_fd; f.m_fd = -1; - m_have_lock = f.m_have_lock; - f.m_have_lock = false; #ifdef REALM_FILELOCK_EMULATION m_pipe_fd = f.m_pipe_fd; f.m_pipe_fd = -1; @@ -1020,6 +1014,8 @@ inline File& File::operator=(File&& f) noexcept f.m_has_exclusive_lock = false; #endif #endif + m_have_lock = f.m_have_lock; + f.m_have_lock = false; m_encryption_key = std::move(f.m_encryption_key); return *this; } From e55c5dd410fd22bcc0a7118e1c337d6b7f1af4c8 Mon Sep 17 00:00:00 2001 From: Kirill Burtsev Date: Fri, 8 Sep 2023 18:47:22 +0200 Subject: [PATCH 5/5] Remove unneeded message, which may be confusing. (#6963) Further check is need what can be supported on external android storage. --- src/realm/util/file.cpp | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/realm/util/file.cpp b/src/realm/util/file.cpp index bfd517aeb5c..b60834d452b 100644 --- a/src/realm/util/file.cpp +++ b/src/realm/util/file.cpp @@ -1101,14 +1101,7 @@ static void _unlock(int m_fd) r = flock(m_fd, LOCK_UN); } while (r != 0 && errno == EINTR); if (r) { - std::string message = - "File::unlock() has failed, this is likely due to some limitation in the OS file system."; -#ifdef REALM_ANDROID - message += - " The most common cause is trying to use Realm on external storage. From Android API 30 this is no " - "longer supported. See https://developer.android.com/about/versions/11/privacy/storage for more details."; -#endif - throw SystemError(errno, message); + throw SystemError(errno, "File::unlock() has failed"); } } #endif