Throw an exception if File::unlock has failed + use same logic for all platforms, for detecting when we hold a lock on a file #6926

nicola-cab · 2023-08-24T14:25:38Z

What, How & Why?

Mitigate issues like this: #6912

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)
C-API, if public C++ API changed.

src/realm/util/file.cpp

kiburtse

Since we are touching this: are there any other assertions in the file which could potentially be exceptions?

src/realm/util/file.cpp

nicola-cab · 2023-08-25T10:23:56Z

Since we are touching this: are there any other assertions in the file which could potentially be exceptions?

Probably, but I would not touch them to be honest, as long as we don't have a clear API that is constantly breaking, probably it is not really worth to throw an exception. If we run in one syscall failure, it is very likely that we can't recover... all this PR does is to give a better user experience, rather than crashing brutally, the user will be informed that there is a problem.

CHANGELOG.md

src/realm/util/file.cpp

nicola-cab · 2023-08-25T11:17:33Z

@kiburtse I don't think it is safe to keep using realm after this exception. The whole point is to stop crashing all suddenly without giving any information.

kiburtse · 2023-08-25T11:33:52Z

@kiburtse I don't think it is safe to keep using realm after this exception. The whole point is to stop crashing all suddenly without giving any information.

If that's the case then i doesn't make any sense to turn it into the exception. The original issues were filled due to unawareness about android security restrictions. So really detailed error message and suggestion were missing in general unlock assertion. What's the point of throwing exception if user code can recover and try to use realm again? After this assertion InterprocessMutex and DB are going to be in a half-interrupted state. If we can't guarantee that DB object is usable after that exception and application must be restarted, then it should not be allowed, otherwise we will get other crashes and bugreports after this, which doesn't make any sense to me.

So once again, it's crashing because fundamental security policy for application is violated by user code - and it's preventable. What was missing is some helpful information for this situation, so augmented assertion message should suffice.

UP: just to clarify what was my original intention in the issue discussion... Since unlock is just a method in File, we could possible check needed operation on file path in constructor and throw pretty descriptive error earlier on File object construction rather than thinking about how to bail out half way through. Or even better: earlier still on realm initialization from the path.

nicola-cab · 2023-08-25T12:16:13Z

If that's the case then i doesn't make any sense to turn it into the exception. The original issues were filled due to unawareness about android security restrictions. So really detailed error message and suggestion were missing in general unlock assertion. What's the point of throwing exception if user code can recover and try to use realm again? After this assertion InterprocessMutex and DB are going to be in a half-interrupted state. If we can't guarantee that DB o

To clarify, on Java and Kotlin (dunno for Dart) we don't print any stack trace as far as I know, so when we crash, the user just gets a generic failure on libRealm, which is difficult to navigate, thus adding a better message in the assertion won't suffice. We need to trigger an exception. We can then debate whether trigger this only for Android or for all the platforms (IMO for all the platforms) ... also preventing a possible failure is not necessarily a good thing to do, the system call could fail for any reason. I am not in favour of trying to poke the file system before to use any system call.

finnschiermer

Code looks good to me, but the comment should die or be updated.

src/realm/util/file.cpp

finnschiermer

Sorry, spotted two other things...

src/realm/util/file.hpp

src/realm/util/file.cpp

kiburtse · 2023-08-31T21:43:08Z

src/realm/util/file.cpp

@@ -562,7 +563,8 @@ void File::close() noexcept

    if (m_fd < 0)
        return;
-    unlock();
+    if (m_have_lock)


great! So that's what was the issue all along it seems.... We just don't need to lock at all for mentioned usecases in bug reports

Yes, but bear in mind that is very likely that since the system call for unlocking the file fails on certain circumstances, the same system call will eventually fail also when locking the file. So an exception will be thrown either way.

kiburtse · 2023-08-31T22:05:33Z

src/realm/util/file.cpp

