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 by making lru_list drop before map #3

Merged
merged 1 commit into from
Jun 11, 2022

Conversation

saethlin
Copy link
Contributor

In its Drop impl, intrusive_collections::LinkedList walks the list, which is a use after free if it is dropped second. Using ManuallyDrop is the clearest way to control drop order and prevent this issue.

@cehteh
Copy link
Owner

cehteh commented Jun 11, 2022

Thanks for the report. Is that really true? (Do you have an example/test-case)

I was under the assumption that by drop order the HashSet becomes dropped first and each of it's entries destructors removes the LinkedListLink from the List, eventually leaving an empty list when the LinkedList becomes dropped. Otherwise apologies for my misunderstanding if I got that wrong.

@saethlin
Copy link
Contributor Author

My test cases are your test cases :) I think any nonempty CacheDb will run into this.

You can run

RUSTFLAGS=-Zsanitizer=address cargo +nightly test

to see AddressSanitizer detect the use after free.

I'm not an expert on intrusive data structures, I'm just following what the tools tell me here. But I think you may be assuming that the intrusive linked list here is much more clever than it actually is. The LinkedListLink doesn't have a Drop impl, so when you drop the HashSet nothing happens to update the LinkedList to let it know that the list has been cleared. Then it does have a Drop impl that tries to walk and clear the list. https://docs.rs/intrusive-collections/0.9.4/src/intrusive_collections/linked_list.rs.html#1333

@cehteh cehteh merged commit 7743046 into cehteh:master Jun 11, 2022
@cehteh
Copy link
Owner

cehteh commented Jun 11, 2022

Thanks, I learned something new.

@cehteh
Copy link
Owner

cehteh commented Jun 11, 2022

.. makes me wonder if LinkedListLink should have a Drop that at least removes it from the List (but I haven't put much thoughts into it, if it has any ill side effects)

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