dermesser / leveldb-rs

A reimplementation of LevelDB in Rust (no bindings).
Other
521 stars 59 forks source link

Investigate Skipmap memory leaks #12

Closed dermesser closed 2 years ago

dermesser commented 2 years ago

There seems to be a bug in the implementation of SkipMap leading to memory leaks. I found this while investigating #11 which refers to a different area though.

Instrumenting the Drop implementation:

impl Drop for Node {
    fn drop(&mut self) {
        // large object should drop
        if let Some(mut next) = self.next.take() {
            println!("destroyed {:?}", next.key);
            while let Some(child) = next.next.take() {
                next = child;
                println!("destroyed {:?}", next.key);
            }
        }
            println!("destroyed {:?}", self.key);
        unsafe {
            for skip in self.skips.iter_mut() {
                if let Some(mut next) = skip.take() {
                    while let Some(child) = (*next).next.take() {
                        next = Box::into_raw(child);
                    }
                }
            }
        }
    }
}

gives

running 1 test
destroyed [97, 98, 97]
destroyed [97, 98, 97]
destroyed [97, 98, 98]
destroyed [97, 98, 98]
destroyed []
test skipmap::tests::test_skipmap_iterator_init ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 142 filtered out; finished in 0.00s

for a skipmap constructed as


    pub fn make_skipmap() -> SkipMap {
        let mut skm = SkipMap::new(options::for_test().cmp);
        let keys = vec![
            "aba", "abb", "abc", "abd", "abe", "abf", "abg", "abh", "abi", "abj", "abk", "abl",
            "abm", "abn", "abo", "abp", "abq", "abr", "abs", "abt", "abu", "abv", "abw", "abx",
            "aby", "abz",
        ];

        for k in keys {
            skm.insert(k.as_bytes().to_vec(), "def".as_bytes().to_vec());
        }
        skm
    }
dermesser commented 2 years ago

Nicely enough, the default implementation already did the right thing. By deleting the Drop impl, this memory leak is gone.