Fix async prefetch heap use after free #11049
Conversation
|
@anand1976 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
file/file_prefetch_buffer.cc
Outdated
| @@ -327,6 +332,14 @@ Status FilePrefetchBuffer::HandleOverlappingData( | |||
| size_t alignment = reader->file()->GetRequiredBufferAlignment(); | |||
| uint32_t second = curr_ ^ 1; | |||
There was a problem hiding this comment.
I think we should move this statement below after the newly added check just to be safe if buffers get swapped in PollAndUpdateBuffersIfNeeded, although they won't but code will be consistent to always update second after PollAndUpdateBuffersIfNeeded.
|
@anand1976 has updated the pull request. You must reimport the pull request before landing. |
|
@anand1976 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@anand1976 has updated the pull request. You must reimport the pull request before landing. |
|
@anand1976 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@anand1976 merged this pull request in dbf37c2. |
Summary: This PR fixes a heap use after free bug in the async prefetch code that happens in the following scenario - 1. Scan thread starts 2 async reads for Seek, one for the seek block and one for prefetching 2. Before the first read in #1 completes, another thread reads and loads the block in cache 3. The first scan thread finds the block in cache, continues and the next block cache miss is for a block that spans the boundary of the 2 prefetch buffers, and the 1st read is complete but the 2nd one is not complete yet 4. The scan thread will reallocate (i.e free the old buffer and allocate a new one) the 2nd prefetch buffer, and the in-progress prefetch is orphaned 5. The orphaned prefetch finally completes, resulting in a use after free Also add a few asserts to surface bugs earlier in the crash tests. Pull Request resolved: #11049 Test Plan: Repro with db_stress and verify the fix Reviewed By: akankshamahajan15 Differential Revision: D42181118 Pulled By: anand1976 fbshipit-source-id: 1ac55d2f64a89ce128c1c574262b8aa7d82eb8cc
|
@anand1976 - seems like an irrelevant issue #1 is quoted in summary? |
This PR fixes a heap use after free bug in the async prefetch code that happens in the following scenario -
Also add a few asserts to surface bugs earlier in the crash tests.
Test plan:
Repro with db_stress and verify the fix