Skip to content
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

Fix Use After Free #48

Merged
merged 1 commit into from
Jul 7, 2017
Merged

Conversation

CryZe
Copy link
Contributor

@CryZe CryZe commented Jun 29, 2017

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 they were never interned properly, so they were super likely to get freed at some point and cause a Use after Free.

Fixes #47

@shepmaster
Copy link
Owner

I'd appreciate it if you'd avoid applying styling changes at the same time as any fixes. This code is quite old and predates many of the current best practices, so I'd prefer to apply style fixes en masse in a completely separate PR. In general, coupling these completely distinct sets of changes makes it very difficult to see what is changing and why (and makes git log and git bisect more difficult to use in the future).

@CryZe
Copy link
Contributor Author

CryZe commented Jul 1, 2017

Okay, I'll revert those, give me a second.

@CryZe CryZe force-pushed the fix-use-after-free branch 2 times, most recently from cb710d1 to b5be12b Compare July 1, 2017 18:01
@CryZe
Copy link
Contributor Author

CryZe commented Jul 1, 2017

Alright, I reverted all the styling changes.

}

index.insert(self.do_intern(s));
let interned_str: &str = index.get(s).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, there'd be some kind of entry-esque API so we reduce it down to a single hash calculation. I think that would need something akin to rust-lang/rfcs#1769 though.

In the meantime, I think it's OK to have extra hashes for the case where we have to insert anyway. However, can we reduce it from 3 to 2 with something like this?

let foo = self.do_intern(s);
index.insert(foo);
unsafe { mem::transmute(foo) }

That is, it's safe to copy the interned string as the pointer points into the pool and the length is just a number.

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
@CryZe CryZe force-pushed the fix-use-after-free branch from b5be12b to 9657cca Compare July 1, 2017 20:02
@CryZe
Copy link
Contributor Author

CryZe commented Jul 1, 2017

Alright, I got rid of the additional lookup.

@CryZe
Copy link
Contributor Author

CryZe commented Jul 7, 2017

Is there anything left to do?

@shepmaster
Copy link
Owner

Is there anything left to do?

Sorry, I was on a plane the last time we were talking and forgot about this 😊

I wanted to record some performance numbers:

Baseline

test string_pool::bench::many_repeated_string ... bench:      48,558 ns/iter (+/- 5,877) = 162 MB/s
test string_pool::bench::many_unique_string   ... bench:      38,882 ns/iter (+/- 5,716) = 228 MB/s
test string_pool::bench::single_string        ... bench:          31 ns/iter (+/- 4) = 161 MB/s

3-lookup (original PR)

test string_pool::bench::many_repeated_string ... bench:      45,971 ns/iter (+/- 5,451) = 171 MB/s
test string_pool::bench::many_unique_string   ... bench:      46,516 ns/iter (+/- 6,019) = 191 MB/s
test string_pool::bench::single_string        ... bench:          34 ns/iter (+/- 5) = 147 MB/s

2-lookup (current state of the PR)

test string_pool::bench::many_repeated_string ... bench:      41,426 ns/iter (+/- 13,485) = 190 MB/s
test string_pool::bench::many_unique_string   ... bench:      43,630 ns/iter (+/- 5,541) = 203 MB/s
test string_pool::bench::single_string        ... bench:          34 ns/iter (+/- 6) = 147 MB/s

@shepmaster shepmaster merged commit 34447ad into shepmaster:master Jul 7, 2017
@shepmaster
Copy link
Owner

Thank you very much for fixing this, and I'm sorry for any inconvenience we caused!

@shepmaster
Copy link
Owner

Released version 0.2.3

@CryZe
Copy link
Contributor Author

CryZe commented Jul 7, 2017

Don't worry about it, it never caused any trouble in practice. Thanks for releasing 0.2.3 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants