From 98870c7b9c24b423d374d0b8c2227a0e95efaeea Mon Sep 17 00:00:00 2001 From: Igor Sugak Date: Thu, 19 Feb 2015 14:07:38 -0800 Subject: [PATCH] rocksdb: Fix scan-build memory warning in table/block_based_table_reader.cc Summary: scan-build is reporting two memory leak bugs in `table/block_based_table_reader.cc`. They are both false positives. In both cases we allocate memory in `ReadBlockFromFile` if `s.ok()`. Then after the function `ReadBlockFromFile` returns we check for the same variable if `s.ok()` and then use the memory that was allocated. The bugs reported by scan-build is if `ReadBlockFromFile` allocates memory and returns, but for some reason status `s` is not the same and `s.ok() != true`. In this case scan-build is concerned that memory owner transfer is not explicit. I modified `ReadBlockFromFile` to accept `std::unique_ptr*` as a parameter, instead of raw pointer. scan-build reports: http://home.fburl.com/~sugak/latest2/report-a4b3fa.html#EndPath http://home.fburl.com/~sugak/latest2/report-29adbf.html#EndPath Test Plan: Make sure scan-build does not report these bugs and all tests are passing. ```lang=bash % make check % make analyze ``` Reviewers: sdong, lgalanis, meyering, igor Reviewed By: igor Subscribers: dhruba, leveldb Differential Revision: https://reviews.facebook.net/D33681 --- table/block_based_table_reader.cc | 41 ++++++++++++++++++------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/table/block_based_table_reader.cc b/table/block_based_table_reader.cc index abda6b8bf..01243a4fc 100644 --- a/table/block_based_table_reader.cc +++ b/table/block_based_table_reader.cc @@ -64,12 +64,13 @@ const size_t kMaxCacheKeyPrefixSize __attribute__((unused)) = // On success fill *result and return OK - caller owns *result Status ReadBlockFromFile(RandomAccessFile* file, const Footer& footer, const ReadOptions& options, const BlockHandle& handle, - Block** result, Env* env, bool do_uncompress = true) { + std::unique_ptr* result, Env* env, + bool do_uncompress = true) { BlockContents contents; Status s = ReadBlockContents(file, footer, options, handle, &contents, env, do_uncompress); if (s.ok()) { - *result = new Block(std::move(contents)); + result->reset(new Block(std::move(contents))); } return s; @@ -168,12 +169,13 @@ class BinarySearchIndexReader : public IndexReader { const BlockHandle& index_handle, Env* env, const Comparator* comparator, IndexReader** index_reader) { - Block* index_block = nullptr; + std::unique_ptr index_block; auto s = ReadBlockFromFile(file, footer, ReadOptions(), index_handle, &index_block, env); if (s.ok()) { - *index_reader = new BinarySearchIndexReader(comparator, index_block); + *index_reader = + new BinarySearchIndexReader(comparator, std::move(index_block)); } return s; @@ -192,8 +194,9 @@ class BinarySearchIndexReader : public IndexReader { } private: - BinarySearchIndexReader(const Comparator* comparator, Block* index_block) - : IndexReader(comparator), index_block_(index_block) { + BinarySearchIndexReader(const Comparator* comparator, + std::unique_ptr&& index_block) + : IndexReader(comparator), index_block_(std::move(index_block)) { assert(index_block_ != nullptr); } std::unique_ptr index_block_; @@ -209,7 +212,7 @@ class HashIndexReader : public IndexReader { const BlockHandle& index_handle, Iterator* meta_index_iter, IndexReader** index_reader, bool hash_index_allow_collision) { - Block* index_block = nullptr; + std::unique_ptr index_block; auto s = ReadBlockFromFile(file, footer, ReadOptions(), index_handle, &index_block, env); @@ -222,7 +225,7 @@ class HashIndexReader : public IndexReader { // So, Create will succeed regardless, from this point on. auto new_index_reader = - new HashIndexReader(comparator, index_block); + new HashIndexReader(comparator, std::move(index_block)); *index_reader = new_index_reader; // Get prefixes block @@ -300,8 +303,9 @@ class HashIndexReader : public IndexReader { } private: - HashIndexReader(const Comparator* comparator, Block* index_block) - : IndexReader(comparator), index_block_(index_block) { + HashIndexReader(const Comparator* comparator, + std::unique_ptr&& index_block) + : IndexReader(comparator), index_block_(std::move(index_block)) { assert(index_block_ != nullptr); } @@ -615,7 +619,7 @@ Status BlockBasedTable::ReadMetaBlock( // TODO(sanjay): Skip this if footer.metaindex_handle() size indicates // it is an empty block. // TODO: we never really verify check sum for meta index block - Block* meta = nullptr; + std::unique_ptr meta; Status s = ReadBlockFromFile( rep->file.get(), rep->footer, @@ -628,13 +632,12 @@ Status BlockBasedTable::ReadMetaBlock( Log(InfoLogLevel::ERROR_LEVEL, rep->ioptions.info_log, "Encountered error while reading data from properties" " block %s", s.ToString().c_str()); - delete meta; return s; } - meta_block->reset(meta); + *meta_block = std::move(meta); // meta block uses bytewise comparator. - iter->reset(meta->NewIterator(BytewiseComparator())); + iter->reset(meta_block->get()->NewIterator(BytewiseComparator())); return Status::OK(); } @@ -970,7 +973,7 @@ Iterator* BlockBasedTable::NewDataBlockIterator(Rep* rep, rep->table_options.format_version); if (block.value == nullptr && !no_io && ro.fill_cache) { - Block* raw_block = nullptr; + std::unique_ptr raw_block; { StopWatch sw(rep->ioptions.env, statistics, READ_BLOCK_GET_MICROS); s = ReadBlockFromFile(rep->file.get(), rep->footer, ro, handle, @@ -980,7 +983,7 @@ Iterator* BlockBasedTable::NewDataBlockIterator(Rep* rep, if (s.ok()) { s = PutDataBlockToCache(key, ckey, block_cache, block_cache_compressed, - ro, statistics, &block, raw_block, + ro, statistics, &block, raw_block.release(), rep->table_options.format_version); } } @@ -997,8 +1000,12 @@ Iterator* BlockBasedTable::NewDataBlockIterator(Rep* rep, return NewErrorIterator(Status::Incomplete("no blocking io")); } } + std::unique_ptr block_value; s = ReadBlockFromFile(rep->file.get(), rep->footer, ro, handle, - &block.value, rep->ioptions.env); + &block_value, rep->ioptions.env); + if (s.ok()) { + block.value = block_value.release(); + } } Iterator* iter;