From 5a9b139a7ffcef69a49fecc22748ebb8ce00c7b0 Mon Sep 17 00:00:00 2001 From: Paul Lietar Date: Fri, 5 Feb 2016 21:11:55 +0000 Subject: [PATCH] Intern returned C strings to avoid leaking them. --- capi/Cargo.toml | 1 - capi/src/artist.rs | 16 ++--- capi/src/cstring_cache.rs | 21 +++++++ capi/src/lib.rs | 9 +-- capi/src/link.rs | 6 +- capi/src/metadata.rs | 85 ++++++++++++++++++--------- capi/src/session.rs | 120 ++++++++++++++++++++++++++++---------- capi/src/track.rs | 26 +++------ capi/src/types.rs | 110 +++++++++++++++++++++++++++++----- 9 files changed, 282 insertions(+), 112 deletions(-) create mode 100644 capi/src/cstring_cache.rs diff --git a/capi/Cargo.toml b/capi/Cargo.toml index 6782ef2d..ad8385c4 100644 --- a/capi/Cargo.toml +++ b/capi/Cargo.toml @@ -10,7 +10,6 @@ crate-type = ["staticlib"] [dependencies] libc = "0.2" eventual = "~0.1.5" -owning_ref = "0.1.*" [dependencies.librespot] path = "../" diff --git a/capi/src/artist.rs b/capi/src/artist.rs index 5bc5b42a..c5a458c9 100644 --- a/capi/src/artist.rs +++ b/capi/src/artist.rs @@ -1,6 +1,4 @@ use libc::c_char; -use std::ffi::CString; -use std::mem; use librespot::metadata::Artist; @@ -17,17 +15,11 @@ pub unsafe extern "C" fn sp_artist_is_loaded(c_artist: *mut sp_artist) -> bool { #[no_mangle] pub unsafe extern "C" fn sp_artist_name(c_artist: *mut sp_artist) -> *const c_char { - let artist = &*c_artist; + let artist = &mut *c_artist; let name = artist.get() - .map(|metadata| metadata.name.clone()) - .unwrap_or("".to_owned()); + .map(|metadata| &metadata.name as &str) + .unwrap_or(""); - let name = CString::new(name).unwrap(); - let c_name = name.as_ptr(); - - // FIXME - mem::forget(name); - - c_name + artist.intern(name).as_ptr() } diff --git a/capi/src/cstring_cache.rs b/capi/src/cstring_cache.rs new file mode 100644 index 00000000..4999c0c3 --- /dev/null +++ b/capi/src/cstring_cache.rs @@ -0,0 +1,21 @@ +use std::collections::HashMap; +use std::ffi::{CString, CStr}; + +pub struct CStringCache { + cache: HashMap +} + +impl CStringCache { + pub fn new() -> CStringCache { + CStringCache { + cache: HashMap::new() + } + } + + pub fn intern(&mut self, string: &str) -> &CStr { + self.cache.entry(string.to_owned()).or_insert_with(|| { + CString::new(string).unwrap() + }) + } +} + diff --git a/capi/src/lib.rs b/capi/src/lib.rs index 5b600fe0..e31176d6 100644 --- a/capi/src/lib.rs +++ b/capi/src/lib.rs @@ -1,7 +1,8 @@ +#![feature(fnbox)] + extern crate librespot; extern crate libc; extern crate eventual; -extern crate owning_ref; pub mod artist; pub mod link; @@ -9,9 +10,5 @@ pub mod metadata; pub mod session; pub mod track; mod types; - -pub use types::sp_session_config; -pub use types::sp_error; -pub use types::sp_error::*; - +mod cstring_cache; diff --git a/capi/src/link.rs b/capi/src/link.rs index a57fda07..57a3904f 100644 --- a/capi/src/link.rs +++ b/capi/src/link.rs @@ -1,5 +1,5 @@ use metadata::SpMetadata; -use session::global_session; +use session::SpSession; use track::sp_track; use types::sp_error; use types::sp_error::*; @@ -29,8 +29,8 @@ pub unsafe extern "C" fn sp_link_release(c_link: *mut sp_link) -> sp_error { #[no_mangle] pub unsafe extern "C" fn sp_link_as_track(c_link: *mut sp_link) -> *mut sp_track { let link = &*c_link; - let session = &*global_session.unwrap(); + let session = SpSession::global(); - let track = SpMetadata::from_future(link.as_track(session).unwrap()); + let track = SpMetadata::from_future(link.as_track(&session.session).unwrap()); Box::into_raw(Box::new(track)) } diff --git a/capi/src/metadata.rs b/capi/src/metadata.rs index 60175232..ef5a0c65 100644 --- a/capi/src/metadata.rs +++ b/capi/src/metadata.rs @@ -1,54 +1,87 @@ use eventual::Async; -use owning_ref::MutexGuardRef; -use std::sync::{Mutex, Arc}; +use std::sync::Arc; +use std::ffi::CStr; +use std::cell::UnsafeCell; use librespot::metadata::{MetadataTrait, MetadataRef}; -pub enum SpMetadataInner { +use cstring_cache::CStringCache; + +use session::SpSession; + +pub struct UnsafeSyncCell { + cell: UnsafeCell +} + +impl UnsafeSyncCell { + fn new(value: T) -> UnsafeSyncCell { + UnsafeSyncCell { cell: UnsafeCell::new(value) } + } + + fn get(&self) -> *mut T { + self.cell.get() + } +} + +unsafe impl Sync for UnsafeSyncCell {} + +pub enum SpMetadataState { Loading, Error, Loaded(T), } -pub struct SpMetadata(Arc>>); +pub struct SpMetadata { + state: Arc>>, + cache: CStringCache, +} impl SpMetadata { pub fn from_future(future: MetadataRef) -> SpMetadata { - let metadata = Arc::new(Mutex::new(SpMetadataInner::Loading)); + let state = Arc::new(UnsafeSyncCell::new(SpMetadataState::Loading)); { - let metadata = metadata.clone(); - future.receive(move |result| { - //let metadata = metadata.upgrade().unwrap(); - let mut metadata = metadata.lock().unwrap(); - - *metadata = match result { - Ok(data) => SpMetadataInner::Loaded(data), - Err(_) => SpMetadataInner::Error, + let state = state.clone(); + SpSession::receive(future, move |session, result| { + let state = unsafe { + &mut *state.get() }; + + *state = match result { + Ok(data) => SpMetadataState::Loaded(data), + Err(_) => SpMetadataState::Error, + }; + + unsafe { + if let Some(f) = session.callbacks.metadata_updated { + f(session as *mut _) + } + } }); } - SpMetadata(metadata) + SpMetadata { + state: state, + cache: CStringCache::new(), + } } pub fn is_loaded(&self) -> bool { - self.get().is_some() + unsafe { + self.get().is_some() + } } - pub fn get(&self) -> Option, T>> { - let inner = self.0.lock().unwrap(); + pub unsafe fn get(&self) -> Option<&'static T> { + let state = &*self.state.get(); - match *inner { - SpMetadataInner::Loaded(_) => { - Some(MutexGuardRef::new(inner).map(|inner| { - match *inner { - SpMetadataInner::Loaded(ref metadata) => metadata, - _ => unreachable!(), - } - })) - } + match *state { + SpMetadataState::Loaded(ref metadata) => Some(metadata), _ => None, } } + + pub fn intern(&mut self, string: &str) -> &CStr { + self.cache.intern(string) + } } diff --git a/capi/src/session.rs b/capi/src/session.rs index 3381c931..35731a11 100644 --- a/capi/src/session.rs +++ b/capi/src/session.rs @@ -1,23 +1,63 @@ use libc::{c_int, c_char}; -use std::ffi::{CStr, CString}; -use std::mem; +use std::ffi::CStr; use std::slice::from_raw_parts; +use std::sync::mpsc; +use std::boxed::FnBox; +use std::sync::Mutex; use librespot::session::{Session, Config, Bitrate}; +use eventual::{Async, AsyncResult, Future}; +use cstring_cache::CStringCache; use types::sp_error; use types::sp_error::*; use types::sp_session_config; +use types::sp_session_callbacks; -pub static mut global_session: Option<*mut Session> = None; +static mut global_session: Option<(*const sp_session, *const Mutex>)> = None; + +pub type SpSessionEvent = Box ()>; + +pub struct SpSession { + pub session: Session, + cache: CStringCache, + rx: mpsc::Receiver, + + pub callbacks: &'static sp_session_callbacks, +} + +impl SpSession { + pub unsafe fn global() -> &'static SpSession { + &*global_session.unwrap().0 + } + + pub fn run () + 'static>(event: F) { + let tx = unsafe { + &*global_session.unwrap().1 + }; + + tx.lock().unwrap().send(Box::new(event)).unwrap(); + } + + pub fn receive(future: Future, handler: F) + where T : Send, E: Send, + F : FnOnce(&mut SpSession, AsyncResult) -> () + Send + 'static { + + future.receive(move |result| { + SpSession::run(move |session| { + handler(session, result); + }) + }) + } +} #[allow(non_camel_case_types)] -pub type sp_session = Session; +pub type sp_session = SpSession; #[no_mangle] pub unsafe extern "C" fn sp_session_create(c_config: *const sp_session_config, c_session: *mut *mut sp_session) -> sp_error { - assert_eq!(global_session, None); + assert!(global_session.is_none()); let c_config = &*c_config; @@ -36,10 +76,20 @@ pub unsafe extern "C" fn sp_session_create(c_config: *const sp_session_config, bitrate: Bitrate::Bitrate160, }; - let session = Box::new(Session::new(config)); - let session = Box::into_raw(session); + let (tx, rx) = mpsc::channel(); + + let session = SpSession { + session: Session::new(config), + cache: CStringCache::new(), + rx: rx, + callbacks: &*c_config.callbacks, + }; + + let session = Box::into_raw(Box::new(session)); + let tx = Box::into_raw(Box::new(Mutex::new(tx))); + + global_session = Some((session, tx)); - global_session = Some(session); *c_session = session; SP_ERROR_OK @@ -47,8 +97,6 @@ pub unsafe extern "C" fn sp_session_create(c_config: *const sp_session_config, #[no_mangle] pub unsafe extern "C" fn sp_session_release(c_session: *mut sp_session) -> sp_error { - assert_eq!(global_session, Some(c_session)); - global_session = None; drop(Box::from_raw(c_session)); @@ -61,20 +109,25 @@ pub unsafe extern "C" fn sp_session_login(c_session: *mut sp_session, c_password: *const c_char, _remember_me: bool, _blob: *const c_char) -> sp_error { - assert_eq!(global_session, Some(c_session)); - let session = &*c_session; let username = CStr::from_ptr(c_username).to_string_lossy().into_owned(); let password = CStr::from_ptr(c_password).to_string_lossy().into_owned(); - session.login_password(username, password).unwrap(); - { - let session = session.clone(); - ::std::thread::spawn(move || { - loop { - session.poll(); + let session = session.session.clone(); + SpSession::receive(Future::spawn(move || { + session.login_password(username, password) + }), |session, result| { + result.unwrap(); + + { + let session = session.session.clone(); + ::std::thread::spawn(move || { + loop { + session.poll(); + } + }); } }); } @@ -84,27 +137,32 @@ pub unsafe extern "C" fn sp_session_login(c_session: *mut sp_session, #[no_mangle] pub unsafe extern "C" fn sp_session_user_name(c_session: *mut sp_session) -> *const c_char { - assert_eq!(global_session, Some(c_session)); + let session = &mut *c_session; - let session = &*c_session; - - let username = CString::new(session.username()).unwrap(); - let c_username = username.as_ptr(); - - // FIXME - mem::forget(username); - - c_username + let username = session.session.username(); + session.cache.intern(&username).as_ptr() } #[no_mangle] pub unsafe extern "C" fn sp_session_user_country(c_session: *mut sp_session) -> c_int { - assert_eq!(global_session, Some(c_session)); - let session = &*c_session; - let country = session.username(); + let country = session.session.country(); country.chars().fold(0, |acc, x| { acc << 8 | (x as u32) }) as c_int } + +#[no_mangle] +pub unsafe extern "C" fn sp_session_process_events(c_session: *mut sp_session, next_timeout: *mut c_int) -> sp_error { + let session = &mut *c_session; + + if !next_timeout.is_null() { + *next_timeout = 10; + } + + let event = session.rx.recv().unwrap(); + event.call_box((session,)); + + SP_ERROR_OK +} diff --git a/capi/src/track.rs b/capi/src/track.rs index 2910419c..b5280d2e 100644 --- a/capi/src/track.rs +++ b/capi/src/track.rs @@ -1,11 +1,9 @@ use libc::{c_int, c_char}; -use std::ffi::CString; -use std::mem; use std::ptr::null_mut; use artist::sp_artist; use metadata::SpMetadata; -use session::global_session; +use session::SpSession; use librespot::metadata::{Track, Artist}; @@ -20,19 +18,13 @@ pub unsafe extern "C" fn sp_track_is_loaded(c_track: *mut sp_track) -> bool { #[no_mangle] pub unsafe extern "C" fn sp_track_name(c_track: *mut sp_track) -> *const c_char { - let track = &*c_track; + let track = &mut *c_track; let name = track.get() - .map(|metadata| metadata.name.clone()) - .unwrap_or("".to_owned()); + .map(|metadata| &metadata.name as &str) + .unwrap_or(""); - let name = CString::new(name).unwrap(); - let c_name = name.as_ptr(); - - // FIXME - mem::forget(name); - - c_name + track.intern(name).as_ptr() } #[no_mangle] @@ -47,14 +39,12 @@ pub unsafe extern "C" fn sp_track_num_artists(c_track: *mut sp_track) -> c_int { #[no_mangle] pub unsafe extern "C" fn sp_track_artist(c_track: *mut sp_track, index: c_int) -> *mut sp_artist { let track = &*c_track; - let session = &*global_session.unwrap(); + let session = SpSession::global(); track.get() .and_then(|metadata| metadata.artists.get(index as usize).map(|x| *x)) - .map(|artist_id| { - let artist = SpMetadata::from_future(session.metadata::(artist_id)); - Box::into_raw(Box::new(artist)) - }) + .map(|artist_id| session.session.metadata::(artist_id)) + .map(|artist| Box::into_raw(Box::new(SpMetadata::from_future(artist)))) .unwrap_or(null_mut()) } diff --git a/capi/src/types.rs b/capi/src/types.rs index 6387000f..c3926772 100644 --- a/capi/src/types.rs +++ b/capi/src/types.rs @@ -1,8 +1,7 @@ -#![allow(non_camel_case_types)] +#![allow(non_camel_case_types, dead_code)] -use libc::size_t; - -pub enum sp_session_callbacks {} +use libc::{size_t, c_int, c_char, c_void}; +use session::sp_session; #[derive(Clone, Copy)] #[repr(u32)] @@ -48,21 +47,102 @@ pub enum sp_error { #[repr(C)] #[derive(Copy,Clone)] pub struct sp_session_config { - pub api_version: ::std::os::raw::c_int, - pub cache_location: *const ::std::os::raw::c_char, - pub settings_location: *const ::std::os::raw::c_char, - pub application_key: *const ::std::os::raw::c_void, + pub api_version: c_int, + pub cache_location: *const c_char, + pub settings_location: *const c_char, + pub application_key: *const c_void, pub application_key_size: size_t, - pub user_agent: *const ::std::os::raw::c_char, + pub user_agent: *const c_char, pub callbacks: *const sp_session_callbacks, - pub userdata: *mut ::std::os::raw::c_void, + pub userdata: *mut c_void, pub compress_playlists: bool, pub dont_save_metadata_for_playlists: bool, pub initially_unload_playlists: bool, - pub device_id: *const ::std::os::raw::c_char, - pub proxy: *const ::std::os::raw::c_char, - pub proxy_username: *const ::std::os::raw::c_char, - pub proxy_password: *const ::std::os::raw::c_char, - pub tracefile: *const ::std::os::raw::c_char, + pub device_id: *const c_char, + pub proxy: *const c_char, + pub proxy_username: *const c_char, + pub proxy_password: *const c_char, + pub tracefile: *const c_char, } +#[repr(C)] +#[derive(Clone, Copy)] +pub struct sp_session_callbacks { + pub logged_in: Option, + + pub logged_out: Option, + + pub metadata_updated: Option, + + pub connection_error: Option, + + pub message_to_user: Option, + + pub notify_main_thread: Option, + + pub music_delivery: Option c_int>, + + pub play_token_lost: Option, + + pub log_message: Option, + + pub end_of_track: Option, + + pub streaming_error: Option, + + pub userinfo_updated: Option, + + pub start_playback: Option, + + pub stop_playback: Option, + + pub get_audio_buffer_stats: Option, + + pub offline_status_updated: Option, + + pub offline_error: Option, + + pub credentials_blob_updated: Option, + + pub connectionstate_updated: Option, + + pub scrobble_error: Option, + + pub private_session_mode_changed: Option, +} + +#[repr(C)] +#[derive(Clone, Copy)] +pub struct sp_audioformat { + pub sample_type: sp_sampletype, + pub sample_rate: c_int, + pub channels: c_int, +} + +#[derive(Clone, Copy)] +#[repr(u32)] +pub enum sp_sampletype { + SP_SAMPLETYPE_INT16_NATIVE_ENDIAN = 0, + _Dummy // rust #10292 +} + +#[repr(C)] +#[derive(Clone, Copy)] +pub struct sp_audio_buffer_stats { + pub samples: c_int, + pub stutter: c_int, +}