From 0ca29bbeed3975865809701d76bd3fa50aa1bcfd Mon Sep 17 00:00:00 2001 From: link2xt Date: Sat, 6 Apr 2024 20:17:06 +0000 Subject: [PATCH] fix(deltachat-jsonrpc): block in `inner_get_backup_qr` This change avoids the race between `provide_backup` changing the state from NoProvider to Pending and a call to `get_backup_qr` or `get_backup_qr_svg`. With this change `get_backup_qr` and `get_backup_qr_svg` always block until QR code is available, even if `provide_backup` was not called yet. --- deltachat-jsonrpc/src/api.rs | 84 +++++++++++++++++------------------- 1 file changed, 39 insertions(+), 45 deletions(-) diff --git a/deltachat-jsonrpc/src/api.rs b/deltachat-jsonrpc/src/api.rs index 859c62715c..74cf7631ec 100644 --- a/deltachat-jsonrpc/src/api.rs +++ b/deltachat-jsonrpc/src/api.rs @@ -1,5 +1,6 @@ use std::collections::BTreeMap; use std::sync::Arc; +use std::time::Duration; use std::{collections::HashMap, str::FromStr}; use anyhow::{anyhow, bail, ensure, Context, Result}; @@ -62,14 +63,14 @@ use crate::api::types::qr::QrObject; struct AccountState { /// The Qr code for current [`CommandApi::provide_backup`] call. /// - /// If there currently is a call to [`CommandApi::provide_backup`] this will be - /// `Pending` or `Ready`, otherwise `NoProvider`. - backup_provider_qr: watch::Sender, + /// If there is currently is a call to [`CommandApi::provide_backup`] this will be + /// `Some`, otherwise `None`. + backup_provider_qr: watch::Sender>, } impl Default for AccountState { fn default() -> Self { - let (tx, _rx) = watch::channel(ProviderQr::NoProvider); + let tx = watch::Sender::new(None); Self { backup_provider_qr: tx, } @@ -123,21 +124,13 @@ impl CommandApi { .with_state(account_id, |state| state.backup_provider_qr.subscribe()) .await; - let val: ProviderQr = receiver.borrow_and_update().clone(); - match val { - ProviderQr::NoProvider => bail!("No backup being provided"), - ProviderQr::Pending => loop { - if receiver.changed().await.is_err() { - bail!("No backup being provided (account state dropped)"); - } - let val: ProviderQr = receiver.borrow().clone(); - match val { - ProviderQr::NoProvider => bail!("No backup being provided"), - ProviderQr::Pending => continue, - ProviderQr::Ready(qr) => break Ok(qr), - }; - }, - ProviderQr::Ready(qr) => Ok(qr), + loop { + if let Some(qr) = receiver.borrow_and_update().clone() { + return Ok(qr); + } + if receiver.changed().await.is_err() { + bail!("No backup being provided (account state dropped)"); + } } } } @@ -1569,20 +1562,21 @@ impl CommandApi { /// Returns once a remote device has retrieved the backup, or is cancelled. async fn provide_backup(&self, account_id: u32) -> Result<()> { let ctx = self.get_context(account_id).await?; + + let provider = imex::BackupProvider::prepare(&ctx).await?; self.with_state(account_id, |state| { - state.backup_provider_qr.send_replace(ProviderQr::Pending); + state.backup_provider_qr.send_replace(Some(provider.qr())); }) .await; - let provider = imex::BackupProvider::prepare(&ctx).await?; + let res = provider.await; + self.with_state(account_id, |state| { - state - .backup_provider_qr - .send_replace(ProviderQr::Ready(provider.qr())); + state.backup_provider_qr.send_replace(None); }) .await; - provider.await + res } /// Returns the text of the QR code for the running [`CommandApi::provide_backup`]. @@ -1590,11 +1584,17 @@ impl CommandApi { /// This QR code text can be used in [`CommandApi::get_backup`] on a second device to /// retrieve the backup and setup this second device. /// - /// This call will fail if there is currently no concurrent call to - /// [`CommandApi::provide_backup`]. This call may block if the QR code is not yet - /// ready. + /// This call will block until the QR code is ready, + /// even if there is no concurrent call to [`CommandApi::provide_backup`], + /// but will fail after 10 seconds to avoid deadlocks. async fn get_backup_qr(&self, account_id: u32) -> Result { - let qr = self.inner_get_backup_qr(account_id).await?; + let qr = tokio::time::timeout( + Duration::from_secs(10), + self.inner_get_backup_qr(account_id), + ) + .await + .context("Backup provider did not start in time")? + .context("Failed to get backup QR code")?; qr::format_backup(&qr) } @@ -1603,14 +1603,20 @@ impl CommandApi { /// This QR code can be used in [`CommandApi::get_backup`] on a second device to /// retrieve the backup and setup this second device. /// - /// This call will fail if there is currently no concurrent call to - /// [`CommandApi::provide_backup`]. This call may block if the QR code is not yet - /// ready. + /// This call will block until the QR code is ready, + /// even if there is no concurrent call to [`CommandApi::provide_backup`], + /// but will fail after 10 seconds to avoid deadlocks. /// /// Returns the QR code rendered as an SVG image. async fn get_backup_qr_svg(&self, account_id: u32) -> Result { let ctx = self.get_context(account_id).await?; - let qr = self.inner_get_backup_qr(account_id).await?; + let qr = tokio::time::timeout( + Duration::from_secs(10), + self.inner_get_backup_qr(account_id), + ) + .await + .context("Backup provider did not start in time")? + .context("Failed to get backup QR code")?; generate_backup_qr(&ctx, &qr).await } @@ -2141,15 +2147,3 @@ async fn get_config( .await } } - -/// Whether a QR code for a BackupProvider is currently available. -#[allow(clippy::large_enum_variant)] -#[derive(Clone, Debug)] -enum ProviderQr { - /// There is no provider, asking for a QR is an error. - NoProvider, - /// There is a provider, the QR code is pending. - Pending, - /// There is a provider and QR code. - Ready(Qr), -}