CVE-2018-25001

@@ -1,5 +1,16 @@
 # ??? (??? ??, ????)
 
+**Note: This includes a security fix!**
+
+ * Proplist: Fixed potential use-after-free with `proplist::Iterator` (not to be confused with the
+   `std::iter::Iterator` trait). An instance of this object type is created from a `Proplist` object
+   and holds a copy of the same raw pointer to the underlying C object; the `Proplist` object had
+   sole responsibility for destroying it via its `Drop` implementation. There was no actual lifetime
+   association however linking the lifetime of the `Iterator` object to the `Proplist` object, and
+   thus it was possible for the `Proplist` object to be destroyed first, leaving the `Iterator`
+   object working on a freed C object. This is unlikely to have been done in actual user code, but
+   would have been trivial to achieve, including simply by using the `into_iter()` function. This
+   affects versions all the way back to 1.0.5.
  * Time: Simplified converting `Duration` to `MicroSeconds` or `Timeval` using
    `Duration::subsec_millis()`.
  * Proplist: Made `Iterator::new()` private, since it’s very unlikely anyone needs it

@@ -213,7 +213,7 @@ impl Context {
         // as_ptr() giving dangling pointers!
         let c_name = CString::new(name.clone()).unwrap();
         let ptr = unsafe { capi::pa_context_new_with_proplist(
-            std::mem::transmute(mainloop.inner().get_api()), c_name.as_ptr(), proplist.ptr) };
+            std::mem::transmute(mainloop.inner().get_api()), c_name.as_ptr(), proplist.0.ptr) };
         if ptr.is_null() {
             return None;
         }
