From 440621aab8e2669d5f74985780ed1c2bec2dfbf3 Mon Sep 17 00:00:00 2001 From: Sagar Vemuri Date: Wed, 11 Jul 2018 12:24:07 -0700 Subject: [PATCH] Fix Copying of data between buffers in FilePrefetchBuffer (#4100) Summary: Copy data between buffers inside FilePrefetchBuffer only when chunk length is greater than 0. Otherwise AlignedBuffer was accessing memory out of its range causing crashes. Removing the tracking of buffer length outside of `AlignedBuffer`, i.e. in `FilePrefetchBuffer` and `ReadaheadRandomAccessFile`, will follow in a separate PR, as it is not the root cause of the crash reported in #4051. (`FilePrefetchBuffer` itself has been this way from its inception, and `ReadaheadRandomAccessFile` was updated to add the buffer length at some point). Comprehensive tests for `FilePrefetchBuffer` also to follow in a separate PR. Pull Request resolved: https://github.com/facebook/rocksdb/pull/4100 Differential Revision: D8792590 Pulled By: sagar0 fbshipit-source-id: 3578f45761cf6884243e767f749db4016ccc93e1 --- util/aligned_buffer.h | 1 + util/file_reader_writer.cc | 9 ++++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/util/aligned_buffer.h b/util/aligned_buffer.h index 0024e4df4..2201b4877 100644 --- a/util/aligned_buffer.h +++ b/util/aligned_buffer.h @@ -121,6 +121,7 @@ public: ~static_cast(alignment_ - 1)); if (copy_data) { + assert(bufstart_ + copy_offset + copy_len <= bufstart_ + cursize_); memcpy(new_bufstart, bufstart_ + copy_offset, copy_len); cursize_ = copy_len; } else { diff --git a/util/file_reader_writer.cc b/util/file_reader_writer.cc index 127a19475..e8b03af49 100644 --- a/util/file_reader_writer.cc +++ b/util/file_reader_writer.cc @@ -672,7 +672,14 @@ Status FilePrefetchBuffer::Prefetch(RandomAccessFileReader* reader, chunk_len = buffer_len_ - chunk_offset_in_buffer; assert(chunk_offset_in_buffer % alignment == 0); assert(chunk_len % alignment == 0); - copy_data_to_new_buffer = true; + assert(chunk_offset_in_buffer + chunk_len <= + buffer_offset_ + buffer_len_); + if (chunk_len > 0) { + copy_data_to_new_buffer = true; + } else { + // this reset is not necessary, but just to be safe. + chunk_offset_in_buffer = 0; + } } }