From 8353f7c96044c525f49abf8bef67d77d939fa2d2 Mon Sep 17 00:00:00 2001 From: Gnome! Date: Wed, 3 Jan 2024 18:53:38 +0000 Subject: [PATCH] Get rid of `unsafe` (#2686) A discord bot library should not be using the tools reserved for low level OS interaction/data structure libraries. --- CONTRIBUTING.md | 7 ++- command_attr/src/util.rs | 2 +- src/http/routing.rs | 22 +++++++--- src/lib.rs | 1 + src/model/guild/audit_log/mod.rs | 74 ++++++++++++++++++++++++-------- src/model/id.rs | 2 +- 6 files changed, 77 insertions(+), 31 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7d615bc4cc9..c7ca981d5bd 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -63,10 +63,9 @@ your code. ## Unsafe -Code that defines or uses `unsafe` functions must be reasoned with comments. -`unsafe` code can pose a potential for undefined behaviour related bugs and other -kinds of bugs to sprout if misused, weakening security. If you commit code containing -`unsafe`, you should confirm that its usage is necessary and correct. +Unsafe code is forbidden, and safe alternatives must be found. This can be mitigated by using +a third party crate to offload the burden of justifying the unsafe code, or finding a safe +alternative. # Comment / Documentation style diff --git a/command_attr/src/util.rs b/command_attr/src/util.rs index 60eb1afd8f3..ae1c59a07f3 100644 --- a/command_attr/src/util.rs +++ b/command_attr/src/util.rs @@ -31,7 +31,7 @@ impl LitExt for Lit { fn to_str(&self) -> String { match self { Self::Str(s) => s.value(), - Self::ByteStr(s) => unsafe { String::from_utf8_unchecked(s.value()) }, + Self::ByteStr(s) => String::from_utf8_lossy(&s.value()).into_owned(), Self::Char(c) => c.value().to_string(), Self::Byte(b) => (b.value() as char).to_string(), _ => panic!("values must be a (byte)string or a char"), diff --git a/src/http/routing.rs b/src/http/routing.rs index 40cba502b20..9bc4b1508ab 100644 --- a/src/http/routing.rs +++ b/src/http/routing.rs @@ -5,7 +5,7 @@ use crate::model::id::*; /// Used to group requests together for ratelimiting. #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] -pub struct RatelimitingBucket(Option<(std::mem::Discriminant>, Option)>); +pub struct RatelimitingBucket(Option<(RouteKind, Option)>); impl RatelimitingBucket { #[must_use] @@ -40,7 +40,20 @@ macro_rules! routes { )+ } + #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] + enum RouteKind { + $($name,)+ + } + impl<$lt> Route<$lt> { + fn kind(&self) -> RouteKind { + match self { + $( + Self::$name {..} => RouteKind::$name, + )+ + } + } + #[must_use] pub fn path(self) -> Cow<'static, str> { match self { @@ -59,17 +72,12 @@ macro_rules! routes { )+ }; - // To avoid adding a lifetime on RatelimitingBucket and causing lifetime infection, - // we transmute the Discriminant> to Discriminant>. - // SAFETY: std::mem::discriminant erases lifetimes. - let discriminant = unsafe { std::mem::transmute(std::mem::discriminant(self)) }; - RatelimitingBucket(ratelimiting_kind.map(|r| { let id = match r { RatelimitingKind::PathAndId(id) => Some(id), RatelimitingKind::Path => None, }; - (discriminant, id) + (self.kind(), id) })) } diff --git a/src/lib.rs b/src/lib.rs index a35154eb514..100ce5cdabb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -50,6 +50,7 @@ //! [gateway docs]: crate::gateway #![doc(html_root_url = "https://docs.rs/serenity/*")] #![cfg_attr(docsrs, feature(doc_auto_cfg))] +#![forbid(unsafe_code)] #![warn( unused, rust_2018_idioms, diff --git a/src/model/guild/audit_log/mod.rs b/src/model/guild/audit_log/mod.rs index b7cc316e54b..f71c3f3fcf1 100644 --- a/src/model/guild/audit_log/mod.rs +++ b/src/model/guild/audit_log/mod.rs @@ -1,7 +1,5 @@ //! Audit log types for administrative actions within guilds. -use std::mem::transmute; - use nonmax::{NonMaxU32, NonMaxU64}; use serde::ser::{Serialize, Serializer}; @@ -70,22 +68,62 @@ impl Action { pub fn from_value(value: u8) -> Action { match value { 1 => Action::GuildUpdate, - 10..=12 => Action::Channel(unsafe { transmute(value) }), - 13..=15 => Action::ChannelOverwrite(unsafe { transmute(value) }), - 20..=28 => Action::Member(unsafe { transmute(value) }), - 30..=32 => Action::Role(unsafe { transmute(value) }), - 40..=42 => Action::Invite(unsafe { transmute(value) }), - 50..=52 => Action::Webhook(unsafe { transmute(value) }), - 60..=62 => Action::Emoji(unsafe { transmute(value) }), - 72..=75 => Action::Message(unsafe { transmute(value) }), - 80..=82 => Action::Integration(unsafe { transmute(value) }), - 83..=85 => Action::StageInstance(unsafe { transmute(value) }), - 90..=92 => Action::Sticker(unsafe { transmute(value) }), - 100..=102 => Action::ScheduledEvent(unsafe { transmute(value) }), - 110..=112 => Action::Thread(unsafe { transmute(value) }), - 140..=145 => Action::AutoMod(unsafe { transmute(value) }), - 150..=151 => Action::CreatorMonetization(unsafe { transmute(value) }), - 192..=193 => Action::VoiceChannelStatus(unsafe { transmute(value) }), + 10 => Action::Channel(ChannelAction::Create), + 11 => Action::Channel(ChannelAction::Update), + 12 => Action::Channel(ChannelAction::Delete), + 13 => Action::ChannelOverwrite(ChannelOverwriteAction::Create), + 14 => Action::ChannelOverwrite(ChannelOverwriteAction::Update), + 15 => Action::ChannelOverwrite(ChannelOverwriteAction::Delete), + 20 => Action::Member(MemberAction::Kick), + 21 => Action::Member(MemberAction::Prune), + 22 => Action::Member(MemberAction::BanAdd), + 23 => Action::Member(MemberAction::BanRemove), + 24 => Action::Member(MemberAction::Update), + 25 => Action::Member(MemberAction::RoleUpdate), + 26 => Action::Member(MemberAction::MemberMove), + 27 => Action::Member(MemberAction::MemberDisconnect), + 28 => Action::Member(MemberAction::BotAdd), + 30 => Action::Role(RoleAction::Create), + 31 => Action::Role(RoleAction::Update), + 32 => Action::Role(RoleAction::Delete), + 40 => Action::Invite(InviteAction::Create), + 41 => Action::Invite(InviteAction::Update), + 42 => Action::Invite(InviteAction::Delete), + 50 => Action::Webhook(WebhookAction::Create), + 51 => Action::Webhook(WebhookAction::Update), + 52 => Action::Webhook(WebhookAction::Delete), + 60 => Action::Emoji(EmojiAction::Create), + 61 => Action::Emoji(EmojiAction::Update), + 62 => Action::Emoji(EmojiAction::Delete), + 72 => Action::Message(MessageAction::Delete), + 73 => Action::Message(MessageAction::BulkDelete), + 74 => Action::Message(MessageAction::Pin), + 75 => Action::Message(MessageAction::Unpin), + 80 => Action::Integration(IntegrationAction::Create), + 81 => Action::Integration(IntegrationAction::Update), + 82 => Action::Integration(IntegrationAction::Delete), + 83 => Action::StageInstance(StageInstanceAction::Create), + 84 => Action::StageInstance(StageInstanceAction::Update), + 85 => Action::StageInstance(StageInstanceAction::Delete), + 90 => Action::Sticker(StickerAction::Create), + 91 => Action::Sticker(StickerAction::Update), + 92 => Action::Sticker(StickerAction::Delete), + 100 => Action::ScheduledEvent(ScheduledEventAction::Create), + 101 => Action::ScheduledEvent(ScheduledEventAction::Update), + 102 => Action::ScheduledEvent(ScheduledEventAction::Delete), + 110 => Action::Thread(ThreadAction::Create), + 111 => Action::Thread(ThreadAction::Update), + 112 => Action::Thread(ThreadAction::Delete), + 140 => Action::AutoMod(AutoModAction::RuleCreate), + 141 => Action::AutoMod(AutoModAction::RuleUpdate), + 142 => Action::AutoMod(AutoModAction::RuleDelete), + 143 => Action::AutoMod(AutoModAction::BlockMessage), + 144 => Action::AutoMod(AutoModAction::FlagToChannel), + 145 => Action::AutoMod(AutoModAction::UserCommunicationDisabled), + 150 => Action::CreatorMonetization(CreatorMonetizationAction::RequestCreated), + 151 => Action::CreatorMonetization(CreatorMonetizationAction::TermsAccepted), + 192 => Action::VoiceChannelStatus(VoiceChannelStatusAction::StatusUpdate), + 193 => Action::VoiceChannelStatus(VoiceChannelStatusAction::StatusDelete), _ => Action::Unknown(value), } } diff --git a/src/model/id.rs b/src/model/id.rs index 69616621ff5..a44ca66ea98 100644 --- a/src/model/id.rs +++ b/src/model/id.rs @@ -90,7 +90,7 @@ macro_rules! id_u64 { impl From<$name> for NonZeroI64 { fn from(id: $name) -> NonZeroI64 { - unsafe {NonZeroI64::new_unchecked(id.get() as i64)} + NonZeroI64::new(id.get() as i64).unwrap() } }