@@ -440,12 +440,12 @@ impl Context {
     /// right device.
     ///
     /// Panics if the underlying C function returns a null pointer.
-    pub fn proplist_update<F>(&mut self, mode: ::proplist::UpdateMode, p: &Proplist, callback: F)
+    pub fn proplist_update<F>(&mut self, mode: ::proplist::UpdateMode, pl: &Proplist, callback: F)
         -> Operation<FnMut(bool)>
         where F: FnMut(bool) + 'static
     {
         let cb_data = box_closure_get_capi_ptr::<FnMut(bool)>(Box::new(callback));
-        let ptr = unsafe { capi::pa_context_proplist_update(self.ptr, mode, p.ptr,
+        let ptr = unsafe { capi::pa_context_proplist_update(self.ptr, mode, pl.0.ptr,
             Some(success_cb_proxy), cb_data) };
         assert!(!ptr.is_null());
         Operation::from_raw(ptr, cb_data as *mut Box<FnMut(bool)>)

@@ -175,7 +175,7 @@ impl Context {
             ::callbacks::get_su_capi_params::<_, _>(callback, play_sample_success_cb_proxy);
         let ptr = unsafe {
             capi::pa_context_play_sample_with_proplist(self.ptr, c_name.as_ptr(), p_dev, volume.0,
-                proplist.ptr, cb_fn, cb_data)
+                proplist.0.ptr, cb_fn, cb_data)
         };
         assert!(!ptr.is_null());
         Operation::from_raw(ptr, cb_data as *mut Box<FnMut(Result<u32, ()>)>)

@@ -20,6 +20,7 @@ use capi;
 use std::os::raw::{c_char, c_void};
 use std::ffi::{CStr, CString};
 use std::ptr::{null, null_mut};
+use std::marker::PhantomData;
 use error::PAErr;
 
 pub(crate) use capi::pa_proplist as ProplistInternal;
@@ -136,7 +137,11 @@ pub mod properties {
 
 /// A property list object. Basically a dictionary with ASCII strings as keys and arbitrary data as
 /// values.
-pub struct Proplist {
+pub struct Proplist(pub(crate) ProplistInner);
+
+/// Inner type holding ownership over actual C object, necessary to guard against use-after-free
+/// issues with respect to the related `Iterator` object.
+pub(crate) struct ProplistInner {
     /// The actual C object.
     pub(crate) ptr: *mut ProplistInternal,
     /// Used to avoid freeing the internal object when used as a weak wrapper in callbacks
@@ -151,24 +156,35 @@ impl std::fmt::Debug for Proplist {
 
 /// Proplist iterator, used for iterating over the list’s keys. Returned by the
 /// [`iter`](struct.Proplist.html#method.iter) method.
-pub struct Iterator {
+///
+/// Note, lifetime `'a` is used to tie an instance of this struct to the associated `Proplist`, and
+/// thus prevent a use-after-free issue that would otherwise occur should the `Proplist` be
+/// destroyed first. Conversion from a `Proplist` via `into_iter` is okay though as responsibility
+/// for destruction is transfered to it.
+pub struct Iterator<'a> {
     /// The actual C proplist object.
-    ptr: *const ProplistInternal,
+    pl_ref: ProplistInner,
     /// State tracker, used by underlying C function
     state: *mut c_void,
+    /// Use lifetime `'a`
+    phantom: PhantomData<&'a ProplistInner>,
 }
 
-impl Iterator {
-    fn new(pl: *const ProplistInternal) -> Self {
-        Self { ptr: pl, state: null_mut::<c_void>() }
+impl<'a> Iterator<'a> {
+    fn new(pl: *mut ProplistInternal) -> Self {
+        Self {
+            pl_ref: ProplistInner { ptr: pl, weak: true },
+            state: null_mut::<c_void>(),
+            phantom: PhantomData,
+        }
     }
 }
 
-impl std::iter::Iterator for Iterator {
+impl<'a> std::iter::Iterator for Iterator<'a> {
     type Item = String;
     fn next(&mut self) -> Option<Self::Item> {
         let state_actual = &mut self.state as *mut *mut c_void;
-        let key_ptr = unsafe { capi::pa_proplist_iterate(self.ptr, state_actual) };
+        let key_ptr = unsafe { capi::pa_proplist_iterate(self.pl_ref.ptr, state_actual) };
         if key_ptr.is_null() {
             return None;
         }
@@ -179,10 +195,14 @@ impl std::iter::Iterator for Iterator {
 
 impl IntoIterator for Proplist {
     type Item = String;
-    type IntoIter = Iterator;
-
-    fn into_iter(self) -> Self::IntoIter {
-        self.iter()
+    type IntoIter = Iterator<'static>;
+
+    fn into_iter(mut self) -> Self::IntoIter {
+        let mut iter = Iterator::new(self.0.ptr);
+        // Move responsibility for destruction, if it has it (is not weak itself)
+        iter.pl_ref.weak = self.0.weak;
+        self.0.weak = true;
+        iter
     }
 }
 
@@ -212,15 +232,15 @@ impl Proplist {
     /// pointer.
     pub(crate) fn from_raw(ptr: *mut ProplistInternal) -> Self {
         assert_eq!(false, ptr.is_null());
-        Self { ptr: ptr, weak: false }
+        Proplist(ProplistInner { ptr: ptr, weak: false })
     }
 
     /// Create a new `Proplist` from an existing [`ProplistInternal`](enum.ProplistInternal.html)
     /// pointer. This is the ‘weak’ version, which avoids destroying the internal object when
     /// dropped.
     pub(crate) fn from_raw_weak(ptr: *mut ProplistInternal) -> Self {
         assert_eq!(false, ptr.is_null());
-        Self { ptr: ptr, weak: true }
+        Proplist(ProplistInner { ptr: ptr, weak: true })
     }
 
     /// Returns `true` if the key is valid.
@@ -238,7 +258,7 @@ impl Proplist {
         // as_ptr() giving dangling pointers!
         let c_key = CString::new(key.clone()).unwrap();
         let c_value = CString::new(value.clone()).unwrap();
-        match unsafe { capi::pa_proplist_sets(self.ptr, c_key.as_ptr(), c_value.as_ptr()) } {
+        match unsafe { capi::pa_proplist_sets(self.0.ptr, c_key.as_ptr(), c_value.as_ptr()) } {
             0 => Ok(()),
             _ => Err(()),
         }
@@ -254,7 +274,7 @@ impl Proplist {
         // Warning: New CStrings will be immediately freed if not bound to a variable, leading to
         // as_ptr() giving dangling pointers!
         let c_pair = CString::new(pair.clone()).unwrap();
-        match unsafe { capi::pa_proplist_setp(self.ptr, c_pair.as_ptr()) } {
+        match unsafe { capi::pa_proplist_setp(self.0.ptr, c_pair.as_ptr()) } {
             0 => Ok(()),
             _ => Err(()),
         }
@@ -266,8 +286,8 @@ impl Proplist {
         // Warning: New CStrings will be immediately freed if not bound to a variable, leading to
         //  as_ptr() giving dangling pointers!
         let c_key = CString::new(key.clone()).unwrap();
-        match unsafe { capi::pa_proplist_set(self.ptr, c_key.as_ptr(), data.as_ptr() as *mut c_void,
-            data.len()) }
+        match unsafe { capi::pa_proplist_set(self.0.ptr, c_key.as_ptr(),
+            data.as_ptr() as *mut c_void, data.len()) }
         {
             0 => Ok(()),
             _ => Err(()),
@@ -280,7 +300,7 @@ impl Proplist {
         // Warning: New CStrings will be immediately freed if not bound to a variable, leading to
         // as_ptr() giving dangling pointers!
         let c_key = CString::new(key.clone()).unwrap();
-        let ptr = unsafe { capi::pa_proplist_gets(self.ptr, c_key.as_ptr()) };
+        let ptr = unsafe { capi::pa_proplist_gets(self.0.ptr, c_key.as_ptr()) };
         if ptr.is_null() {
             return None;
         }
@@ -300,7 +320,7 @@ impl Proplist {
         let c_key = CString::new(key.clone()).unwrap();
         let mut data_ptr = null::<c_void>();
         let mut nbytes: usize = 0;
-        if unsafe { capi::pa_proplist_get(self.ptr, c_key.as_ptr(), &mut data_ptr,
+        if unsafe { capi::pa_proplist_get(self.0.ptr, c_key.as_ptr(), &mut data_ptr,
             &mut nbytes) } != 0
         {
             return None;
@@ -313,15 +333,15 @@ impl Proplist {
 
     /// Merge property list “other” into self, adhering to the merge mode specified.
     pub fn merge(&mut self, other: &Self, mode: UpdateMode) {
-        unsafe { capi::pa_proplist_update(self.ptr, mode, other.ptr); }
+        unsafe { capi::pa_proplist_update(self.0.ptr, mode, other.0.ptr); }
     }
 
     /// Removes a single entry from the property list, identified by the specified key name.
     pub fn unset(&mut self, key: &str) -> Result<(), PAErr> {
         // Warning: New CStrings will be immediately freed if not bound to a variable, leading to
         // as_ptr() giving dangling pointers!
         let c_key = CString::new(key.clone()).unwrap();
-        match unsafe { capi::pa_proplist_unset(self.ptr, c_key.as_ptr()) } {
+        match unsafe { capi::pa_proplist_unset(self.0.ptr, c_key.as_ptr()) } {
             0 => Ok(()),
             e => Err(PAErr(e)),
         }
@@ -347,7 +367,7 @@ impl Proplist {
         }
         c_keys_ptrs.push(null());
 
-        match unsafe { capi::pa_proplist_unset_many(self.ptr, c_keys_ptrs.as_ptr()) } {
+        match unsafe { capi::pa_proplist_unset_many(self.0.ptr, c_keys_ptrs.as_ptr()) } {
             r if r < 0 => None,
             r => Some(r as u32),
         }
@@ -373,16 +393,16 @@ impl Proplist {
     /// }
     /// # }
     /// ```
-    pub fn iter(&self) -> Iterator {
-        Iterator::new(self.ptr)
+    pub fn iter(&self) -> Iterator<'_> {
+        Iterator::new(self.0.ptr)
     }
 
     /// Format the property list nicely as a human readable string.
     ///
     /// This works very much like [`to_string_sep`](#method.to_string_sep) and uses a newline as
     /// separator and appends one final one.
     pub fn to_string(&self) -> Option<String> {
-        let ptr = unsafe { capi::pa_proplist_to_string(self.ptr) };
+        let ptr = unsafe { capi::pa_proplist_to_string(self.0.ptr) };
         if ptr.is_null() {
             return None;
         }
@@ -400,7 +420,7 @@ impl Proplist {
         // Warning: New CStrings will be immediately freed if not bound to a variable, leading to
         // as_ptr() giving dangling pointers!
         let c_sep = CString::new(sep.clone()).unwrap();
-        let ptr = unsafe { capi::pa_proplist_to_string_sep(self.ptr, c_sep.as_ptr()) };
+        let ptr = unsafe { capi::pa_proplist_to_string_sep(self.0.ptr, c_sep.as_ptr()) };
         if ptr.is_null() {
             return None;
         }
@@ -419,7 +439,7 @@ impl Proplist {
         // Warning: New CStrings will be immediately freed if not bound to a variable, leading to
         // as_ptr() giving dangling pointers!
         let c_key = CString::new(key.clone()).unwrap();
-        match unsafe { capi::pa_proplist_contains(self.ptr, c_key.as_ptr()) } {
+        match unsafe { capi::pa_proplist_contains(self.0.ptr, c_key.as_ptr()) } {
             0 => Some(false),
             1 => Some(true),
             _ => None,
@@ -428,26 +448,26 @@ impl Proplist {
 
     /// Remove all entries from the property list object.
     pub fn clear(&mut self) {
-        unsafe { capi::pa_proplist_clear(self.ptr); }
+        unsafe { capi::pa_proplist_clear(self.0.ptr); }
     }
 
     /// Returns the number of entries in the property list.
     pub fn len(&self) -> u32 {
-        unsafe { capi::pa_proplist_size(self.ptr) }
+        unsafe { capi::pa_proplist_size(self.0.ptr) }
     }
 
     /// Returns `true` when the proplist is empty, false otherwise
     pub fn is_empty(&self) -> bool {
-        unsafe { capi::pa_proplist_isempty(self.ptr) == 0 }
+        unsafe { capi::pa_proplist_isempty(self.0.ptr) == 0 }
     }
 
     /// Returns `true` when self and `to` have the same keys and values.
     pub fn equal_to(&self, to: &Self) -> bool {
-        unsafe { capi::pa_proplist_equal(self.ptr, to.ptr) != 0 }
+        unsafe { capi::pa_proplist_equal(self.0.ptr, to.0.ptr) != 0 }
     }
 }
 
-impl Drop for Proplist {
+impl Drop for ProplistInner {
     fn drop(&mut self) {
         if !self.weak {
             unsafe { capi::pa_proplist_free(self.ptr) };
@@ -460,6 +480,41 @@ impl Clone for Proplist {
     /// Allocate a new property list and copy over every single entry from the specified list. If
     /// this is called on a ‘weak’ instance, a non-weak object is returned.
     fn clone(&self) -> Self {
-        Self::from_raw(unsafe { capi::pa_proplist_copy(self.ptr) })
+        Self::from_raw(unsafe { capi::pa_proplist_copy(self.0.ptr) })
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// Test that you cannot create a use-after-free situation by destroying a `Proplist` before an
+    /// associated `Iterator` (we avoid `Rc`/`Arc`).
+    #[test]
+    #[cfg(compile_fail)]
+    fn proplist_iter_lifetime() {
+        let iter = {
+            let my_props = Proplist::new().unwrap();
+            my_props.iter() //Returning this should not compile!
+        };
+
+        for key in iter {
+            //do something with it
+            println!("key: {}", key);
+        }
+    }
+
+    /// Test that you can however return an iterator if you convert the `Proplist` into one
+    #[test]
+    fn proplist_iter_lifetime_conv() {
+        let iter = {
+            let my_props = Proplist::new().unwrap();
+            my_props.into_iter()
+        };
+
+        for key in iter {
+            //do something with it
+            println!("key: {}", key);
+        }
     }
 }

@@ -610,7 +610,7 @@ impl Stream {
 
         let ptr = unsafe {
             capi::pa_stream_new_with_proplist(ctx.ptr, c_name.as_ptr(), std::mem::transmute(ss),
-                p_map, proplist.ptr)
+                p_map, proplist.0.ptr)
         };
         if ptr.is_null() {
             return None;
@@ -644,7 +644,7 @@ impl Stream {
 
         let ptr = unsafe {
             capi::pa_stream_new_extended(ctx.ptr, c_name.as_ptr(), info_ptrs.as_ptr(),
-                info_ptrs.len() as u32, proplist.ptr)
+                info_ptrs.len() as u32, proplist.0.ptr)
         };
         if ptr.is_null() {
             return None;
@@ -1576,7 +1576,7 @@ impl Stream {
         where F: FnMut(bool) + 'static
     {
         let cb_data = box_closure_get_capi_ptr::<FnMut(bool)>(Box::new(callback));
-        let ptr = unsafe { capi::pa_stream_proplist_update(self.ptr, mode, proplist.ptr,
+        let ptr = unsafe { capi::pa_stream_proplist_update(self.ptr, mode, proplist.0.ptr,
             Some(success_cb_proxy), cb_data) };
         assert!(!ptr.is_null());
         Operation::from_raw(ptr, cb_data as *mut Box<FnMut(bool)>)