From 607d7ebc22a31199a7d008950be663e63aaeca7a Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Tue, 5 Jul 2022 08:59:11 +0200 Subject: [PATCH] fix(bindings/cryto-nodejs): Fix memory corruption in async functions. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In async functions, the Node.js GC may or may not (that's a random behavior) collect the arguments passed to the function as soon as it returns. The function may not be executed yet, since it's async. Thus, it leads to memory corruption: The function tries to read later on the value inside an argument and… it crashes at best. To avoid this bug, there is no other choice than cloning the values before the function returns, in its “sync path” (so before any transformation of an `.await` point into an “async block”). The performance impact is not “massive”, I'm not sure it could be noticeable easily since it is most of the time related to identifiers (e.g. `UserId`), which are cheap to clone. I have to find the balance here, and cloning offers the best trade off from my point of view. --- .../src/identifiers.rs | 4 --- .../matrix-sdk-crypto-nodejs/src/machine.rs | 34 +++++++++++++------ 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/bindings/matrix-sdk-crypto-nodejs/src/identifiers.rs b/bindings/matrix-sdk-crypto-nodejs/src/identifiers.rs index 976563265..eb45873b8 100644 --- a/bindings/matrix-sdk-crypto-nodejs/src/identifiers.rs +++ b/bindings/matrix-sdk-crypto-nodejs/src/identifiers.rs @@ -59,10 +59,6 @@ impl UserId { } } -pub(crate) fn lower_user_ids_to_ruma(users: Vec<&UserId>) -> impl Iterator { - users.into_iter().map(|user| user.inner.as_ref()) -} - /// A Matrix device ID. /// /// Device identifiers in Matrix are completely opaque character diff --git a/bindings/matrix-sdk-crypto-nodejs/src/machine.rs b/bindings/matrix-sdk-crypto-nodejs/src/machine.rs index 0f92b6131..3899981df 100644 --- a/bindings/matrix-sdk-crypto-nodejs/src/machine.rs +++ b/bindings/matrix-sdk-crypto-nodejs/src/machine.rs @@ -63,6 +63,9 @@ impl OlmMachine { store_path: Option, mut store_passphrase: Option, ) -> napi::Result { + let user_id = user_id.clone(); + let device_id = device_id.clone(); + let store = store_path .map(|store_path| { matrix_sdk_sled::CryptoStore::open_with_passphrase( @@ -150,7 +153,7 @@ impl OlmMachine { unused_fallback_keys: Vec, ) -> napi::Result { let to_device_events = serde_json::from_str(to_device_events.as_ref()).map_err(into_err)?; - let changed_devices = &changed_devices.inner; + let changed_devices = changed_devices.inner.clone(); let one_time_key_counts = one_time_key_counts .iter() .map(|(key, value)| (DeviceKeyAlgorithm::from(key.as_str()), UInt::from(*value))) @@ -167,7 +170,7 @@ impl OlmMachine { .inner .receive_sync_changes( to_device_events, - changed_devices, + &changed_devices, &one_time_key_counts, unused_fallback_keys.as_deref(), ) @@ -275,9 +278,15 @@ impl OlmMachine { &self, users: Option>, ) -> napi::Result> { + let users = users + .unwrap_or_default() + .into_iter() + .map(|user| user.inner.clone()) + .collect::>(); + match self .inner - .get_missing_sessions(identifiers::lower_user_ids_to_ruma(users.unwrap_or_default())) + .get_missing_sessions(users.iter().map(AsRef::as_ref)) .await .map_err(into_err)? { @@ -306,7 +315,9 @@ impl OlmMachine { /// * `users`, an array over user IDs that should be marked for tracking. #[napi] pub async fn update_tracked_users(&self, users: Vec<&identifiers::UserId>) { - self.inner.update_tracked_users(identifiers::lower_user_ids_to_ruma(users)).await; + let users = users.into_iter().map(|user| user.inner.clone()).collect::>(); + + self.inner.update_tracked_users(users.iter().map(AsRef::as_ref)).await; } /// Get to-device requests to share a room key with users in a room. @@ -323,15 +334,15 @@ impl OlmMachine { users: Vec<&identifiers::UserId>, encryption_settings: &encryption::EncryptionSettings, ) -> napi::Result { - let room_id = room_id.inner.as_ref(); - let users = identifiers::lower_user_ids_to_ruma(users); + let room_id = room_id.inner.clone(); + let users = users.into_iter().map(|user| user.inner.clone()).collect::>(); let encryption_settings = matrix_sdk_crypto::olm::EncryptionSettings::from(encryption_settings); serde_json::to_string( &self .inner - .share_room_key(room_id, users, encryption_settings) + .share_room_key(&room_id, users.iter().map(AsRef::as_ref), encryption_settings) .await .map_err(into_err)?, ) @@ -354,13 +365,13 @@ impl OlmMachine { event_type: String, content: String, ) -> napi::Result { - let room_id = room_id.inner.as_ref(); + let room_id = room_id.inner.clone(); let content: JsonValue = serde_json::from_str(content.as_str()).map_err(into_err)?; serde_json::to_string( &self .inner - .encrypt_room_event_raw(room_id, content, event_type.as_ref()) + .encrypt_room_event_raw(&room_id, content, event_type.as_ref()) .await .map_err(into_err)?, ) @@ -381,8 +392,9 @@ impl OlmMachine { ) -> napi::Result { let event: OriginalSyncRoomEncryptedEvent = serde_json::from_str(event.as_str()).map_err(into_err)?; - let room_id = room_id.inner.as_ref(); - let room_event = self.inner.decrypt_room_event(&event, room_id).await.map_err(into_err)?; + let room_id = room_id.inner.clone(); + + let room_event = self.inner.decrypt_room_event(&event, &room_id).await.map_err(into_err)?; Ok(room_event.into()) }