From 3a4ae42cc247143cbf89e10e48017dd0b6812eb3 Mon Sep 17 00:00:00 2001 From: link2xt Date: Fri, 10 Oct 2025 21:58:16 +0000 Subject: [PATCH 1/2] fix: do not fail to receive call accepted/ended messages referring to non-call Message-ID In-Reply-To may refer to non-call message as we do not control the sender. It may also happen that call message was received by older version and processed as text, in which case correct In-Reply-To appears to be referring to the text message. --- deltachat-jsonrpc/src/api/types/calls.rs | 6 +- src/calls.rs | 61 ++++++++++++---- src/calls/calls_tests.rs | 91 +++++++++++++++++++++--- 3 files changed, 136 insertions(+), 22 deletions(-) diff --git a/deltachat-jsonrpc/src/api/types/calls.rs b/deltachat-jsonrpc/src/api/types/calls.rs index 0c555dc2ef..e779f1c89e 100644 --- a/deltachat-jsonrpc/src/api/types/calls.rs +++ b/deltachat-jsonrpc/src/api/types/calls.rs @@ -1,4 +1,4 @@ -use anyhow::Result; +use anyhow::{Context as _, Result}; use deltachat::calls::{call_state, sdp_has_video, CallState}; use deltachat::context::Context; @@ -26,7 +26,9 @@ pub struct JsonrpcCallInfo { impl JsonrpcCallInfo { pub async fn from_msg_id(context: &Context, msg_id: MsgId) -> Result { - let call_info = context.load_call_by_id(msg_id).await?; + let call_info = context.load_call_by_id(msg_id).await?.with_context(|| { + format!("Attempting to get call state of non-call message {msg_id}") + })?; let sdp_offer = call_info.place_call_info.clone(); let has_video = sdp_has_video(&sdp_offer).unwrap_or_default(); let state = JsonrpcCallState::from_msg_id(context, msg_id).await?; diff --git a/src/calls.rs b/src/calls.rs index c988ad5b2f..2daf0e8eca 100644 --- a/src/calls.rs +++ b/src/calls.rs @@ -213,7 +213,9 @@ impl Context { call_id: MsgId, accept_call_info: String, ) -> Result<()> { - let mut call: CallInfo = self.load_call_by_id(call_id).await?; + let mut call: CallInfo = self.load_call_by_id(call_id).await?.with_context(|| { + format!("accept_incoming_call is called with {call_id} which does not refer to a call") + })?; ensure!(call.is_incoming()); if call.is_accepted() || call.is_ended() { info!(self, "Call already accepted/ended"); @@ -248,7 +250,9 @@ impl Context { /// Cancel, decline or hangup an incoming or outgoing call. pub async fn end_call(&self, call_id: MsgId) -> Result<()> { - let mut call: CallInfo = self.load_call_by_id(call_id).await?; + let mut call: CallInfo = self.load_call_by_id(call_id).await?.with_context(|| { + format!("end_call is called with {call_id} which does not refer to a call") + })?; if call.is_ended() { info!(self, "Call already ended"); return Ok(()); @@ -291,7 +295,13 @@ impl Context { call_id: MsgId, ) -> Result<()> { sleep(Duration::from_secs(wait)).await; - let mut call = context.load_call_by_id(call_id).await?; + let Some(mut call) = context.load_call_by_id(call_id).await? else { + warn!( + context, + "emit_end_call_if_unaccepted is called with {call_id} which does not refer to a call." + ); + return Ok(()); + }; if !call.is_accepted() && !call.is_ended() { if call.is_incoming() { call.mark_as_canceled(&context).await?; @@ -316,7 +326,10 @@ impl Context { from_id: ContactId, ) -> Result<()> { if mime_message.is_call() { - let call = self.load_call_by_id(call_id).await?; + let Some(call) = self.load_call_by_id(call_id).await? else { + warn!(self, "{call_id} does not refer to a call message"); + return Ok(()); + }; if call.is_incoming() { if call.is_stale() { @@ -352,7 +365,11 @@ impl Context { } else { match mime_message.is_system_message { SystemMessage::CallAccepted => { - let mut call = self.load_call_by_id(call_id).await?; + let Some(mut call) = self.load_call_by_id(call_id).await? else { + warn!(self, "{call_id} does not refer to a call message"); + return Ok(()); + }; + if call.is_ended() || call.is_accepted() { info!(self, "CallAccepted received for accepted/ended call"); return Ok(()); @@ -377,7 +394,11 @@ impl Context { } } SystemMessage::CallEnded => { - let mut call = self.load_call_by_id(call_id).await?; + let Some(mut call) = self.load_call_by_id(call_id).await? else { + warn!(self, "{call_id} does not refer to a call message"); + return Ok(()); + }; + if call.is_ended() { // may happen eg. if a a message is missed info!(self, "CallEnded received for ended call"); @@ -421,15 +442,26 @@ impl Context { } /// Loads information about the call given its ID. - pub async fn load_call_by_id(&self, call_id: MsgId) -> Result { + /// + /// If the message referred to by ID is + /// not a call message, returns `None`. + pub async fn load_call_by_id(&self, call_id: MsgId) -> Result> { let call = Message::load_from_db(self, call_id).await?; - self.load_call_by_message(call) + Ok(self.load_call_by_message(call)) } - fn load_call_by_message(&self, call: Message) -> Result { - ensure!(call.viewtype == Viewtype::Call); + // Loads information about the call given the `Message`. + // + // If the `Message` is not a call message, returns `None` + fn load_call_by_message(&self, call: Message) -> Option { + if call.viewtype != Viewtype::Call { + // This can happen e.g. if a "call accepted" + // or "call ended" message is received + // with `In-Reply-To` referring to non-call message. + return None; + } - Ok(CallInfo { + Some(CallInfo { place_call_info: call .param .get(Param::WebrtcRoom) @@ -497,8 +529,13 @@ pub enum CallState { } /// Returns call state given the message ID. +/// +/// Returns an error if the message is not a call message. pub async fn call_state(context: &Context, msg_id: MsgId) -> Result { - let call = context.load_call_by_id(msg_id).await?; + let call = context + .load_call_by_id(msg_id) + .await? + .with_context(|| format!("{msg_id} is not a call message"))?; let state = if call.is_incoming() { if call.is_accepted() { if call.is_ended() { diff --git a/src/calls/calls_tests.rs b/src/calls/calls_tests.rs index bbd382734d..34fe7e7200 100644 --- a/src/calls/calls_tests.rs +++ b/src/calls/calls_tests.rs @@ -1,6 +1,8 @@ use super::*; use crate::chat::forward_msgs; use crate::config::Config; +use crate::constants::DC_CHAT_ID_TRASH; +use crate::receive_imf::receive_imf; use crate::test_utils::{TestContext, TestContextManager}; struct CallSetup { @@ -53,7 +55,10 @@ async fn setup_call() -> Result { for (t, m) in [(&alice, &alice_call), (&alice2, &alice2_call)] { assert!(!m.is_info()); assert_eq!(m.viewtype, Viewtype::Call); - let info = t.load_call_by_id(m.id).await?; + let info = t + .load_call_by_id(m.id) + .await? + .expect("m should be a call message"); assert!(!info.is_incoming()); assert!(!info.is_accepted()); assert_eq!(info.place_call_info, PLACE_INFO); @@ -71,7 +76,10 @@ async fn setup_call() -> Result { t.evtracker .get_matching(|evt| matches!(evt, EventType::IncomingCall { .. })) .await; - let info = t.load_call_by_id(m.id).await?; + let info = t + .load_call_by_id(m.id) + .await? + .expect("IncomingCall event should refer to a call message"); assert!(info.is_incoming()); assert!(!info.is_accepted()); assert_eq!(info.place_call_info, PLACE_INFO); @@ -111,7 +119,10 @@ async fn accept_call() -> Result { .get_matching(|evt| matches!(evt, EventType::IncomingCallAccepted { .. })) .await; let sent2 = bob.pop_sent_msg().await; - let info = bob.load_call_by_id(bob_call.id).await?; + let info = bob + .load_call_by_id(bob_call.id) + .await? + .expect("bob_call should be a call message"); assert!(info.is_accepted()); assert_eq!(info.place_call_info, PLACE_INFO); assert_eq!(call_state(&bob, bob_call.id).await?, CallState::Active); @@ -121,7 +132,10 @@ async fn accept_call() -> Result { bob2.evtracker .get_matching(|evt| matches!(evt, EventType::IncomingCallAccepted { .. })) .await; - let info = bob2.load_call_by_id(bob2_call.id).await?; + let info = bob2 + .load_call_by_id(bob2_call.id) + .await? + .expect("bob2_call should be a call message"); assert!(info.is_accepted()); assert_eq!(call_state(&bob2, bob2_call.id).await?, CallState::Active); @@ -140,7 +154,10 @@ async fn accept_call() -> Result { accept_call_info: ACCEPT_INFO.to_string() } ); - let info = alice.load_call_by_id(alice_call.id).await?; + let info = alice + .load_call_by_id(alice_call.id) + .await? + .expect("alice_call should be a call message"); assert!(info.is_accepted()); assert_eq!(info.place_call_info, PLACE_INFO); assert_eq!(call_state(&alice, alice_call.id).await?, CallState::Active); @@ -460,14 +477,20 @@ async fn test_mark_calls() -> Result<()> { alice, alice_call, .. } = setup_call().await?; - let mut call_info: CallInfo = alice.load_call_by_id(alice_call.id).await?; + let mut call_info: CallInfo = alice + .load_call_by_id(alice_call.id) + .await? + .expect("alice_call should be a call message"); assert!(!call_info.is_accepted()); assert!(!call_info.is_ended()); call_info.mark_as_accepted(&alice).await?; assert!(call_info.is_accepted()); assert!(!call_info.is_ended()); - let mut call_info: CallInfo = alice.load_call_by_id(alice_call.id).await?; + let mut call_info: CallInfo = alice + .load_call_by_id(alice_call.id) + .await? + .expect("alice_call should be a call message"); assert!(call_info.is_accepted()); assert!(!call_info.is_ended()); @@ -484,7 +507,10 @@ async fn test_update_call_text() -> Result<()> { alice, alice_call, .. } = setup_call().await?; - let call_info = alice.load_call_by_id(alice_call.id).await?; + let call_info = alice + .load_call_by_id(alice_call.id) + .await? + .expect("alice_call should be a call message"); call_info.update_text(&alice, "foo bar").await?; let alice_call = Message::load_from_db(&alice, alice_call.id).await?; @@ -529,3 +555,52 @@ async fn test_forward_call() -> Result<()> { Ok(()) } + +/// Tests that "end call" message referring +/// to a text message does not make receive_imf fail. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_end_text_call() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + + let received1 = receive_imf( + alice, + b"From: bob@example.net\n\ + To: alice@example.org\n\ + Message-ID: \n\ + Date: Sun, 22 Mar 2020 22:37:57 +0000\n\ + Chat-Version: 1.0\n\ + \n\ + Hello\n", + false, + ) + .await? + .unwrap(); + assert_eq!(received1.msg_ids.len(), 1); + let msg = Message::load_from_db(alice, received1.msg_ids[0]) + .await + .unwrap(); + assert_eq!(msg.viewtype, Viewtype::Text); + + // Receiving "Call ended" message that refers + // to the text message does not result in an error. + let received2 = receive_imf( + alice, + b"From: bob@example.net\n\ + To: alice@example.org\n\ + Message-ID: \n\ + Date: Sun, 22 Mar 2020 23:37:57 +0000\n\ + In-Reply-To: \n\ + Chat-Version: 1.0\n\ + Chat-Content: call-ended\n\ + \n\ + Call ended\n", + false, + ) + .await? + .unwrap(); + assert_eq!(received2.msg_ids.len(), 1); + assert_eq!(received2.chat_id, DC_CHAT_ID_TRASH); + + Ok(()) +} From aa8a242a5467552661c8a58800fafe7740b637cd Mon Sep 17 00:00:00 2001 From: link2xt Date: Fri, 10 Oct 2025 22:50:18 +0000 Subject: [PATCH 2/2] fix: do not try to process calls from partial messages Any control information from the message should only be downloaded when the message is fully downloaded to avoid processing it twice. Besides, "partial" messages may actually be full messages with an error that are only processed as partial to add a message bubble allowing to download the message later. --- src/calls/calls_tests.rs | 64 +++++++++++++++++++++++++++++++++++++++- src/receive_imf.rs | 12 ++++---- 2 files changed, 70 insertions(+), 6 deletions(-) diff --git a/src/calls/calls_tests.rs b/src/calls/calls_tests.rs index 34fe7e7200..9ad40efeb1 100644 --- a/src/calls/calls_tests.rs +++ b/src/calls/calls_tests.rs @@ -2,7 +2,7 @@ use super::*; use crate::chat::forward_msgs; use crate::config::Config; use crate::constants::DC_CHAT_ID_TRASH; -use crate::receive_imf::receive_imf; +use crate::receive_imf::{receive_imf, receive_imf_from_inbox}; use crate::test_utils::{TestContext, TestContextManager}; struct CallSetup { @@ -604,3 +604,65 @@ async fn test_end_text_call() -> Result<()> { Ok(()) } + +/// Tests that partially downloaded "call ended" +/// messages are not processed. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_no_partial_calls() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + + let seen = false; + + // The messages in the test + // have no `Date` on purpose, + // so they are treated as new. + let received_call = receive_imf( + alice, + b"From: bob@example.net\n\ + To: alice@example.org\n\ + Message-ID: \n\ + Chat-Version: 1.0\n\ + Chat-Content: call\n\ + Chat-Webrtc-Room: YWFhYWFhYWFhCg==\n\ + \n\ + Hello, this is a call\n", + seen, + ) + .await? + .unwrap(); + assert_eq!(received_call.msg_ids.len(), 1); + let call_msg = Message::load_from_db(alice, received_call.msg_ids[0]) + .await + .unwrap(); + assert_eq!(call_msg.viewtype, Viewtype::Call); + assert_eq!(call_state(alice, call_msg.id).await?, CallState::Alerting); + + let imf_raw = b"From: bob@example.net\n\ + To: alice@example.org\n\ + Message-ID: \n\ + In-Reply-To: \n\ + Chat-Version: 1.0\n\ + Chat-Content: call-ended\n\ + \n\ + Call ended\n"; + receive_imf_from_inbox( + alice, + "second@example.net", + imf_raw, + seen, + Some(imf_raw.len().try_into().unwrap()), + ) + .await?; + + // The call is still not ended. + assert_eq!(call_state(alice, call_msg.id).await?, CallState::Alerting); + + // Fully downloading the message ends the call. + receive_imf_from_inbox(alice, "second@example.net", imf_raw, seen, None) + .await + .context("Failed to fully download end call message")?; + assert_eq!(call_state(alice, call_msg.id).await?, CallState::Missed); + + Ok(()) +} diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 51de43da92..47903206c1 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -999,7 +999,7 @@ pub(crate) async fn receive_imf_inner( } } - if mime_parser.is_call() { + if is_partial_download.is_none() && mime_parser.is_call() { context .handle_call_msg(insert_msg_id, &mime_parser, from_id) .await?; @@ -1157,8 +1157,9 @@ async fn decide_chat_assignment( { info!(context, "Chat edit/delete/iroh/sync message (TRASH)."); true - } else if mime_parser.is_system_message == SystemMessage::CallAccepted - || mime_parser.is_system_message == SystemMessage::CallEnded + } else if is_partial_download.is_none() + && (mime_parser.is_system_message == SystemMessage::CallAccepted + || mime_parser.is_system_message == SystemMessage::CallEnded) { info!(context, "Call state changed (TRASH)."); true @@ -1986,8 +1987,9 @@ async fn add_parts( handle_edit_delete(context, mime_parser, from_id).await?; - if mime_parser.is_system_message == SystemMessage::CallAccepted - || mime_parser.is_system_message == SystemMessage::CallEnded + if is_partial_download.is_none() + && (mime_parser.is_system_message == SystemMessage::CallAccepted + || mime_parser.is_system_message == SystemMessage::CallEnded) { if let Some(field) = mime_parser.get_header(HeaderDef::InReplyTo) { if let Some(call) =