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.39k stars 6.29k forks source link

Fix stale memory access with FSBuffer and tiered sec cache #12712

Closed anand1976 closed 4 months ago

anand1976 commented 4 months ago

A BlockBasedTable with TieredSecondaryCache containing a NVM cache inserts blocks into the compressed cache and the corresponding compressed block into the NVM cache. The BlockFetcher is used to get the uncompressed and compressed blocks by calling ReadBlockContents() and GetUncompressedBlock() respectively. If the file system supports FSBuffer (i.e returning a FS allocated buffer rather than caller provided), that buffer gets freed between the two calls. This PR fixes it by making the FSBuffer unique pointer a member rather than local variable.

Test plan:

  1. Add a unit test
  2. Release validation stress test
facebook-github-bot commented 4 months ago

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

facebook-github-bot commented 4 months ago

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

facebook-github-bot commented 4 months ago

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

facebook-github-bot commented 4 months ago

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

facebook-github-bot commented 4 months ago

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

anand1976 commented 4 months ago

Quick question for my learning. I'm wondering where in the the test could have failed if we didn't make the change in the BlockFetcher. Or are we relying on ASAN to detect the issue while running the test?

I thought about adding some assertions, but its rather complicated since there are many different cases for sourcing the data buffer. I think ASAN is the best way to catch it for now.

facebook-github-bot commented 4 months ago

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

facebook-github-bot commented 4 months ago

@anand1976 merged this pull request in facebook/rocksdb@0ae3d9f98deac3259d16eb64009df659d3c8259b.