-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use after Free when parsing this XML Document #47
Comments
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. |
The String Pool now uses just a HashSet, that stores the actual Interned Strings. The old code also stored the str slices that we where looking for as keys, but there were never interned properly, so they were super likely to get freed at some point and cause a Use after Free. Fixes shepmaster#47
Thanks for the report! However, I don't agree with your assessment so I'd like your help to understand what I'm missing...
This isn't quite true... it does take a string slice and converts it to a let search_string = InternedString::from_str(s); This is only used to look up in the
At no point is the input string returned. In fact, there's a test to make sure that specific case doesn't happen ( |
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. |
Ah, that makes sense! I always wanted to switch to a |
The String Pool now uses just a HashSet, that stores the actual Interned Strings. The old code also stored the str slices that we where looking for as keys, but there were never interned properly, so they were super likely to get freed at some point and cause a Use after Free. Fixes shepmaster#47
The String Pool now uses just a HashSet, that stores the actual Interned Strings. The old code also stored the str slices that we where looking for as keys, but there were never interned properly, so they were super likely to get freed at some point and cause a Use after Free. Fixes shepmaster#47
The String Pool now uses just a HashSet, that stores the actual Interned Strings. The old code also stored the str slices that we where looking for as keys, but there were never interned properly, so they were super likely to get freed at some point and cause a Use after Free. Fixes shepmaster#47
Found by cargo-fuzz:
crash-52cdb28f04f0c80d84609394d18ed2c0b8fedb7f.zip
Caused at:
Freed at:
Allocated at:
The text was updated successfully, but these errors were encountered: