fix memory leaks in rust, cache methods (#750)

* fix memory leaks in rust, cache methods

* pin down to older nightly

* fix caches?

* send help

* fix clippy pls

.circleci/config.yml

@@ -6,13 +6,12 @@ references:
   restore_caches: &restore_caches
     restore_cache:
       keys:
-        - cache3-{{ arch }}-{{ .Branch }}
+        - cache7-{{ arch }}-{{ .Branch }}
 
   save_caches: &save_caches
     save_cache:
-      key: cache3-{{ arch }}-{{ .Branch }}
+      key: cache7-{{ arch }}-{{ .Branch }}
       paths:
-        - "rust/target/"
         - "~/.cargo/"
         - "~/.cache/pip/"
         - "~/.rustup/"

Makefile

@@ -33,9 +33,11 @@ export RUST_LOG := vdirsyncer_rustext=debug
 PYTEST_ARGS =
 
 # Variables below this line are not very interesting for getting started.
-
 TEST_EXTRA_PACKAGES =
 
+# The rust toolchain to install. You need nightly to run clippy
+RUST_TOOLCHAIN = nightly-2018-06-20
+
 ifeq ($(COVERAGE), true)
 	TEST_EXTRA_PACKAGES += pytest-cov
 	PYTEST_ARGS += --cov-config .coveragerc --cov vdirsyncer
@@ -107,8 +109,8 @@ style:
 	! git grep -i syncroniz */*
 	! git grep -i 'text/icalendar' */*
 	sphinx-build -W -b html ./docs/ ./docs/_build/html/
-	cd rust/ && cargo +nightly clippy
-	cd rust/ && cargo +nightly fmt --all -- --check
+	cd rust/ && cargo +$(RUST_TOOLCHAIN) clippy
+	cd rust/ && cargo +$(RUST_TOOLCHAIN) fmt --all -- --check
 
 install-docs:
 	pip install -Ur docs-requirements.txt
@@ -149,8 +151,9 @@ ssh-submodule-urls:
 		git remote get-url origin"
 
 install-rust:
-	curl https://sh.rustup.rs -sSf | sh -s -- -y --default-toolchain nightly
-	rustup update nightly
+	curl https://sh.rustup.rs -sSf | sh -s -- -y --default-toolchain $(RUST_TOOLCHAIN)
+	rustup update $(RUST_TOOLCHAIN)
+	rustup default $(RUST_TOOLCHAIN)
 
 rust/vdirsyncer_rustext.h:
 	cd rust/ && cargo build # hack to work around cbindgen bugs

rust/Cargo.toml

@@ -26,3 +26,4 @@ serde_derive = "1.0.66"
 serde_json = "1.0"
 glob = "0.2"
 shellexpand = "1.0"
+lazy-init = "0.3"

rust/src/item.rs

@@ -3,46 +3,66 @@ use vobject;
 use sha2::{Digest, Sha256};
 use std::fmt::Write;
 
+use lazy_init::Lazy;
+
 use errors::*;
 
 #[derive(Clone)]
-pub enum Item {
+enum ItemInner {
     Parsed(vobject::Component),
     Unparseable(String), // FIXME: maybe use https://crates.io/crates/terminated
 }
 
+pub struct Item {
+    inner: ItemInner,
+    uid: Lazy<Option<String>>,
+    hash: Lazy<Option<String>>,
+}
+
 impl Item {
     pub fn from_raw(raw: String) -> Self {
-        match vobject::parse_component(&raw) {
-            Ok(x) => Item::Parsed(x),
+        Self::from_inner(match vobject::parse_component(&raw) {
+            Ok(x) => ItemInner::Parsed(x),
             // Don't chain vobject error here because it cannot be stored/cloned FIXME
-            _ => Item::Unparseable(raw),
-        }
+            _ => ItemInner::Unparseable(raw),
+        })
     }
 
     pub fn from_component(component: vobject::Component) -> Self {
-        Item::Parsed(component)
+        Self::from_inner(ItemInner::Parsed(component))
+    }
+
+    fn from_inner(inner: ItemInner) -> Self {
+        Item {
+            inner,
+            uid: Lazy::new(),
+            hash: Lazy::new(),
+        }
     }
 
     /// Global identifier of the item, across storages, doesn't change after a modification of the
     /// item.
-    pub fn get_uid(&self) -> Option<String> {
-        // FIXME: Cache
-        if let Item::Parsed(ref c) = *self {
-            let mut stack: Vec<&vobject::Component> = vec![c];
-
-            while let Some(vobj) = stack.pop() {
-                if let Some(prop) = vobj.get_only("UID") {
-                    return Some(prop.value_as_string());
+    pub fn get_uid(&self) -> Option<&str> {
+        self.uid
+            .get_or_create(|| {
+                if let ItemInner::Parsed(ref c) = self.inner {
+                    let mut stack: Vec<&vobject::Component> = vec![c];
+
+                    while let Some(vobj) = stack.pop() {
+                        if let Some(prop) = vobj.get_only("UID") {
+                            return Some(prop.value_as_string());
+                        }
+                        stack.extend(vobj.subcomponents.iter());
+                    }
                 }
-                stack.extend(vobj.subcomponents.iter());
-            }
-        }
-        None
+                None
+            })
+            .as_ref()
+            .map(|x| &**x)
     }
 
     pub fn with_uid(&self, uid: &str) -> Fallible<Self> {
-        if let Item::Parsed(ref component) = *self {
+        if let ItemInner::Parsed(ref component) = self.inner {
             let mut new_component = component.clone();
             change_uid(&mut new_component, uid);
             Ok(Item::from_raw(vobject::write_component(&new_component)))
@@ -53,41 +73,46 @@ impl Item {
 
     /// Raw unvalidated content of the item
     pub fn get_raw(&self) -> String {
-        match *self {
-            Item::Parsed(ref component) => vobject::write_component(component),
-            Item::Unparseable(ref x) => x.to_owned(),
+        match self.inner {
+            ItemInner::Parsed(ref component) => vobject::write_component(component),
+            ItemInner::Unparseable(ref x) => x.to_owned(),
         }
     }
 
     /// Component of item if parseable
     pub fn get_component(&self) -> Fallible<&vobject::Component> {
-        match *self {
-            Item::Parsed(ref component) => Ok(component),
+        match self.inner {
+            ItemInner::Parsed(ref component) => Ok(component),
             _ => Err(Error::ItemUnparseable.into()),
         }
     }
 
     /// Component of item if parseable
     pub fn into_component(self) -> Fallible<vobject::Component> {
-        match self {
-            Item::Parsed(component) => Ok(component),
+        match self.inner {
+            ItemInner::Parsed(component) => Ok(component),
             _ => Err(Error::ItemUnparseable.into()),
         }
     }
 
     /// Used for etags
-    pub fn get_hash(&self) -> Fallible<String> {
-        // FIXME: cache
-        if let Item::Parsed(ref component) = *self {
-            Ok(hash_component(component))
-        } else {
-            Err(Error::ItemUnparseable.into())
-        }
+    pub fn get_hash(&self) -> Fallible<&str> {
+        self.hash
+            .get_or_create(|| {
+                if let ItemInner::Parsed(ref component) = self.inner {
+                    Some(hash_component(component))
+                } else {
+                    None
+                }
+            })
+            .as_ref()
+            .map(|x| &**x)
+            .ok_or_else(|| Error::ItemUnparseable.into())
     }
 
     /// Used for generating hrefs and matching up items during synchronization. This is either the
     /// UID or the hash of the item's content.
-    pub fn get_ident(&self) -> Fallible<String> {
+    pub fn get_ident(&self) -> Fallible<&str> {
         if let Some(x) = self.get_uid() {
             return Ok(x);
         }
@@ -100,14 +125,20 @@ impl Item {
     }
 
     pub fn is_parseable(&self) -> bool {
-        if let Item::Parsed(_) = *self {
+        if let ItemInner::Parsed(_) = self.inner {
             true
         } else {
             false
         }
     }
 }
 
+impl Clone for Item {
+    fn clone(&self) -> Item {
+        Item::from_inner(self.inner.clone())
+    }
+}
+
 fn change_uid(c: &mut vobject::Component, uid: &str) {
     let mut stack = vec![c];
     while let Some(component) = stack.pop() {
@@ -195,14 +226,11 @@ pub mod exports {
     use std::os::raw::c_char;
     use std::ptr;
 
-    const EMPTY_STRING: *const c_char = b"\0" as *const u8 as *const c_char;
-
     #[no_mangle]
     pub unsafe extern "C" fn vdirsyncer_get_uid(c: *mut Item) -> *const c_char {
-        match (*c).get_uid() {
-            Some(x) => CString::new(x).unwrap().into_raw(),
-            None => EMPTY_STRING,
-        }
+        CString::new((*c).get_uid().unwrap_or(""))
+            .unwrap()
+            .into_raw()
     }
 
     #[no_mangle]

rust/src/lib.rs

@@ -13,6 +13,7 @@ extern crate log;
 extern crate chrono;
 extern crate env_logger;
 extern crate glob;
+extern crate lazy_init;
 extern crate quick_xml;
 extern crate reqwest;
 extern crate sha2;
@@ -29,15 +30,15 @@ mod item;
 mod storage;
 
 pub mod exports {
-    use std::ffi::CStr;
+    use std::ffi::CString;
     use std::os::raw::c_char;
 
     pub use super::item::exports::*;
     pub use super::storage::exports::*;
 
     #[no_mangle]
-    pub unsafe extern "C" fn vdirsyncer_free_str(s: *const c_char) {
-        CStr::from_ptr(s);
+    pub unsafe extern "C" fn vdirsyncer_free_str(s: *mut c_char) {
+        let _ = CString::from_raw(s);
     }
 
     #[no_mangle]

rust/src/storage/dav/mod.rs

@@ -391,8 +391,8 @@ pub struct CarddavConfig {
 }
 
 impl StorageConfig for CarddavConfig {
-    fn get_collection(&self) -> Option<String> {
-        self.collection.clone()
+    fn get_collection(&self) -> Option<&str> {
+        self.collection.as_ref().map(|x| &**x)
     }
 }
 
@@ -449,23 +449,20 @@ impl ConfigurableStorage for CarddavStorage {
 
 pub struct CaldavStorage {
     inner: DavClient,
-    start_date: Option<chrono::DateTime<chrono::Utc>>, // FIXME: store as Option<(start, end)>
-    end_date: Option<chrono::DateTime<chrono::Utc>>,
+    date_range: Option<(chrono::DateTime<chrono::Utc>, chrono::DateTime<chrono::Utc>)>,
     item_types: Vec<String>,
 }
 
 impl CaldavStorage {
     pub fn new(
         url: &str,
         http_config: HttpConfig,
-        start_date: Option<chrono::DateTime<chrono::Utc>>,
-        end_date: Option<chrono::DateTime<chrono::Utc>>,
+        date_range: Option<(chrono::DateTime<chrono::Utc>, chrono::DateTime<chrono::Utc>)>,
         item_types: Vec<String>,
     ) -> Self {
         CaldavStorage {
             inner: DavClient::new(url, http_config),
-            start_date,
-            end_date,
+            date_range,
             item_types,
         }
     }
@@ -475,7 +472,7 @@ impl CaldavStorage {
         let mut item_types = self.item_types.clone();
         let mut timefilter = "".to_owned();
 
-        if let (Some(start), Some(end)) = (self.start_date, self.end_date) {
+        if let Some((start, end)) = self.date_range {
             timefilter = format!(
                 "<C:time-range start=\"{}\" end=\"{}\" />",
                 start.format(CALDAV_DT_FORMAT),
@@ -616,8 +613,8 @@ pub struct CaldavConfig {
 }
 
 impl StorageConfig for CaldavConfig {
-    fn get_collection(&self) -> Option<String> {
-        self.collection.clone()
+    fn get_collection(&self) -> Option<&str> {
+        self.collection.as_ref().map(|x| &**x)
     }
 }
 
@@ -752,8 +749,7 @@ pub mod exports {
         Box::into_raw(Box::new(Box::new(CaldavStorage::new(
             url.to_str().unwrap(),
             init_http_config(username, password, useragent, verify_cert, auth_cert),
-            parse_date(start_date),
-            parse_date(end_date),
+            parse_date(start_date).and_then(|start| Some((start, parse_date(end_date)?))),
             item_types,
         ))))
     }

rust/src/storage/filesystem.rs

@@ -230,8 +230,8 @@ pub struct Config {
 }
 
 impl StorageConfig for Config {
-    fn get_collection(&self) -> Option<String> {
-        self.collection.clone()
+    fn get_collection(&self) -> Option<&str> {
+        self.collection.as_ref().map(|x| &**x)
     }
 }
 

rust/src/storage/http.rs

@@ -116,8 +116,8 @@ impl Storage for HttpStorage {
         for component in split_collection(&s)? {
             let mut item = Item::from_component(component);
             item = item.with_uid(&item.get_hash()?)?;
-            let ident = item.get_ident()?;
-            let hash = item.get_hash()?;
+            let ident = item.get_ident()?.to_owned();
+            let hash = item.get_hash()?.to_owned();
             new_cache.insert(ident, (item, hash));
         }