sync: Avoid crash on invalid state events #3124

timokoesters · 2024-02-13T15:24:51Z

The problem was in matrix_sdk_sqlite::state_store::SqliteStateStore save_changes, which saved the RoomInfo to the disk by serializing it.

Public API changes documented in changelogs (optional)

Fixes #2949

timokoesters · 2024-02-13T15:25:39Z

The bad events will now be saved as None, which falls back to the default for the event type. For example JoinRule::_Custom("test") falls back to JoinRule::Public. This may or may not be the correct behavior...

Here is the code where it falls back to the default:

matrix-rust-sdk/crates/matrix-sdk-base/src/rooms/normal.rs

Lines 1141 to 1146 in 7493176

    
           fn join_rule(&self) -> &JoinRule { 
        
               match &self.base_info.join_rules { 
        
                   Some(MinimalStateEvent::Original(ev)) => &ev.content.join_rule, 
        
                   _ => &JoinRule::Public, 
        
               } 
        
           }

codecov · 2024-02-13T15:35:52Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (060eaef) 83.85% compared to head (39ea904) 83.83%.
Report is 33 commits behind head on main.

❗ Current head 39ea904 differs from pull request most recent head 6701933. Consider uploading reports for the commit 6701933 to get more accurate results

Files	Patch %	Lines
crates/matrix-sdk-base/src/rooms/mod.rs	55.55%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3124      +/-   ##
==========================================
- Coverage   83.85%   83.83%   -0.03%     
==========================================
  Files         229      229              
  Lines       23706    23718      +12     
==========================================
+ Hits        19879    19884       +5     
- Misses       3827     3834       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bnjbvr

Thanks for writing a fix. The approach isn't great, in the sense that the lib user might have been able to do something with the custom event, and we're losing it here.

Instead, could we try to serialize the Raw<Event>s in the BaseRoomInfo, have getters that automatically deserialize them, please? It shouldn't require a migration, in theory (because a serialized Raw<T> is the same as the serialize T, when the serialization mechanism is JSON). If it does require a migration, that would result in a logout in the EX apps, so it would be an easy way to try it out and make sure no migration is needed.

timokoesters · 2024-02-20T12:16:29Z

crates/matrix-sdk-base/src/rooms/normal.rs

@@ -1778,6 +1823,65 @@ mod tests {
        );
    }

