Use after Free when parsing this XML Document

[CryZe]

Found by cargo-fuzz:

crash-52cdb28f04f0c80d84609394d18ed2c0b8fedb7f.zip

Caused at:

...
<sxd_document::string_pool::InternedString as core::cmp::PartialEq>::eq
...
std::collections::hash::map::search_hashed
...
sxd_document::string_pool::StringPool::intern
sxd_document::raw::Storage::intern
sxd_document::raw::Storage::create_attribute
sxd_document::dom::Element::set_attribute_value
sxd_document::parser::DomBuilder::finish_opening_tag
sxd_document::parser::DomBuilder::consume
sxd_document::parser::parse

Freed at:

...
alloc::raw_vec::RawVec<...>::dealloc_buffer
RawVec<...>::drop
core::ptr::drop_in_place
core::ptr::drop_in_place
core::ptr::drop_in_place
core::ptr::drop_in_place
sxd_document::parser::DomBuilder::finish_opening_tag
sxd_document::parser::DomBuilder::consume
sxd_document::parser::parse

Allocated at:

...
<alloc::vec::Vec<T>>::extend_from_slice
alloc::string::String::push_str
sxd_document::parser::AttributeValueBuilder::ingest
sxd_document::parser::DomBuilder::finish_opening_tag
sxd_document::parser::DomBuilder::consume
sxd_document::parser::parse

[CryZe]

Looks like the String Pool is super unsafe.

    pub fn intern<'s>(&'s self, s: &str) -> &'s str {
        if s == "" { return ""; }

        let search_string = InternedString::from_str(s);

        let mut index = self.index.borrow_mut();
        let interned_str = *index.entry(search_string).or_insert_with(|| self.do_intern(s));

        // The lifetime is really matched to us
        unsafe { mem::transmute(interned_str) }
    }

It takes a random str slice and inserts it into the pool as the key. However there's no guarantee that the key will still be alive at a later point in time, as the key itself is not interned. So later when using the HashMap, it'll compare the keys and so it'll compare to memory that isn't allocated anymore.

[shepmaster]

Thanks for the report! However, I don't agree with your assessment so I'd like your help to understand what I'm missing...

It takes a random str slice and inserts it into the pool as the key

This isn't quite true... it does take a string slice and converts it to a InternedString:

let search_string = InternedString::from_str(s);

This is only used to look up in the HashMap. If the value is found, we return the value from the pool. If it is not found, we add it to the pool and return that value. In both cases, the return value is one of the pooled values

let interned_str = *index.entry(search_string).or_insert_with(|| self.do_intern(s));

At no point is the input string returned. In fact, there's a test to make sure that specific case doesn't happen (does_not_reuse_the_pointer_of_the_input).

[CryZe]

It's not returned, but it's stored as the key of the HashMap through the Entry API, where it later gets compared when looking up more values. The or_insert_with closure only provides the value of the HashMap, it takes the key from the entry parameter.

[shepmaster]

stored as the key of the HashMap

Ah, that makes sense! I always wanted to switch to a HashSet, but couldn't originally because the feature to get the value stored in the HashSet wasn't present. It's a good thing that they finally added it, although I guess Rust 1.9.0 was a while ago now!