From 0ebb32349026a64907585fe7d216498f0d681739 Mon Sep 17 00:00:00 2001 From: strawberry Date: Sat, 4 May 2024 09:45:37 -0400 Subject: [PATCH] resolve almost all as_conversions lints may need further opinion from others on these Signed-off-by: strawberry --- Cargo.toml | 2 +- src/api/client_server/backup.rs | 90 +++++++++++++++---------- src/api/client_server/context.rs | 4 +- src/api/client_server/directory.rs | 17 +++-- src/api/client_server/message.rs | 10 +-- src/api/client_server/search.rs | 11 ++- src/api/client_server/space.rs | 16 +++-- src/api/client_server/threads.rs | 8 ++- src/api/client_server/user_directory.rs | 2 +- src/database/mod.rs | 1 + src/database/rocksdb/mod.rs | 5 ++ src/database/sqlite/mod.rs | 44 +++++++----- src/database/watchers.rs | 2 +- src/service/globals/resolver.rs | 1 + src/service/mod.rs | 1 + src/service/pusher/mod.rs | 7 +- src/service/rooms/pdu_metadata/mod.rs | 7 +- src/utils/mod.rs | 1 + 18 files changed, 144 insertions(+), 85 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index eeac6db2..54cd8268 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -626,6 +626,7 @@ trivially_copy_pass_by_ref = "warn" wildcard_imports = "warn" checked_conversions = "warn" #integer_arithmetic = "warn" +#as_conversions = "warn" # some sadness missing_errors_doc = "allow" @@ -646,7 +647,6 @@ map_err_ignore = "allow" missing_docs_in_private_items = "allow" multiple_inherent_impl = "allow" error_impl_error = "allow" -as_conversions = "allow" string_add = "allow" string_slice = "allow" ref_patterns = "allow" diff --git a/src/api/client_server/backup.rs b/src/api/client_server/backup.rs index 4dcb5ce9..fb7e7f31 100644 --- a/src/api/client_server/backup.rs +++ b/src/api/client_server/backup.rs @@ -1,11 +1,14 @@ -use ruma::api::client::{ - backup::{ - add_backup_keys, add_backup_keys_for_room, add_backup_keys_for_session, create_backup_version, - delete_backup_keys, delete_backup_keys_for_room, delete_backup_keys_for_session, delete_backup_version, - get_backup_info, get_backup_keys, get_backup_keys_for_room, get_backup_keys_for_session, - get_latest_backup_info, update_backup_version, +use ruma::{ + api::client::{ + backup::{ + add_backup_keys, add_backup_keys_for_room, add_backup_keys_for_session, create_backup_version, + delete_backup_keys, delete_backup_keys_for_room, delete_backup_keys_for_session, delete_backup_version, + get_backup_info, get_backup_keys, get_backup_keys_for_room, get_backup_keys_for_session, + get_latest_backup_info, update_backup_version, + }, + error::ErrorKind, }, - error::ErrorKind, + UInt, }; use crate::{services, Error, Result, Ruma}; @@ -56,7 +59,8 @@ pub(crate) async fn get_latest_backup_info_route( Ok(get_latest_backup_info::v3::Response { algorithm, - count: (services().key_backups.count_keys(sender_user, &version)? as u32).into(), + count: (UInt::try_from(services().key_backups.count_keys(sender_user, &version)?) + .expect("user backup keys count should not be that high")), etag: services().key_backups.get_etag(sender_user, &version)?, version, }) @@ -76,10 +80,12 @@ pub(crate) async fn get_backup_info_route( Ok(get_backup_info::v3::Response { algorithm, - count: (services() - .key_backups - .count_keys(sender_user, &body.version)? as u32) - .into(), + count: (UInt::try_from( + services() + .key_backups + .count_keys(sender_user, &body.version)?, + ) + .expect("user backup keys count should not be that high")), etag: services() .key_backups .get_etag(sender_user, &body.version)?, @@ -139,10 +145,12 @@ pub(crate) async fn add_backup_keys_route( } Ok(add_backup_keys::v3::Response { - count: (services() - .key_backups - .count_keys(sender_user, &body.version)? as u32) - .into(), + count: (UInt::try_from( + services() + .key_backups + .count_keys(sender_user, &body.version)?, + ) + .expect("user backup keys count should not be that high")), etag: services() .key_backups .get_etag(sender_user, &body.version)?, @@ -181,10 +189,12 @@ pub(crate) async fn add_backup_keys_for_room_route( } Ok(add_backup_keys_for_room::v3::Response { - count: (services() - .key_backups - .count_keys(sender_user, &body.version)? as u32) - .into(), + count: (UInt::try_from( + services() + .key_backups + .count_keys(sender_user, &body.version)?, + ) + .expect("user backup keys count should not be that high")), etag: services() .key_backups .get_etag(sender_user, &body.version)?, @@ -221,10 +231,12 @@ pub(crate) async fn add_backup_keys_for_session_route( .add_key(sender_user, &body.version, &body.room_id, &body.session_id, &body.session_data)?; Ok(add_backup_keys_for_session::v3::Response { - count: (services() - .key_backups - .count_keys(sender_user, &body.version)? as u32) - .into(), + count: (UInt::try_from( + services() + .key_backups + .count_keys(sender_user, &body.version)?, + ) + .expect("user backup keys count should not be that high")), etag: services() .key_backups .get_etag(sender_user, &body.version)?, @@ -294,10 +306,12 @@ pub(crate) async fn delete_backup_keys_route( .delete_all_keys(sender_user, &body.version)?; Ok(delete_backup_keys::v3::Response { - count: (services() - .key_backups - .count_keys(sender_user, &body.version)? as u32) - .into(), + count: (UInt::try_from( + services() + .key_backups + .count_keys(sender_user, &body.version)?, + ) + .expect("user backup keys count should not be that high")), etag: services() .key_backups .get_etag(sender_user, &body.version)?, @@ -317,10 +331,12 @@ pub(crate) async fn delete_backup_keys_for_room_route( .delete_room_keys(sender_user, &body.version, &body.room_id)?; Ok(delete_backup_keys_for_room::v3::Response { - count: (services() - .key_backups - .count_keys(sender_user, &body.version)? as u32) - .into(), + count: (UInt::try_from( + services() + .key_backups + .count_keys(sender_user, &body.version)?, + ) + .expect("user backup keys count should not be that high")), etag: services() .key_backups .get_etag(sender_user, &body.version)?, @@ -340,10 +356,12 @@ pub(crate) async fn delete_backup_keys_for_session_route( .delete_room_key(sender_user, &body.version, &body.room_id, &body.session_id)?; Ok(delete_backup_keys_for_session::v3::Response { - count: (services() - .key_backups - .count_keys(sender_user, &body.version)? as u32) - .into(), + count: (UInt::try_from( + services() + .key_backups + .count_keys(sender_user, &body.version)?, + ) + .expect("user backup keys count should not be that high")), etag: services() .key_backups .get_etag(sender_user, &body.version)?, diff --git a/src/api/client_server/context.rs b/src/api/client_server/context.rs index 2291e586..23a89f6c 100644 --- a/src/api/client_server/context.rs +++ b/src/api/client_server/context.rs @@ -63,8 +63,8 @@ pub(crate) async fn get_context_route(body: Ruma) -> R lazy_loaded.insert(base_event.sender.as_str().to_owned()); } - // Use limit with maximum 100 - let limit = u64::from(body.limit).min(100) as usize; + // Use limit or else 10, with maximum 100 + let limit = usize::try_from(body.limit).unwrap_or(10).min(100); let base_event = base_event.to_room_event(); diff --git a/src/api/client_server/directory.rs b/src/api/client_server/directory.rs index 37745749..094e007a 100644 --- a/src/api/client_server/directory.rs +++ b/src/api/client_server/directory.rs @@ -20,7 +20,7 @@ use ruma::{ }, StateEventType, }, - ServerName, UInt, + uint, ServerName, UInt, }; use tracing::{error, info, warn}; @@ -198,8 +198,9 @@ pub(crate) async fn get_public_rooms_filtered_helper( }); } + // Use limit or else 10, with maximum 100 let limit = limit.map_or(10, u64::from); - let mut num_since = 0_u64; + let mut num_since: u64 = 0; if let Some(s) = &since { let mut characters = s.chars(); @@ -363,12 +364,16 @@ pub(crate) async fn get_public_rooms_filtered_helper( all_rooms.sort_by(|l, r| r.num_joined_members.cmp(&l.num_joined_members)); - let total_room_count_estimate = (all_rooms.len() as u32).into(); + let total_room_count_estimate = UInt::try_from(all_rooms.len()).unwrap_or_else(|_| uint!(0)); let chunk: Vec<_> = all_rooms .into_iter() - .skip(num_since as usize) - .take(limit as usize) + .skip( + num_since + .try_into() + .expect("num_since should not be this high"), + ) + .take(limit.try_into().expect("limit should not be this high")) .collect(); let prev_batch = if num_since == 0 { @@ -377,7 +382,7 @@ pub(crate) async fn get_public_rooms_filtered_helper( Some(format!("p{num_since}")) }; - let next_batch = if chunk.len() < limit as usize { + let next_batch = if chunk.len() < limit.try_into().unwrap() { None } else { Some(format!( diff --git a/src/api/client_server/message.rs b/src/api/client_server/message.rs index 3a30ed15..0aa7792d 100644 --- a/src/api/client_server/message.rs +++ b/src/api/client_server/message.rs @@ -144,7 +144,7 @@ pub(crate) async fn get_message_events_route( .lazy_load_confirm_delivery(sender_user, sender_device, &body.room_id, from) .await?; - let limit = u64::from(body.limit).min(100) as usize; + let limit = usize::try_from(body.limit).unwrap_or(10).min(100); let next_token; @@ -159,8 +159,9 @@ pub(crate) async fn get_message_events_route( .timeline .pdus_after(sender_user, &body.room_id, from)? .filter_map(Result::ok) // Filter out buggy events - .filter(|(_, pdu)| contains_url_filter(pdu, &body.filter)) - .filter(|(_, pdu)| visibility_filter(pdu, sender_user, &body.room_id)) + .filter(|(_, pdu)| { contains_url_filter(pdu, &body.filter) && visibility_filter(pdu, sender_user, &body.room_id) + + }) .take_while(|&(k, _)| Some(k) != to) // Stop at `to` .take(limit) .collect(); @@ -205,8 +206,7 @@ pub(crate) async fn get_message_events_route( .timeline .pdus_until(sender_user, &body.room_id, from)? .filter_map(Result::ok) // Filter out buggy events - .filter(|(_, pdu)| contains_url_filter(pdu, &body.filter)) - .filter(|(_, pdu)| visibility_filter(pdu, sender_user, &body.room_id)) + .filter(|(_, pdu)| {contains_url_filter(pdu, &body.filter) && visibility_filter(pdu, sender_user, &body.room_id)}) .take_while(|&(k, _)| Some(k) != to) // Stop at `to` .take(limit) .collect(); diff --git a/src/api/client_server/search.rs b/src/api/client_server/search.rs index b80694e6..b09c6326 100644 --- a/src/api/client_server/search.rs +++ b/src/api/client_server/search.rs @@ -10,7 +10,7 @@ use ruma::{ }, events::AnyStateEvent, serde::Raw, - OwnedRoomId, + uint, OwnedRoomId, }; use tracing::debug; @@ -39,7 +39,12 @@ pub(crate) async fn search_events_route(body: Ruma) }); // Use limit or else 10, with maximum 100 - let limit = filter.limit.map_or(10, u64::from).min(100) as usize; + let limit: usize = filter + .limit + .unwrap_or_else(|| uint!(10)) + .try_into() + .unwrap_or(10) + .min(100); let mut room_states: BTreeMap>> = BTreeMap::new(); @@ -167,7 +172,7 @@ pub(crate) async fn search_events_route(body: Ruma) Ok(search_events::v3::Response::new(ResultCategories { room_events: ResultRoomEvents { - count: Some((results.len() as u32).into()), + count: Some(results.len().try_into().unwrap_or_else(|_| uint!(0))), groups: BTreeMap::new(), // TODO next_batch, results, diff --git a/src/api/client_server/space.rs b/src/api/client_server/space.rs index 69ccbc70..61431c01 100644 --- a/src/api/client_server/space.rs +++ b/src/api/client_server/space.rs @@ -2,7 +2,7 @@ use std::str::FromStr; use ruma::{ api::client::{error::ErrorKind, space::get_hierarchy}, - UInt, + uint, UInt, }; use crate::{service::rooms::spaces::PagnationToken, services, Error, Result, Ruma}; @@ -14,10 +14,12 @@ use crate::{service::rooms::spaces::PagnationToken, services, Error, Result, Rum pub(crate) async fn get_hierarchy_route(body: Ruma) -> Result { let sender_user = body.sender_user.as_ref().expect("user is authenticated"); - let limit = body + let limit: usize = body .limit - .unwrap_or_else(|| UInt::from(10_u32)) - .min(UInt::from(100_u32)); + .unwrap_or_else(|| uint!(10)) + .try_into() + .unwrap_or(10) + .min(100); let max_depth = body .max_depth @@ -45,9 +47,9 @@ pub(crate) async fn get_hierarchy_route(body: Ruma) .get_client_hierarchy( sender_user, &body.room_id, - u64::from(limit) as usize, - key.map_or(0, |token| u64::from(token.skip) as usize), - u64::from(max_depth) as usize, + limit, + key.map_or(0, |token| token.skip.try_into().unwrap_or(0)), + max_depth.try_into().unwrap_or(3), body.suggested_only, ) .await diff --git a/src/api/client_server/threads.rs b/src/api/client_server/threads.rs index 6b09f03c..2895573a 100644 --- a/src/api/client_server/threads.rs +++ b/src/api/client_server/threads.rs @@ -1,4 +1,7 @@ -use ruma::api::client::{error::ErrorKind, threads::get_threads}; +use ruma::{ + api::client::{error::ErrorKind, threads::get_threads}, + uint, +}; use crate::{services, Error, Result, Ruma}; @@ -9,7 +12,8 @@ pub(crate) async fn get_threads_route(body: Ruma) -> R // Use limit or else 10, with maximum 100 let limit = body .limit - .and_then(|l| l.try_into().ok()) + .unwrap_or_else(|| uint!(10)) + .try_into() .unwrap_or(10) .min(100); diff --git a/src/api/client_server/user_directory.rs b/src/api/client_server/user_directory.rs index c57f569e..4d9947f8 100644 --- a/src/api/client_server/user_directory.rs +++ b/src/api/client_server/user_directory.rs @@ -17,7 +17,7 @@ use crate::{services, Result, Ruma}; /// and don't share a room with the sender pub(crate) async fn search_users_route(body: Ruma) -> Result { let sender_user = body.sender_user.as_ref().expect("user is authenticated"); - let limit = u64::from(body.limit) as usize; + let limit = usize::try_from(body.limit).unwrap_or(10); // default limit is 10 let mut users = services().users.iter().filter_map(|user_id| { // Filter out buggy users (they should not exist, but you never know...) diff --git a/src/database/mod.rs b/src/database/mod.rs index b61142ea..825a21e8 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -340,6 +340,7 @@ impl KeyValueDatabase { roomid_inviteviaservers: builder.open_tree("roomid_inviteviaservers")?, + #[allow(clippy::as_conversions, clippy::cast_sign_loss, clippy::cast_possible_truncation)] auth_chain_cache: Mutex::new(LruCache::new( (f64::from(config.auth_chain_cache_capacity) * config.conduit_cache_capacity_modifier) as usize, )), diff --git a/src/database/rocksdb/mod.rs b/src/database/rocksdb/mod.rs index 1eec597a..4ef8f9a7 100644 --- a/src/database/rocksdb/mod.rs +++ b/src/database/rocksdb/mod.rs @@ -35,7 +35,11 @@ pub(crate) struct Engine { impl KeyValueDatabaseEngine for Arc { fn open(config: &Config) -> Result { let cache_capacity_bytes = config.db_cache_capacity_mb * 1024.0 * 1024.0; + + #[allow(clippy::as_conversions, clippy::cast_sign_loss, clippy::cast_possible_truncation)] let row_cache_capacity_bytes = (cache_capacity_bytes * 0.50) as usize; + + #[allow(clippy::as_conversions, clippy::cast_sign_loss, clippy::cast_possible_truncation)] let col_cache_capacity_bytes = (cache_capacity_bytes * 0.50) as usize; let mut col_cache = HashMap::new(); @@ -128,6 +132,7 @@ impl KeyValueDatabaseEngine for Arc { Ok(()) } + #[allow(clippy::as_conversions, clippy::cast_sign_loss, clippy::cast_possible_truncation)] fn memory_usage(&self) -> Result { let mut res = String::new(); let stats = rust_rocksdb::perf::get_memory_usage_stats(Some(&[&self.rocks]), Some(&[&self.row_cache]))?; diff --git a/src/database/sqlite/mod.rs b/src/database/sqlite/mod.rs index 60014ab0..c43d2fe0 100644 --- a/src/database/sqlite/mod.rs +++ b/src/database/sqlite/mod.rs @@ -21,7 +21,7 @@ thread_local! { struct PreparedStatementIterator<'a> { iterator: Box + 'a>, - _statement_ref: NonAliasingBox>, + _statement_ref: AliasableBox>, } impl Iterator for PreparedStatementIterator<'_> { @@ -30,16 +30,25 @@ impl Iterator for PreparedStatementIterator<'_> { fn next(&mut self) -> Option { self.iterator.next() } } -struct NonAliasingBox(*mut T); -impl Drop for NonAliasingBox { +struct AliasableBox(*mut T); +impl Drop for AliasableBox { fn drop(&mut self) { - // TODO: figure out why this is necessary, but also this is sqlite so dont think - // i care that much. i tried checking commit history but couldn't find out why - // this was done. - #[allow(clippy::undocumented_unsafe_blocks)] - unsafe { - _ = Box::from_raw(self.0); - }; + // SAFETY: This is cursed and relies on non-local reasoning. + // + // In order for this to be safe: + // + // * All aliased references to this value must have been dropped first, for + // example by coming after its referrers in struct fields, because struct + // fields are automatically dropped in order from top to bottom in the absence + // of an explicit Drop impl. Otherwise, the referrers may read into + // deallocated memory. + // * This type must not be copyable or cloneable. Otherwise, double-free can + // occur. + // + // These conditions are met, but again, note that changing safe code in + // this module can result in unsoundness if any of these constraints are + // violated. + unsafe { drop(Box::from_raw(self.0)) } } } @@ -93,8 +102,13 @@ impl KeyValueDatabaseEngine for Arc { // 2. divide by permanent connections + permanent iter connections + write // connection // 3. round down to nearest integer - let cache_size_per_thread: u32 = - ((config.db_cache_capacity_mb * 1024.0) / ((num_cpus::get().max(1) * 2) + 1) as f64) as u32; + #[allow( + clippy::as_conversions, + clippy::cast_possible_truncation, + clippy::cast_precision_loss, + clippy::cast_sign_loss + )] + let cache_size_per_thread = ((config.db_cache_capacity_mb * 1024.0) / ((num_cpus::get() as f64 * 2.0) + 1.0)) as u32; let writer = Mutex::new(Engine::prepare_conn(&path, cache_size_per_thread)?); @@ -161,7 +175,7 @@ impl SqliteTable { .unwrap(), )); - let statement_ref = NonAliasingBox(statement); + let statement_ref = AliasableBox(statement); //let name = self.name.clone(); @@ -250,7 +264,7 @@ impl KvTree for SqliteTable { .unwrap(), )); - let statement_ref = NonAliasingBox(statement); + let statement_ref = AliasableBox(statement); let iterator = Box::new( statement @@ -272,7 +286,7 @@ impl KvTree for SqliteTable { .unwrap(), )); - let statement_ref = NonAliasingBox(statement); + let statement_ref = AliasableBox(statement); let iterator = Box::new( statement diff --git a/src/database/watchers.rs b/src/database/watchers.rs index a4152a09..93c82b44 100644 --- a/src/database/watchers.rs +++ b/src/database/watchers.rs @@ -47,7 +47,7 @@ impl Watchers { let mut watchers = self.watchers.write().unwrap(); for prefix in triggered { if let Some(tx) = watchers.remove(prefix) { - _ = tx.0.send(()); + tx.0.send(()).expect("channel should still be open"); } } }; diff --git a/src/service/globals/resolver.rs b/src/service/globals/resolver.rs index 96ee7382..190c8579 100644 --- a/src/service/globals/resolver.rs +++ b/src/service/globals/resolver.rs @@ -30,6 +30,7 @@ pub(crate) struct Hooked { } impl Resolver { + #[allow(clippy::as_conversions, clippy::cast_sign_loss, clippy::cast_possible_truncation)] pub(crate) fn new(config: &Config) -> Self { let (sys_conf, mut opts) = hickory_resolver::system_conf::read_system_conf() .map_err(|e| { diff --git a/src/service/mod.rs b/src/service/mod.rs index 86fd103a..3fbc435a 100644 --- a/src/service/mod.rs +++ b/src/service/mod.rs @@ -40,6 +40,7 @@ pub(crate) struct Services<'a> { } impl Services<'_> { + #[allow(clippy::as_conversions, clippy::cast_sign_loss, clippy::cast_possible_truncation)] pub(crate) fn build< D: appservice::Data + pusher::Data diff --git a/src/service/pusher/mod.rs b/src/service/pusher/mod.rs index 9d2b53b7..3f7c5d80 100644 --- a/src/service/pusher/mod.rs +++ b/src/service/pusher/mod.rs @@ -188,13 +188,14 @@ impl Service { let ctx = PushConditionRoomCtx { room_id: room_id.to_owned(), - member_count: UInt::from( + member_count: UInt::try_from( services() .rooms .state_cache .room_joined_count(room_id)? - .unwrap_or(1) as u32, - ), + .unwrap_or(1), + ) + .unwrap_or_else(|_| uint!(0)), user_id: user.to_owned(), user_display_name: services() .users diff --git a/src/service/rooms/pdu_metadata/mod.rs b/src/service/rooms/pdu_metadata/mod.rs index d5afe247..0908c573 100644 --- a/src/service/rooms/pdu_metadata/mod.rs +++ b/src/service/rooms/pdu_metadata/mod.rs @@ -5,7 +5,7 @@ pub(crate) use data::Data; use ruma::{ api::{client::relations::get_relating_events, Direction}, events::{relation::RelationType, TimelineEventType}, - EventId, RoomId, UInt, UserId, + uint, EventId, RoomId, UInt, UserId, }; use serde::Deserialize; @@ -57,8 +57,9 @@ impl Service { // Use limit or else 10, with maximum 100 let limit = limit - .and_then(|u| u32::try_from(u).ok()) - .map_or(10_usize, |u| u as usize) + .unwrap_or_else(|| uint!(10)) + .try_into() + .unwrap_or(10) .min(100); let next_token; diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 1184e181..ab472b28 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -21,6 +21,7 @@ use crate::{services, Error, Result}; pub(crate) fn clamp(val: T, min: T, max: T) -> T { cmp::min(cmp::max(val, min), max) } +#[allow(clippy::as_conversions)] pub(crate) fn millis_since_unix_epoch() -> u64 { SystemTime::now() .duration_since(UNIX_EPOCH)