search

facebook / rocksdb

A library that provides an embeddable, persistent key-value store for fast storage.
http://rocksdb.org
GNU General Public License v2.0
28.38k stars 6.29k forks source link

Disable "uncache" behavior in DB shutdown #12751

Closed pdillinger closed 3 months ago

pdillinger commented 3 months ago

Summary: Crash test showed a potential use-after-free where a file marked as obsolete and eligible for uncache on destruction is destroyed in the VersionSet destructor, which only happens as part of DB shutdown. At that point, the in-memory column families have already been destroyed, so attempting to uncache could use-after-free on stuff like getting the user_comparator() from the internal_comparator().

I attempted to make it smarter, but wasn't able to untangle the destruction dependencies in a way that was safe, understandable, and maintainable.

Test Plan: Reproduced by adding uncache_aggressiveness to an existing (but otherwise unrelated) test. This makes it a fair regression test.

Also added testing to ensure that trivial moves and DB close & reopen are well behaved with uncache_aggressiveness. Specifically, this issue doesn't seem to be because things are uncached inappropriately in those cases.

facebook-github-bot commented 3 months ago

@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot commented 3 months ago

@pdillinger has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot commented 3 months ago

@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot commented 3 months ago

@pdillinger merged this pull request in facebook/rocksdb@21eb82ebec503ab9fa9b2d9f36e3c332920f8274.