+    #[async_test]
+    async fn test_save_bad_joinrules() {


This test is currently not testing what it's supposed to, because it runs in-memory instead of on sqlite or indexeddb. How can I change that?

make_room below likely creates and configures the Client; you'd need another Client that is configured to use sqlite or indexeddb.

Did you address this comment? ^

timokoesters · 2024-02-20T12:20:32Z

crates/matrix-sdk-indexeddb/src/state_store/mod.rs


            for (room_id, redactions) in &changes.redactions {
+                // TODO: Load room version lazily, only if it is necessary


I removed the lazy loading to make it simpler while implementing, but I should add it back in. The approach from the sqlite make_room_version closure doesn't work here because it's async

With the last set of changes, I had to make the sqlite version non-async too, to allow passing the room version into the base_info.handle_redaction function.

timokoesters · 2024-02-20T12:25:57Z

This PR is ready for another review. I've changed the fundamental approach, so now the raw events are stored in the database. I did a quick test using element x android (sqlite), but I could not test indexeddb yet and I haven't tested redactions yet.

The problem was in matrix_sdk_sqlite::state_store::SqliteStateStore save_changes, which saved the RoomInfo to the disk by serializing it. Now the events are serialized as Raws using a separate struct.

bnjbvr · 2024-02-20T14:30:49Z

crates/matrix-sdk-base/src/rooms/normal.rs

@@ -1778,6 +1823,65 @@ mod tests {
        );
    }

+    #[async_test]
+    async fn test_save_bad_joinrules() {


make_room below likely creates and configures the Client; you'd need another Client that is configured to use sqlite or indexeddb.

bnjbvr · 2024-02-20T14:55:54Z

crates/matrix-sdk-base/src/rooms/mod.rs

@@ -77,33 +78,44 @@ impl fmt::Display for DisplayName {
 #[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct BaseRoomInfo {
    /// The avatar URL of this room.
+    #[serde(skip_serializing)]


Instead of all those annotations, can we use something like #[serde(into = "RawRoomInfo", from = "RawRoomInfo")], so that the (de)serialization of this struct goes through that of RawRoomInfo instead, and we don't have to worry about redacting events?

bnjbvr · 2024-02-20T14:56:05Z

crates/matrix-sdk-base/src/rooms/mod.rs

@@ -116,6 +128,34 @@ pub struct BaseRoomInfo {
    pub(crate) notable_tags: RoomNotableTags,
 }

+#[derive(Clone, Debug, Default, Serialize, Deserialize)]
+pub struct RawRoomInfo {


nit: doc comment for the whole struct please

also, question: does it need to be public so that downstream implementations of the state store trait can reuse it? (If so, please indicate so in the comment.)

I wonder if it still needs that, with my suggestion above to use serde(from / into).

bnjbvr · 2024-02-20T16:31:42Z

crates/matrix-sdk-sqlite/src/state_store.rs

+                    // Clean up raw events we saved for roominfo
+                    if room_infos.get(room_id).is_some() {


Why do we need this at all? 😨 Do we need something similar for edits? If we actually have to keep this, can we commonize the code with the indexeddb implementation?

bnjbvr · 2024-02-20T16:37:30Z

crates/matrix-sdk-indexeddb/src/state_store/mod.rs

@@ -101,6 +102,7 @@ mod keys {

    pub const ROOM_STATE: &str = "room_state";
    pub const ROOM_INFOS: &str = "room_infos";
+    pub const RAW_ROOM_INFOS: &str = "raw_room_infos";


I don't think you'd need a new table, in both cases; the justification being that serializing Raw<T> with serde_json is the same as serializing T with serde_json. So you should be able to reuse the same table.

codecov-commenter · 2024-03-04T14:06:20Z

Codecov Report

Attention: Patch coverage is 55.55556% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 83.83%. Comparing base (59c468c) to head (39ea904).
Report is 49 commits behind head on main.

❗ Current head 39ea904 differs from pull request most recent head 17c4201. Consider uploading reports for the commit 17c4201 to get more accurate results

Files	Patch %	Lines
crates/matrix-sdk-base/src/rooms/mod.rs	55.55%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3124      +/-   ##
==========================================
- Coverage   83.86%   83.83%   -0.03%     
==========================================
  Files         231      229       -2     
  Lines       23856    23718     -138     
==========================================
- Hits        20006    19884     -122     
+ Misses       3850     3834      -16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bnjbvr

Thanks for the new version of the PR! Here's another round of comments. I'm a bit scared by the code duplication, so let's see if we can lower it a bit.

bnjbvr · 2024-03-04T15:43:10Z

crates/matrix-sdk-base/src/rooms/mod.rs

+                    if event.deserialize().is_ok_and(|e| e.event_id() == event_id) {
+                        if let Ok(mut object) = event.deserialize_as() {


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

bnjbvr · 2024-03-04T15:46:08Z

crates/matrix-sdk-base/src/rooms/mod.rs

+                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() {


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

bnjbvr · 2024-03-04T15:49:13Z

crates/matrix-sdk-base/src/rooms/mod.rs

+                    .is_some_and(|e| e.event_id() == event_id)
+                {
+                    if let Ok(mut object) = event.as_ref().unwrap().deserialize_as() {
+                        if redact_in_place(&mut object, room_version, None).is_ok() {


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

bnjbvr · 2024-03-04T15:49:18Z

crates/matrix-sdk-base/src/rooms/mod.rs

+                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() {
+                            if redact_in_place(&mut object, room_version, None).is_ok() {


bnjbvr · 2024-03-04T15:49:34Z

crates/matrix-sdk-base/src/rooms/mod.rs

+            },
+            // Clean up rtc_members
+            &|map| {


style nit: please let the code breath 😁

Suggested change

},

// Clean up rtc_members

&|map| {

},

// Clean up rtc_members

&|map| {

bnjbvr · 2024-03-04T16:05:35Z

crates/matrix-sdk-base/src/rooms/normal.rs

+            Some(MinimalStateEvent::Redacted(ev)) => &ev.content.history_visibility,
+            None => &HistoryVisibility::WorldReadable,


What's the motivation behind this change?

bnjbvr · 2024-03-04T16:05:42Z

crates/matrix-sdk-base/src/rooms/normal.rs

+            Some(MinimalStateEvent::Redacted(ev)) => &ev.content.join_rule,
+            None => &JoinRule::Public,


bnjbvr · 2024-03-04T16:06:12Z

crates/matrix-sdk-base/src/rooms/normal.rs

@@ -1778,6 +1823,65 @@ mod tests {
        );
    }

+    #[async_test]
+    async fn test_save_bad_joinrules() {


Did you address this comment? ^

bnjbvr · 2024-03-04T16:09:35Z

crates/matrix-sdk-indexeddb/src/state_store/mod.rs

+                if let Some(room_state_changes) = changes.state.get(room_id) {
+                    raw_room_info.base_info.extend_with_changes(&room_state_changes);
+                }


It seems it would be much simpler to throw away the previous RawRoomInfo and convert the RoomInfo struct into a RawRoomInfo struct, instead of applying a diff of changes to the previously-serialized RawRoomInfo? That way maybe we wouldn't even need extend_with_changes at all.

bnjbvr · 2024-03-04T16:11:36Z

crates/matrix-sdk-indexeddb/src/state_store/mod.rs

+
+                // Clean up raw events we saved for roominfo
+                if changes.room_infos.get(room_id).is_some() {
+                    // This roominfo has changed, so let's update the raw events


I don't understand why we need this :/ It seems that a redaction would cause a change to the RoomInfo, so we'd convert it into a RawRoomInfo before persisting it, and we wouldn't need any special handling of redacted events?

Sorry, I think I understand it now. So as I've suggested in a private channel, the duplication of the code applying the redaction to events both in BaseRoomInfo and RawRoomInfo is a bit problematic, and a hindrance in the long term. I wonder if we could regenerate a BaseRoomInfo from the RawRoomInfo every time we received an update to the RawRoomInfo, instead? That would keep the redaction code in a single place and avoid duplication of concerns.

bnjbvr · 2024-05-02T08:00:37Z

This wasn't the path we wanted to take with this, so closing, we can revisit the approach later, as we still have an issue open describing the issue.

timokoesters requested a review from a team as a code owner February 13, 2024 15:24

timokoesters requested review from bnjbvr and removed request for a team February 13, 2024 15:24

bnjbvr requested changes Feb 15, 2024

View reviewed changes

timokoesters force-pushed the timo/badstate branch from 39ea904 to 915974d Compare February 20, 2024 12:13

timokoesters commented Feb 20, 2024

View reviewed changes

timokoesters force-pushed the timo/badstate branch from 915974d to f34b593 Compare February 20, 2024 12:17

timokoesters commented Feb 20, 2024

View reviewed changes

timokoesters force-pushed the timo/badstate branch from f34b593 to 6701933 Compare February 20, 2024 12:24

timokoesters requested a review from bnjbvr February 20, 2024 12:26

sync: Avoid crash on invalid state events

3498433

The problem was in matrix_sdk_sqlite::state_store::SqliteStateStore save_changes, which saved the RoomInfo to the disk by serializing it. Now the events are serialized as Raws using a separate struct.

bnjbvr requested changes Feb 20, 2024

View reviewed changes

timokoesters force-pushed the timo/badstate branch 3 times, most recently from 32a1fa3 to 7de60a1 Compare March 4, 2024 11:32

sync: Refactor rawroominfo to different struct

17c4201

timokoesters force-pushed the timo/badstate branch from 7de60a1 to 17c4201 Compare March 4, 2024 14:06

bnjbvr requested changes Mar 4, 2024

View reviewed changes

Hywan assigned bnjbvr Apr 25, 2024

bnjbvr closed this May 2, 2024

poljar deleted the timo/badstate branch November 5, 2024 11:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sync: Avoid crash on invalid state events #3124

sync: Avoid crash on invalid state events #3124

timokoesters commented Feb 13, 2024 •

edited

Loading

timokoesters commented Feb 13, 2024 •

edited

Loading

codecov bot commented Feb 13, 2024 •

edited

Loading

bnjbvr left a comment

timokoesters Feb 20, 2024

bnjbvr Feb 20, 2024

bnjbvr Mar 4, 2024

timokoesters Feb 20, 2024

timokoesters Mar 4, 2024

timokoesters commented Feb 20, 2024

bnjbvr Feb 20, 2024

bnjbvr Feb 20, 2024

bnjbvr Feb 20, 2024

bnjbvr Feb 20, 2024

bnjbvr Feb 20, 2024

bnjbvr Feb 20, 2024

codecov-commenter commented Mar 4, 2024

bnjbvr left a comment

bnjbvr Mar 4, 2024

bnjbvr Mar 4, 2024

bnjbvr Mar 4, 2024

bnjbvr Mar 4, 2024

bnjbvr Mar 4, 2024

bnjbvr Mar 4, 2024

bnjbvr Mar 4, 2024

bnjbvr Mar 4, 2024

bnjbvr Mar 4, 2024

bnjbvr Mar 4, 2024

bnjbvr Mar 4, 2024

bnjbvr commented May 2, 2024


		for (room_id, redactions) in &changes.redactions {
		// TODO: Load room version lazily, only if it is necessary

		// Clean up raw events we saved for roominfo
		if room_infos.get(room_id).is_some() {

		if event.deserialize().is_ok_and(\|e\| e.event_id() == event_id) {
		if let Ok(mut object) = event.deserialize_as() {

		Some(MinimalStateEvent::Redacted(ev)) => &ev.content.history_visibility,
		None => &HistoryVisibility::WorldReadable,

		Some(MinimalStateEvent::Redacted(ev)) => &ev.content.join_rule,
		None => &JoinRule::Public,

sync: Avoid crash on invalid state events #3124

sync: Avoid crash on invalid state events #3124

Conversation

timokoesters commented Feb 13, 2024 • edited Loading

timokoesters commented Feb 13, 2024 • edited Loading

codecov bot commented Feb 13, 2024 • edited Loading

Codecov Report

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timokoesters commented Feb 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 4, 2024

Codecov Report

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnjbvr commented May 2, 2024

timokoesters commented Feb 13, 2024 •

edited

Loading

timokoesters commented Feb 13, 2024 •

edited

Loading

codecov bot commented Feb 13, 2024 •

edited

Loading