Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sync: Avoid crash on invalid state events #3124

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions crates/matrix-sdk-base/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ pub use http;
pub use matrix_sdk_crypto as crypto;
pub use once_cell;
pub use rooms::{
DisplayName, Room, RoomCreateWithCreatorEventContent, RoomInfo, RoomInfoUpdate, RoomMember,
RoomMemberships, RoomState, RoomStateFilter,
DisplayName, RawRoomInfo, Room, RoomCreateWithCreatorEventContent, RoomInfo, RoomInfoUpdate,
RoomMember, RoomMemberships, RoomState, RoomStateFilter,
};
pub use store::{StateChanges, StateStore, StateStoreDataKey, StateStoreDataValue, StoreError};
pub use utils::{
Expand Down
163 changes: 158 additions & 5 deletions crates/matrix-sdk-base/src/rooms/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,12 @@ use std::{

use bitflags::bitflags;
pub use members::RoomMember;
pub use normal::{Room, RoomInfo, RoomInfoUpdate, RoomState, RoomStateFilter};
pub use normal::{RawRoomInfo, Room, RoomInfo, RoomInfoUpdate, RoomState, RoomStateFilter};
use ruma::{
assign,
canonical_json::redact_in_place,
events::{
call::member::CallMemberEventContent,
call::member::{CallMemberEvent, CallMemberEventContent},
macros::EventContent,
room::{
avatar::RoomAvatarEventContent,
Expand All @@ -32,9 +33,10 @@ use ruma::{
},
tag::{TagName, Tags},
AnyStrippedStateEvent, AnySyncStateEvent, EmptyStateKey, RedactContent,
RedactedStateEventContent, StaticStateEventContent, SyncStateEvent,
RedactedStateEventContent, StateEventType, StaticStateEventContent, SyncStateEvent,
},
room::RoomType,
serde::Raw,
EventId, OwnedUserId, RoomVersionId,
};
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -116,12 +118,163 @@ pub struct BaseRoomInfo {
pub(crate) notable_tags: RoomNotableTags,
}

/// This contains the same fields as BaseRoomInfo, but events are in the raw, unparsed form.
#[derive(Clone, Debug, Default, Serialize, Deserialize)]
pub struct RawBaseRoomInfo {
/// The avatar URL of this room.
pub avatar: Option<Raw<AnySyncStateEvent>>,
/// The canonical alias of this room.
pub canonical_alias: Option<Raw<AnySyncStateEvent>>,
/// The `m.room.create` event content of this room.
pub create: Option<Raw<AnySyncStateEvent>>,
/// A list of user ids this room is considered as direct message, if this
/// room is a DM.
pub dm_targets: HashSet<OwnedUserId>,
/// The `m.room.encryption` event content that enabled E2EE in this room.
pub encryption: Option<Raw<AnySyncStateEvent>>,
/// The guest access policy of this room.
pub guest_access: Option<Raw<AnySyncStateEvent>>,
/// The history visibility policy of this room.
pub history_visibility: Option<Raw<AnySyncStateEvent>>,
/// The join rule policy of this room.
pub join_rules: Option<Raw<AnySyncStateEvent>>,
/// The maximal power level that can be found in this room.
pub max_power_level: i64,
/// The `m.room.name` of this room.
pub name: Option<Raw<AnySyncStateEvent>>,
/// The `m.room.tombstone` event content of this room.
pub tombstone: Option<Raw<AnySyncStateEvent>>,
/// The topic of this room.
pub topic: Option<Raw<AnySyncStateEvent>>,
/// All minimal state events that containing one or more running matrixRTC
/// memberships.
#[serde(skip_serializing_if = "BTreeMap::is_empty", default)]
pub rtc_member: BTreeMap<OwnedUserId, Raw<AnySyncStateEvent>>,
/// Whether this room has been manually marked as unread.
#[serde(default)]
pub is_marked_unread: bool,
/// Some notable tags.
///
/// We are not interested by all the tags. Some tags are more important than
/// others, and this field collects them.
#[serde(skip_serializing_if = "RoomNotableTags::is_empty", default)]
pub notable_tags: RoomNotableTags,
}

impl RawBaseRoomInfo {
pub fn map_events(
&mut self,
f: &dyn Fn(&mut Option<Raw<AnySyncStateEvent>>, StateEventType, &str),
f_rtc_member: &dyn Fn(&mut BTreeMap<OwnedUserId, Raw<AnySyncStateEvent>>),
) {
Comment on lines +165 to +169
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a code comment. Are f and f_rtc_member called both for rtc member events? What's the str passed as the third argument to f? (also, is it useful, since it's always the empty static string?) Why are the signatures of both functions so different?

Also, should the function's signature be different, so that it returns a boolean indicating whether it did find the event? If I understand correctly, there's at most one event that we need to redact, so there's no need to iterate over all the events once we've found the one we wanted to redact? Maybe the best API would be use to the ControlFlow data structure from the stdlib.

f(&mut self.avatar, StateEventType::RoomAvatar, "");
f(&mut self.canonical_alias, StateEventType::RoomCanonicalAlias, "");
f(&mut self.create, StateEventType::RoomCreate, "");
f(&mut self.encryption, StateEventType::RoomEncryption, "");
f(&mut self.guest_access, StateEventType::RoomGuestAccess, "");
f(&mut self.history_visibility, StateEventType::RoomHistoryVisibility, "");
f(&mut self.join_rules, StateEventType::RoomJoinRules, "");
f(&mut self.name, StateEventType::RoomName, "");
f(&mut self.tombstone, StateEventType::RoomTombstone, "");
f(&mut self.topic, StateEventType::RoomTopic, "");
f_rtc_member(&mut self.rtc_member);
}

pub fn handle_redaction(&mut self, event_id: &EventId, room_version: &RoomVersionId) {
self.map_events(
// Clean up normal events
&|event, _event_type, _state_key| {
if event
.as_ref()
.and_then(|a| a.deserialize().ok())
.is_some_and(|e| e.event_id() == event_id)
{
if let Ok(mut object) = event.as_ref().unwrap().deserialize_as() {
Comment on lines +187 to +192
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto, the event is deserialized twice here: can we deserialize it once with deserialize_as and then check the event_id matches later, instead?

if redact_in_place(&mut object, room_version, None).is_ok() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use a match and log the error message instead?

*event = Some(Raw::new(&object).unwrap().cast());
}
}
}
},
// Clean up rtc_members
&|map| {
Comment on lines +198 to +200
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: please let the code breath 😁

Suggested change
},
// Clean up rtc_members
&|map| {
},
// Clean up rtc_members
&|map| {

for event in map.values_mut() {
if event.deserialize().is_ok_and(|e| e.event_id() == event_id) {
if let Ok(mut object) = event.deserialize_as() {
Comment on lines +202 to +203
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we avoid deserializing twice here? Maybe call deserialize_as and check against the target event_id instead?

if redact_in_place(&mut object, room_version, None).is_ok() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto here

*event = Raw::new(&object).unwrap().cast();
}
}
}
}
},
);
}
pub fn extend_with_changes(
Comment on lines +212 to +213
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
pub fn extend_with_changes(
}
pub fn extend_with_changes(

Comment on lines +212 to +213
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also badly needs a comment. I have no ideas what it's doing, just from looking at the name, signature, or body...

&mut self,
changes: &BTreeMap<StateEventType, BTreeMap<String, Raw<AnySyncStateEvent>>>,
) {
self.map_events(
&|event, event_type, state_key| {
if let Some(e) = changes.get(&event_type).and_then(|c| c.get(state_key)) {
*event = Some(e.clone());
}
},
&|map| {
if let Some(rtc_changes) = changes.get(&StateEventType::CallMember) {
map.extend(
rtc_changes
.clone()
.into_iter()
.filter_map(|(u, r)| OwnedUserId::try_from(u).ok().map(|u| (u, r))),
);
// Remove all events that don't contain any memberships anymore.
map.retain(|_, ev| {
ev.deserialize_as::<CallMemberEvent>().is_ok_and(|c| {
c.as_original()
.is_some_and(|o| !o.content.active_memberships(None).is_empty())
})
});
}
},
);
}
}

impl BaseRoomInfo {
/// Create a new, empty base room info.
pub fn new() -> Self {
Self::default()
}

/// Converts BaseRoomInfo into RawBaseRoomInfo, potentially losing information about some events.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: in doc comments, prefer the [`RawBaseRoomInfo`] notation, so we get nice internal links and all 😍

pub fn into_raw_lossy(self) -> RawBaseRoomInfo {
RawBaseRoomInfo {
avatar: self.avatar.and_then(|e| Raw::new(&e).ok()).map(|r| r.cast()),
canonical_alias: self.canonical_alias.and_then(|e| Raw::new(&e).ok()).map(|r| r.cast()),
create: self.create.and_then(|e| Raw::new(&e).ok()).map(|r| r.cast()),
dm_targets: self.dm_targets,
encryption: self.encryption.and_then(|e| Raw::new(&e).ok()).map(|r| r.cast()),
guest_access: self.guest_access.and_then(|e| Raw::new(&e).ok()).map(|r| r.cast()),
history_visibility: self
.history_visibility
.and_then(|e| Raw::new(&e).ok())
.map(|r| r.cast()),
join_rules: self.join_rules.and_then(|e| Raw::new(&e).ok()).map(|r| r.cast()),
max_power_level: self.max_power_level,
name: self.name.and_then(|e| Raw::new(&e).ok()).map(|r| r.cast()),
tombstone: self.tombstone.and_then(|e| Raw::new(&e).ok()).map(|r| r.cast()),
topic: self.topic.and_then(|e| Raw::new(&e).ok()).map(|r| r.cast()),
rtc_member: self
.rtc_member
.into_iter()
.filter_map(|(u, e)| Raw::new(&e).ok().map(|r| (u, r.cast())))
.collect(),
is_marked_unread: self.is_marked_unread,
notable_tags: self.notable_tags,
}
}

pub(crate) fn calculate_room_name(
&self,
joined_member_count: u64,
Expand Down Expand Up @@ -190,7 +343,7 @@ impl BaseRoomInfo {
return false;
};

// we modify the event so that `origin_sever_ts` gets copied into
// we modify the event so that `origin_server_ts` gets copied into
// `content.created_ts`
let mut o_ev = o_ev.clone();
o_ev.content.set_created_ts_if_none(o_ev.origin_server_ts);
Expand Down Expand Up @@ -318,7 +471,7 @@ bitflags! {
/// others, and this struct describes them.
#[repr(transparent)]
#[derive(Debug, Default, Clone, Copy, Deserialize, Serialize)]
pub(crate) struct RoomNotableTags: u8 {
pub struct RoomNotableTags: u8 {
/// The `m.favourite` tag.
const FAVOURITE = 0b0000_0001;

Expand Down
Loading
Loading