From ceebb374f0db8848a8d4135c5a06b466f2d963e1 Mon Sep 17 00:00:00 2001 From: Jason Gray Date: Sun, 23 Jan 2022 12:02:04 -0600 Subject: [PATCH] Remove unsafe code (#940) Remove unsafe code --- CHANGELOG.md | 1 + core/src/cache.rs | 17 ++- core/src/spotify_id.rs | 34 +++-- metadata/src/lib.rs | 262 +++++++++++++++++++++++------------- playback/src/player.rs | 7 +- src/player_event_handler.rs | 124 +++++++++++++---- 6 files changed, 295 insertions(+), 150 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c0a91a29..74809104 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - [main] Prevent hang when discovery is disabled and there are no credentials or when bad credentials are given. - [main] Don't panic when parsing options. Instead list valid values and exit. - [main] `--alsa-mixer-device` and `--alsa-mixer-index` now fallback to the card and index specified in `--device`. +- [core] Removed unsafe code (breaking) ### Removed - [playback] `alsamixer`: previously deprecated option `mixer-card` has been removed. diff --git a/core/src/cache.rs b/core/src/cache.rs index da2ad022..f8b0ca2c 100644 --- a/core/src/cache.rs +++ b/core/src/cache.rs @@ -351,12 +351,17 @@ impl Cache { } fn file_path(&self, file: FileId) -> Option { - self.audio_location.as_ref().map(|location| { - let name = file.to_base16(); - let mut path = location.join(&name[0..2]); - path.push(&name[2..]); - path - }) + match file.to_base16() { + Ok(name) => self.audio_location.as_ref().map(|location| { + let mut path = location.join(&name[0..2]); + path.push(&name[2..]); + path + }), + Err(e) => { + warn!("Invalid FileId: {}", e.utf8_error()); + None + } + } } pub fn file(&self, file: FileId) -> Option { diff --git a/core/src/spotify_id.rs b/core/src/spotify_id.rs index e6e2bae0..10298a42 100644 --- a/core/src/spotify_id.rs +++ b/core/src/spotify_id.rs @@ -2,6 +2,7 @@ use std::convert::TryInto; use std::fmt; +use std::string::FromUtf8Error; #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub enum SpotifyAudioType { @@ -136,7 +137,7 @@ impl SpotifyId { /// Returns the `SpotifyId` as a base16 (hex) encoded, `SpotifyId::SIZE_BASE16` (32) /// character long `String`. - pub fn to_base16(&self) -> String { + pub fn to_base16(&self) -> Result { to_base16(&self.to_raw(), &mut [0u8; SpotifyId::SIZE_BASE16]) } @@ -144,7 +145,7 @@ impl SpotifyId { /// character long `String`. /// /// [canonically]: https://developer.spotify.com/documentation/web-api/#spotify-uris-and-ids - pub fn to_base62(&self) -> String { + pub fn to_base62(&self) -> Result { let mut dst = [0u8; 22]; let mut i = 0; let n = self.id; @@ -182,10 +183,7 @@ impl SpotifyId { dst.reverse(); - unsafe { - // Safety: We are only dealing with ASCII characters. - String::from_utf8_unchecked(dst.to_vec()) - } + String::from_utf8(dst.to_vec()) } /// Returns a copy of the `SpotifyId` as an array of `SpotifyId::SIZE` (16) bytes in @@ -202,7 +200,7 @@ impl SpotifyId { /// be encoded as `unknown`. /// /// [Spotify URI]: https://developer.spotify.com/documentation/web-api/#spotify-uris-and-ids - pub fn to_uri(&self) -> String { + pub fn to_uri(&self) -> Result { // 8 chars for the "spotify:" prefix + 1 colon + 22 chars base62 encoded ID = 31 // + unknown size audio_type. let audio_type: &str = self.audio_type.into(); @@ -210,9 +208,10 @@ impl SpotifyId { dst.push_str("spotify:"); dst.push_str(audio_type); dst.push(':'); - dst.push_str(&self.to_base62()); + let base62 = self.to_base62()?; + dst.push_str(&base62); - dst + Ok(dst) } } @@ -220,7 +219,7 @@ impl SpotifyId { pub struct FileId(pub [u8; 20]); impl FileId { - pub fn to_base16(&self) -> String { + pub fn to_base16(&self) -> Result { to_base16(&self.0, &mut [0u8; 40]) } } @@ -233,12 +232,12 @@ impl fmt::Debug for FileId { impl fmt::Display for FileId { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - f.write_str(&self.to_base16()) + f.write_str(&self.to_base16().unwrap_or_default()) } } #[inline] -fn to_base16(src: &[u8], buf: &mut [u8]) -> String { +fn to_base16(src: &[u8], buf: &mut [u8]) -> Result { let mut i = 0; for v in src { buf[i] = BASE16_DIGITS[(v >> 4) as usize]; @@ -246,10 +245,7 @@ fn to_base16(src: &[u8], buf: &mut [u8]) -> String { i += 2; } - unsafe { - // Safety: We are only dealing with ASCII characters. - String::from_utf8_unchecked(buf.to_vec()) - } + String::from_utf8(buf.to_vec()) } #[cfg(test)] @@ -366,7 +362,7 @@ mod tests { audio_type: c.kind, }; - assert_eq!(id.to_base62(), c.base62); + assert_eq!(id.to_base62().unwrap(), c.base62); } } @@ -389,7 +385,7 @@ mod tests { audio_type: c.kind, }; - assert_eq!(id.to_base16(), c.base16); + assert_eq!(id.to_base16().unwrap(), c.base16); } } @@ -415,7 +411,7 @@ mod tests { audio_type: c.kind, }; - assert_eq!(id.to_uri(), c.uri); + assert_eq!(id.to_uri().unwrap(), c.uri); } } diff --git a/metadata/src/lib.rs b/metadata/src/lib.rs index 2ed9273e..435633ad 100644 --- a/metadata/src/lib.rs +++ b/metadata/src/lib.rs @@ -7,12 +7,12 @@ extern crate log; extern crate async_trait; pub mod cover; - use std::collections::HashMap; +use std::string::FromUtf8Error; use librespot_core::mercury::MercuryError; use librespot_core::session::Session; -use librespot_core::spotify_id::{FileId, SpotifyAudioType, SpotifyId}; +use librespot_core::spotify_id::{FileId, SpotifyAudioType, SpotifyId, SpotifyIdError}; use librespot_protocol as protocol; use protobuf::Message; @@ -83,33 +83,48 @@ trait AudioFiles { #[async_trait] impl AudioFiles for Track { async fn get_audio_item(session: &Session, id: SpotifyId) -> Result { - let item = Self::get(session, id).await?; - Ok(AudioItem { - id, - uri: format!("spotify:track:{}", id.to_base62()), - files: item.files, - name: item.name, - duration: item.duration, - available: item.available, - alternatives: Some(item.alternatives), - }) + match id.to_base62() { + Err(e) => { + warn!("Invalid Track SpotifyId: {}", e); + Err(MercuryError) + } + Ok(uri) => { + let item = Self::get(session, id).await?; + Ok(AudioItem { + id, + uri: format!("spotify:track:{}", uri), + files: item.files, + name: item.name, + duration: item.duration, + available: item.available, + alternatives: Some(item.alternatives), + }) + } + } } } #[async_trait] impl AudioFiles for Episode { async fn get_audio_item(session: &Session, id: SpotifyId) -> Result { - let item = Self::get(session, id).await?; - - Ok(AudioItem { - id, - uri: format!("spotify:episode:{}", id.to_base62()), - files: item.files, - name: item.name, - duration: item.duration, - available: item.available, - alternatives: None, - }) + match id.to_base62() { + Err(e) => { + warn!("Invalid Episode SpotifyId: {}", e); + Err(MercuryError) + } + Ok(uri) => { + let item = Self::get(session, id).await?; + Ok(AudioItem { + id, + uri: format!("spotify:episode:{}", uri), + files: item.files, + name: item.name, + duration: item.duration, + available: item.available, + alternatives: None, + }) + } + } } } @@ -117,16 +132,38 @@ impl AudioFiles for Episode { pub trait Metadata: Send + Sized + 'static { type Message: protobuf::Message; - fn request_url(id: SpotifyId) -> String; - fn parse(msg: &Self::Message, session: &Session) -> Self; + fn request_url(id: SpotifyId) -> Result; + fn parse(msg: &Self::Message, session: &Session) -> Result; async fn get(session: &Session, id: SpotifyId) -> Result { - let uri = Self::request_url(id); - let response = session.mercury().get(uri).await?; - let data = response.payload.first().expect("Empty payload"); - let msg = Self::Message::parse_from_bytes(data).unwrap(); - - Ok(Self::parse(&msg, session)) + match Self::request_url(id) { + Err(e) => { + warn!("Invalid SpotifyId: {}", e); + Err(MercuryError) + } + Ok(uri) => { + let response = session.mercury().get(uri).await?; + match response.payload.first() { + None => { + warn!("Empty payload"); + Err(MercuryError) + } + Some(data) => match Self::Message::parse_from_bytes(data) { + Err(e) => { + warn!("Error parsing message from bytes: {}", e); + Err(MercuryError) + } + Ok(msg) => match Self::parse(&msg, session) { + Err(e) => { + warn!("Error parsing message: {:?}", e); + Err(MercuryError) + } + Ok(parsed_msg) => Ok(parsed_msg), + }, + }, + } + } + } } } @@ -192,101 +229,125 @@ pub struct Artist { impl Metadata for Track { type Message = protocol::metadata::Track; - fn request_url(id: SpotifyId) -> String { - format!("hm://metadata/3/track/{}", id.to_base16()) + fn request_url(id: SpotifyId) -> Result { + let id = id.to_base16()?; + Ok(format!("hm://metadata/3/track/{}", id)) } - fn parse(msg: &Self::Message, session: &Session) -> Self { + fn parse(msg: &Self::Message, session: &Session) -> Result { let country = session.country(); let artists = msg .get_artist() .iter() - .filter(|artist| artist.has_gid()) - .map(|artist| SpotifyId::from_raw(artist.get_gid()).unwrap()) - .collect::>(); + .filter_map(|artist| { + if artist.has_gid() { + SpotifyId::from_raw(artist.get_gid()).ok() + } else { + None + } + }) + .collect(); let files = msg .get_file() .iter() - .filter(|file| file.has_file_id()) - .map(|file| { - let mut dst = [0u8; 20]; - dst.clone_from_slice(file.get_file_id()); - (file.get_format(), FileId(dst)) + .filter_map(|file| { + if file.has_file_id() { + let mut dst = [0u8; 20]; + dst.clone_from_slice(file.get_file_id()); + Some((file.get_format(), FileId(dst))) + } else { + None + } }) .collect(); - Track { - id: SpotifyId::from_raw(msg.get_gid()).unwrap(), + Ok(Track { + id: SpotifyId::from_raw(msg.get_gid())?, name: msg.get_name().to_owned(), duration: msg.get_duration(), - album: SpotifyId::from_raw(msg.get_album().get_gid()).unwrap(), + album: SpotifyId::from_raw(msg.get_album().get_gid())?, artists, files, alternatives: msg .get_alternative() .iter() - .map(|alt| SpotifyId::from_raw(alt.get_gid()).unwrap()) + .filter_map(|alt| SpotifyId::from_raw(alt.get_gid()).ok()) .collect(), available: parse_restrictions(msg.get_restriction(), &country, "premium"), - } + }) } } impl Metadata for Album { type Message = protocol::metadata::Album; - fn request_url(id: SpotifyId) -> String { - format!("hm://metadata/3/album/{}", id.to_base16()) + fn request_url(id: SpotifyId) -> Result { + let id = id.to_base16()?; + Ok(format!("hm://metadata/3/album/{}", id)) } - fn parse(msg: &Self::Message, _: &Session) -> Self { + fn parse(msg: &Self::Message, _: &Session) -> Result { let artists = msg .get_artist() .iter() - .filter(|artist| artist.has_gid()) - .map(|artist| SpotifyId::from_raw(artist.get_gid()).unwrap()) - .collect::>(); + .filter_map(|artist| { + if artist.has_gid() { + SpotifyId::from_raw(artist.get_gid()).ok() + } else { + None + } + }) + .collect(); let tracks = msg .get_disc() .iter() .flat_map(|disc| disc.get_track()) - .filter(|track| track.has_gid()) - .map(|track| SpotifyId::from_raw(track.get_gid()).unwrap()) - .collect::>(); + .filter_map(|track| { + if track.has_gid() { + SpotifyId::from_raw(track.get_gid()).ok() + } else { + None + } + }) + .collect(); let covers = msg .get_cover_group() .get_image() .iter() - .filter(|image| image.has_file_id()) - .map(|image| { - let mut dst = [0u8; 20]; - dst.clone_from_slice(image.get_file_id()); - FileId(dst) + .filter_map(|image| { + if image.has_file_id() { + let mut dst = [0u8; 20]; + dst.clone_from_slice(image.get_file_id()); + Some(FileId(dst)) + } else { + None + } }) - .collect::>(); + .collect(); - Album { - id: SpotifyId::from_raw(msg.get_gid()).unwrap(), + Ok(Album { + id: SpotifyId::from_raw(msg.get_gid())?, name: msg.get_name().to_owned(), artists, tracks, covers, - } + }) } } impl Metadata for Playlist { type Message = protocol::playlist4changes::SelectedListContent; - fn request_url(id: SpotifyId) -> String { - format!("hm://playlist/v2/playlist/{}", id.to_base62()) + fn request_url(id: SpotifyId) -> Result { + let id = id.to_base62()?; + Ok(format!("hm://playlist/v2/playlist/{}", id)) } - fn parse(msg: &Self::Message, _: &Session) -> Self { + fn parse(msg: &Self::Message, _: &Session) -> Result { let tracks = msg .get_contents() .get_items() @@ -306,23 +367,24 @@ impl Metadata for Playlist { ); } - Playlist { + Ok(Playlist { revision: msg.get_revision().to_vec(), name: msg.get_attributes().get_name().to_owned(), tracks, user: msg.get_owner_username().to_string(), - } + }) } } impl Metadata for Artist { type Message = protocol::metadata::Artist; - fn request_url(id: SpotifyId) -> String { - format!("hm://metadata/3/artist/{}", id.to_base16()) + fn request_url(id: SpotifyId) -> Result { + let id = id.to_base16()?; + Ok(format!("hm://metadata/3/artist/{}", id)) } - fn parse(msg: &Self::Message, session: &Session) -> Self { + fn parse(msg: &Self::Message, session: &Session) -> Result { let country = session.country(); let top_tracks: Vec = match msg @@ -333,17 +395,22 @@ impl Metadata for Artist { Some(tracks) => tracks .get_track() .iter() - .filter(|track| track.has_gid()) - .map(|track| SpotifyId::from_raw(track.get_gid()).unwrap()) - .collect::>(), + .filter_map(|track| { + if track.has_gid() { + SpotifyId::from_raw(track.get_gid()).ok() + } else { + None + } + }) + .collect(), None => Vec::new(), }; - Artist { - id: SpotifyId::from_raw(msg.get_gid()).unwrap(), + Ok(Artist { + id: SpotifyId::from_raw(msg.get_gid())?, name: msg.get_name().to_owned(), top_tracks, - } + }) } } @@ -351,11 +418,12 @@ impl Metadata for Artist { impl Metadata for Episode { type Message = protocol::metadata::Episode; - fn request_url(id: SpotifyId) -> String { - format!("hm://metadata/3/episode/{}", id.to_base16()) + fn request_url(id: SpotifyId) -> Result { + let id = id.to_base16()?; + Ok(format!("hm://metadata/3/episode/{}", id)) } - fn parse(msg: &Self::Message, session: &Session) -> Self { + fn parse(msg: &Self::Message, session: &Session) -> Result { let country = session.country(); let files = msg @@ -379,9 +447,9 @@ impl Metadata for Episode { dst.clone_from_slice(image.get_file_id()); FileId(dst) }) - .collect::>(); + .collect(); - Episode { + Ok(Episode { id: SpotifyId::from_raw(msg.get_gid()).unwrap(), name: msg.get_name().to_owned(), external_url: msg.get_external_url().to_owned(), @@ -392,24 +460,30 @@ impl Metadata for Episode { files, available: parse_restrictions(msg.get_restriction(), &country, "premium"), explicit: msg.get_explicit().to_owned(), - } + }) } } impl Metadata for Show { type Message = protocol::metadata::Show; - fn request_url(id: SpotifyId) -> String { - format!("hm://metadata/3/show/{}", id.to_base16()) + fn request_url(id: SpotifyId) -> Result { + let id = id.to_base16()?; + Ok(format!("hm://metadata/3/show/{}", id)) } - fn parse(msg: &Self::Message, _: &Session) -> Self { + fn parse(msg: &Self::Message, _: &Session) -> Result { let episodes = msg .get_episode() .iter() - .filter(|episode| episode.has_gid()) - .map(|episode| SpotifyId::from_raw(episode.get_gid()).unwrap()) - .collect::>(); + .filter_map(|episode| { + if episode.has_gid() { + SpotifyId::from_raw(episode.get_gid()).ok() + } else { + None + } + }) + .collect(); let covers = msg .get_covers() @@ -421,15 +495,15 @@ impl Metadata for Show { dst.clone_from_slice(image.get_file_id()); FileId(dst) }) - .collect::>(); + .collect(); - Show { + Ok(Show { id: SpotifyId::from_raw(msg.get_gid()).unwrap(), name: msg.get_name().to_owned(), publisher: msg.get_publisher().to_owned(), episodes, covers, - } + }) } } diff --git a/playback/src/player.rs b/playback/src/player.rs index b5603491..b2bdea0c 100644 --- a/playback/src/player.rs +++ b/playback/src/player.rs @@ -740,7 +740,10 @@ impl PlayerTrackLoader { let audio = match self.find_available_alternative(audio).await { Some(audio) => audio, None => { - warn!("<{}> is not available", spotify_id.to_uri()); + warn!( + "<{}> is not available", + spotify_id.to_uri().unwrap_or_default() + ); return None; } }; @@ -748,7 +751,7 @@ impl PlayerTrackLoader { if audio.duration < 0 { error!( "Track duration for <{}> cannot be {}", - spotify_id.to_uri(), + spotify_id.to_uri().unwrap_or_default(), audio.duration ); return None; diff --git a/src/player_event_handler.rs b/src/player_event_handler.rs index 4c75128c..785290ed 100644 --- a/src/player_event_handler.rs +++ b/src/player_event_handler.rs @@ -5,6 +5,7 @@ use tokio::process::{Child as AsyncChild, Command as AsyncCommand}; use std::collections::HashMap; use std::io; +use std::io::{Error, ErrorKind}; use std::process::{Command, ExitStatus}; pub fn run_program_on_events(event: PlayerEvent, onevent: &str) -> Option> { @@ -13,45 +14,110 @@ pub fn run_program_on_events(event: PlayerEvent, onevent: &str) -> Option { - env_vars.insert("PLAYER_EVENT", "changed".to_string()); - env_vars.insert("OLD_TRACK_ID", old_track_id.to_base62()); - env_vars.insert("TRACK_ID", new_track_id.to_base62()); - } - PlayerEvent::Started { track_id, .. } => { - env_vars.insert("PLAYER_EVENT", "started".to_string()); - env_vars.insert("TRACK_ID", track_id.to_base62()); - } - PlayerEvent::Stopped { track_id, .. } => { - env_vars.insert("PLAYER_EVENT", "stopped".to_string()); - env_vars.insert("TRACK_ID", track_id.to_base62()); - } + } => match old_track_id.to_base62() { + Err(e) => { + return Some(Err(Error::new( + ErrorKind::InvalidData, + format!( + "PlayerEvent::Changed: Invalid old track id: {}", + e.utf8_error() + ), + ))) + } + Ok(old_id) => match new_track_id.to_base62() { + Err(e) => { + return Some(Err(Error::new( + ErrorKind::InvalidData, + format!( + "PlayerEvent::Changed: Invalid new track id: {}", + e.utf8_error() + ), + ))) + } + Ok(new_id) => { + env_vars.insert("PLAYER_EVENT", "changed".to_string()); + env_vars.insert("OLD_TRACK_ID", old_id); + env_vars.insert("TRACK_ID", new_id); + } + }, + }, + PlayerEvent::Started { track_id, .. } => match track_id.to_base62() { + Err(e) => { + return Some(Err(Error::new( + ErrorKind::InvalidData, + format!("PlayerEvent::Started: Invalid track id: {}", e.utf8_error()), + ))) + } + Ok(id) => { + env_vars.insert("PLAYER_EVENT", "started".to_string()); + env_vars.insert("TRACK_ID", id); + } + }, + PlayerEvent::Stopped { track_id, .. } => match track_id.to_base62() { + Err(e) => { + return Some(Err(Error::new( + ErrorKind::InvalidData, + format!("PlayerEvent::Stopped: Invalid track id: {}", e.utf8_error()), + ))) + } + Ok(id) => { + env_vars.insert("PLAYER_EVENT", "stopped".to_string()); + env_vars.insert("TRACK_ID", id); + } + }, PlayerEvent::Playing { track_id, duration_ms, position_ms, .. - } => { - env_vars.insert("PLAYER_EVENT", "playing".to_string()); - env_vars.insert("TRACK_ID", track_id.to_base62()); - env_vars.insert("DURATION_MS", duration_ms.to_string()); - env_vars.insert("POSITION_MS", position_ms.to_string()); - } + } => match track_id.to_base62() { + Err(e) => { + return Some(Err(Error::new( + ErrorKind::InvalidData, + format!("PlayerEvent::Playing: Invalid track id: {}", e.utf8_error()), + ))) + } + Ok(id) => { + env_vars.insert("PLAYER_EVENT", "playing".to_string()); + env_vars.insert("TRACK_ID", id); + env_vars.insert("DURATION_MS", duration_ms.to_string()); + env_vars.insert("POSITION_MS", position_ms.to_string()); + } + }, PlayerEvent::Paused { track_id, duration_ms, position_ms, .. - } => { - env_vars.insert("PLAYER_EVENT", "paused".to_string()); - env_vars.insert("TRACK_ID", track_id.to_base62()); - env_vars.insert("DURATION_MS", duration_ms.to_string()); - env_vars.insert("POSITION_MS", position_ms.to_string()); - } - PlayerEvent::Preloading { track_id, .. } => { - env_vars.insert("PLAYER_EVENT", "preloading".to_string()); - env_vars.insert("TRACK_ID", track_id.to_base62()); - } + } => match track_id.to_base62() { + Err(e) => { + return Some(Err(Error::new( + ErrorKind::InvalidData, + format!("PlayerEvent::Paused: Invalid track id: {}", e.utf8_error()), + ))) + } + Ok(id) => { + env_vars.insert("PLAYER_EVENT", "paused".to_string()); + env_vars.insert("TRACK_ID", id); + env_vars.insert("DURATION_MS", duration_ms.to_string()); + env_vars.insert("POSITION_MS", position_ms.to_string()); + } + }, + PlayerEvent::Preloading { track_id, .. } => match track_id.to_base62() { + Err(e) => { + return Some(Err(Error::new( + ErrorKind::InvalidData, + format!( + "PlayerEvent::Preloading: Invalid track id: {}", + e.utf8_error() + ), + ))) + } + Ok(id) => { + env_vars.insert("PLAYER_EVENT", "preloading".to_string()); + env_vars.insert("TRACK_ID", id); + } + }, PlayerEvent::VolumeSet { volume } => { env_vars.insert("PLAYER_EVENT", "volume_set".to_string()); env_vars.insert("VOLUME", volume.to_string());