@@ -1098,7 +1100,16 @@ 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) {


So almost all good now, i'm not entirely convinced about this bit still.

We've almost misdiagnosed this issues 9th time in a row, do i understand it right? Essentially, we tried to unlock what we didn't lock (most likely from bugreports) since it wasn't needed for Realm::convert (Realm.writeCopyTo in java/kotlin sdks).

And since the assumption was that unlock is noop if file is not locked, we called it unconditionally every time and it was fine until new storage restrictions in Android hit and this syscall started to fail. So we were crashing apps for no reason for few years already.

Under #5107 and realm-java#6893 it was already mentioned that users activated needed permissions, and it was still crashing. To me this case already looks irrelevant, since we're not going to hit exact same code path now. The message may still be relevant for some realm opening scenarios though. @cmelchior could you confirm this?

Also, it seems that sqlite3 essentially ignored the result of unlock call failure (although logs it still internally) on close at least. The same seems to be true for windows where in most cases the return result is ignored, and it never asserts or propages further.

Exception in this case not much better that assertion. @finnschiermer shouldn't we just dump the error into logger, but essentially ignore it? Why interrupt any flow here?
@nicola-cab please, it's been a long issue if we account for very first bug reports, could you justify why do you think it is still needed here?

We need to throw an exception for 2 reasons.
a) it is just a much better user experience, image that you are developing your app and your app crashes without any apparent reason, what would you do? ... most probably open an issue in order to try to understand what's going on.

b) this is a limitation of some SDKs, but impacts also us, they don't easily print error messages that are coming from our ASSERTIONS. Yes, there are some ways for the developer to check the system logs, but I would not rely on that, and surely we can't grab those logs from the device ourselves.

Ignoring the error can be a viable option, but I don't think it is correct. IMO we need to merge this PR in our code.

Further check is need what can be supported on external android storage.

nicola-cab requested review from jedelbo, finnschiermer and kiburtse August 24, 2023 14:25

github-actions bot assigned nicola-cab Aug 24, 2023

cla-bot bot added the cla: yes label Aug 24, 2023

nicola-cab force-pushed the nc/throw_exception_if_unlock_fails_on_android branch 3 times, most recently from 965bc68 to 2dcd03f Compare August 24, 2023 16:21

nicola-cab changed the title ~~throw an exception only on Android if File::unlock has failed~~ Throw an exception if File::unlock has failed Aug 24, 2023

throw an exception if File::unlock has failed

36b1885

nicola-cab force-pushed the nc/throw_exception_if_unlock_fails_on_android branch from 2dcd03f to 36b1885 Compare August 24, 2023 16:23

jedelbo reviewed Aug 25, 2023

View reviewed changes

src/realm/util/file.cpp Outdated Show resolved Hide resolved

src/realm/util/file.cpp Outdated Show resolved Hide resolved

cmelchior reviewed Aug 25, 2023

View reviewed changes

src/realm/util/file.cpp Outdated Show resolved Hide resolved

kiburtse suggested changes Aug 25, 2023

View reviewed changes

src/realm/util/file.cpp Outdated Show resolved Hide resolved

src/realm/util/file.cpp Outdated Show resolved Hide resolved

code review

2174e62

nicola-cab requested review from cmelchior, jedelbo and kiburtse August 25, 2023 09:29

jedelbo approved these changes Aug 25, 2023

View reviewed changes

kiburtse suggested changes Aug 25, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

src/realm/util/file.cpp Outdated Show resolved Hide resolved

fix changelog

70d56f9

nicola-cab force-pushed the nc/throw_exception_if_unlock_fails_on_android branch from fb4ba1f to 70d56f9 Compare August 25, 2023 11:11

merge master

fb00459

nicola-cab requested a review from kiburtse August 30, 2023 12:39

merge master

7a71337

nicola-cab changed the title ~~Throw an exception if File::unlock has failed~~ Throw an exception if File::unlock has failed + use same logic for all platforms, for detecting when we hold a lock on a file Aug 31, 2023

