fix(imex): Clone provider in dc_backup_provider_wait() by flub · Pull Request #4244 · chatmail/core

flub · 2023-03-29T11:00:01Z

This is a long running process and there has been at least one crash in this function. By owning both the context and the provider when waiting we can avoid them being deallocated while we are still using them.

To make the BackupProvider clonable this transforms all the errors from it into Strings. These are clonable and how we report most our errors anyway. The Future impl of BackupProvider then turns this into an anyhow::Error so all other code can keep using anyhow as usual.

This will likely resolve the crash reported at #4179 (comment) It won't fix #4242 itself though, that's done by #4242, I think the comment reports a different bug.

This is a long running process and there has been at lease one crash in this function. By owning both the context and the provider when waiting we can avoid them being deallocated while we are still using them. To make the BackupProvider clonable this transforms all the errors from it into Strings. These are clonable and how we report most our errors anyway. The Future impl of BackupProvider then turns this into an anyhow::Error so all other code can keep using anyhow as usual.

dignifiedquire · 2023-03-29T11:14:58Z

+                res.map_err(|err| format!("{err:#}"))
            })
+            .map_err(|err| format!("{err}"))
+            .and_then(futures::future::ready)


why is this needed?

The version before this PR did this flattening of the error in the Future::poll implementation by using ? there. This is the equivalent but does it on the future itself: it flattens the task's JoinError into the error of the future the task is running. It just makes it a bit easier to use the future output.

link2xt · 2023-03-29T11:51:42Z

To make the BackupProvider clonable this transforms all the errors from it into Strings. These are clonable and how we report most our errors anyway. The Future impl of BackupProvider then turns this into an anyhow::Error so all other code can keep using anyhow as usual.

There is a related thread https://users.rust-lang.org/t/cloning-anyhow-error/73357 suggesting wrapping the error into Arc, maybe it is better than converting to String? Conversion to string likely loses some info like backtraces.

flub · 2023-03-29T12:47:57Z

To make the BackupProvider clonable this transforms all the errors from it into Strings. These are clonable and how we report most our errors anyway. The Future impl of BackupProvider then turns this into an anyhow::Error so all other code can keep using anyhow as usual.

There is a related thread https://users.rust-lang.org/t/cloning-anyhow-error/73357 suggesting wrapping the error into Arc, maybe it is better than converting to String? Conversion to string likely loses some info like backtraces.

Yeah, my first version did that (i still have this wip thing in a stash). Turns out that Arc<anyhow::Error> is quite a pain to use. E.g. you can't use .context() anymore, you can't use ? into anyhow::Result. It's much easier if your error type itself is clone and you can convert it back into anyhow::Error. You could build a much smarter error type than String that meets those two criteria and still preserves the chain and backtraces etc, but that's a lot of work.

flub · 2023-03-29T15:26:48Z

So, is this approach acceptable or should I figure out some other approach? If so, which way would you like me to explore?

iequidoo · 2023-03-29T22:49:35Z

    let ffi_provider = &mut *provider;
    let ctx = &*ffi_provider.context;
    let provider = &mut ffi_provider.provider;
+    backup_provider_wait(ctx.clone(), provider.clone());


Does it actually help much? A user can pass already unreferenced provider pointer here and it will be a use-after-free anyway. C is unsafe and a user should be careful with it, we can't do much here

So, is the reason of the crash is calling dc_backup_provider_unref() from another thread while doing dc_backup_provider_wait()?

Passing already unrefrenced pointers is not supported, see recently added "Thread safety" section on https://c.delta.chat/ #4233

But if the library using code unrefs backup provider while the function is running, it may also unref it within a short span after calling a function but before it reaches provider.clone(). We cannot prevent this on the library side, it should really be fixed in the UI.

Of course passing already unreferenced pointers is not supported, but what prevents a user from doing so? :) And clone() can actually hide bugs that will eventually appear as race conditions

We can prevent such bugs, but then we need to operate not on pointers, but on some handles that we can look up and validate

We can prevent such bugs, but then we need to operate not on pointers, but on some handles that we can look up and validate

Then we need to allocate not the structures, but pointers to them that can be nullified, sort of an RwLock<Option<Box<provider::Provider>>> that can be set to None to deallocate the provider. Or an AtomicPtr if this is going to be unsafe code anyway. But the Option/AtomicPtr itself can never be deallocated, it will have to stay nullified in its place forever in case someone decides to try using it.

This approach may work for something that is not allocated very frequently, but for dc_msg_t representing a message snapshot, which is allocated many times as you scroll the chats, wasting memory even for pointers does not sound good.

May be an option to make the library user allocate this pointer memory for us (a jlong in Java and similar in other languages), initialized to 0, and then pass a pointer to it everywhere. The core then ensures to atomically access this memory.

This is all fun, but too complicated for real use.

Of course passing already unreferenced pointers is not supported, but what prevents a user from doing so? :) And clone() can actually hide bugs that will eventually appear as race conditions

I agree that this PR is just hiding the bug in the UI code. UI code calls a blocking function and at the same time calls unref() on its argument without waiting for a function to return.

Sure, this is working around bugs on the caller-side. We already follow this pattern in most long-running FFI calls though.

It seems to happen more often that our callers will call us with a pointer that's still valid while later deallocating that pointer then they call us simply with the wrong pointer to start with. The latter we can't do anything about, and usually they find those bugs more quickly themselves :)

We do this cloning for spawn, because there the variable is still used after returning from the function. But in the case of block_on it does not make sense, this only turns a reliable crash into less reliable flaky crash.

It is better to fix the Android code here: https://github.com/deltachat/deltachat-android/blob/dfd8f92068db389529164010fbfea26027a8e674/src/org/thoughtcrime/securesms/qr/BackupProviderFragment.java#L66-L96

onDestroyView should wait until the threads spawned there a finished so there is no waitForReceiver running at the moment of dcBackupProvider.unref().

Maybe it is even enough to add some synchronized keywords here and there, to ensure that synchronized unref cannot be run at the same time as synchronized waitForReceiver.

But it is more obvious and clean to join all the threads spawned before destroying the view.

cc @r10s

flub · 2023-04-04T09:15:28Z

So I guess we're not doing this.

link2xt · 2023-04-04T09:45:59Z

I opened a bugreport in Android repository deltachat/deltachat-android#2531

flub requested review from dignifiedquire, link2xt and r10s March 29, 2023 11:00

Add changelog

bc0f650

dignifiedquire reviewed Mar 29, 2023

View reviewed changes

flub added 2 commits March 29, 2023 17:23

Merge branch 'stable-1.112' into flub/clone-provider-in-wait

19f8dc9

Fixup from merge: make BackupProvider clone again

0c3613d

iequidoo reviewed Mar 29, 2023

View reviewed changes

flub closed this Apr 4, 2023

link2xt mentioned this pull request Apr 4, 2023

BackupProviderFragment may unref backup provider while it is still used deltachat/deltachat-android#2531

Closed

Uh oh!

Uh oh!

Conversation

flub commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

link2xt commented Mar 29, 2023

Uh oh!

flub commented Mar 29, 2023

Uh oh!

flub commented Mar 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iequidoo Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

link2xt Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flub Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

link2xt Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flub commented Apr 4, 2023

Uh oh!

link2xt commented Apr 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

flub commented Mar 29, 2023 •

edited

Loading

iequidoo Mar 29, 2023 •

edited

Loading

link2xt Mar 30, 2023 •

edited

Loading

flub Mar 30, 2023 •

edited

Loading

link2xt Mar 30, 2023 •

edited

Loading