nicola-cab force-pushed the nc/throw_exception_if_unlock_fails_on_android branch 2 times, most recently from 64136bf to 325a3e4 Compare August 31, 2023 13:16

finnschiermer suggested changes Aug 31, 2023

View reviewed changes

src/realm/util/file.cpp Outdated Show resolved Hide resolved

nicola-cab force-pushed the nc/throw_exception_if_unlock_fails_on_android branch from 325a3e4 to b4ec87c Compare August 31, 2023 14:25

nicola-cab requested a review from finnschiermer August 31, 2023 14:26

finnschiermer suggested changes Aug 31, 2023

View reviewed changes

src/realm/util/file.hpp Outdated Show resolved Hide resolved

src/realm/util/file.hpp Outdated Show resolved Hide resolved

use same logic for all platforms for locking and unlocking the file

bc7edfb

nicola-cab force-pushed the nc/throw_exception_if_unlock_fails_on_android branch from b4ec87c to bc7edfb Compare August 31, 2023 14:46

nicola-cab requested a review from finnschiermer August 31, 2023 14:46

finnschiermer approved these changes Aug 31, 2023

View reviewed changes

kiburtse suggested changes Aug 31, 2023

View reviewed changes

nicola-cab added 2 commits September 1, 2023 11:14

merge master

a75e5d2

code review

cca4eb1

kiburtse mentioned this pull request Sep 4, 2023

Android 13 Backup Realm Database #6912

Open

Remove unneeded message, which may be confusing. (#6963)

e55c5dd

Further check is need what can be supported on external android storage.

kiburtse approved these changes Sep 8, 2023

View reviewed changes

nicola-cab merged commit a4432de into master Sep 11, 2023

nicola-cab deleted the nc/throw_exception_if_unlock_fails_on_android branch September 11, 2023 09:52

kiburtse mentioned this pull request Oct 12, 2023

Failure to open a Realm file on Android 14 #7037

Open

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw an exception if File::unlock has failed + use same logic for all platforms, for detecting when we hold a lock on a file #6926

Throw an exception if File::unlock has failed + use same logic for all platforms, for detecting when we hold a lock on a file #6926

nicola-cab commented Aug 24, 2023

kiburtse left a comment

nicola-cab commented Aug 25, 2023

nicola-cab commented Aug 25, 2023

kiburtse commented Aug 25, 2023 •

edited

Loading

nicola-cab commented Aug 25, 2023 •

edited

Loading

finnschiermer left a comment

finnschiermer left a comment

kiburtse Aug 31, 2023

nicola-cab Sep 1, 2023

kiburtse Aug 31, 2023

nicola-cab Sep 1, 2023

Throw an exception if File::unlock has failed + use same logic for all platforms, for detecting when we hold a lock on a file #6926

Throw an exception if File::unlock has failed + use same logic for all platforms, for detecting when we hold a lock on a file #6926

Conversation

nicola-cab commented Aug 24, 2023

What, How & Why?

☑️ ToDos

kiburtse left a comment

Choose a reason for hiding this comment

nicola-cab commented Aug 25, 2023

nicola-cab commented Aug 25, 2023

kiburtse commented Aug 25, 2023 • edited Loading

nicola-cab commented Aug 25, 2023 • edited Loading

finnschiermer left a comment

Choose a reason for hiding this comment

finnschiermer left a comment

Choose a reason for hiding this comment

kiburtse Aug 31, 2023

Choose a reason for hiding this comment

nicola-cab Sep 1, 2023

Choose a reason for hiding this comment

kiburtse Aug 31, 2023

Choose a reason for hiding this comment

nicola-cab Sep 1, 2023

Choose a reason for hiding this comment

kiburtse commented Aug 25, 2023 •

edited

Loading

nicola-cab commented Aug 25, 2023 •

edited

Loading