From 5cb137a860654e98ee30a3913f16bb07faebab2b Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Mon, 7 Feb 2022 18:20:51 -0800 Subject: [PATCH] Work around some new clang-analyze failures (#9515) Summary: ... seen only in internal clang-analyze runs after https://github.com/facebook/rocksdb/issues/9481 * Mostly, this works around falsely reported leaks by using std::unique_ptr in some places where clang-analyze was getting confused. (I didn't see any changes in C++17 that could make our Status implementation leak memory.) * Also fixed SetBGError returning address of a stack variable. * Also fixed another false null deref report by adding an assert. Also, use SKIP_LINK=1 to speed up `make analyze` Pull Request resolved: https://github.com/facebook/rocksdb/pull/9515 Test Plan: Was able to reproduce the reported errors locally and verify they're fixed (except SetBGError). Otherwise, existing tests Reviewed By: hx235 Differential Revision: D34054630 Pulled By: pdillinger fbshipit-source-id: 38600ef3da75ddca307dff96b7a1a523c2885c2e --- Makefile | 2 +- db/error_handler.cc | 9 ++++--- include/rocksdb/io_status.h | 11 +++----- include/rocksdb/status.h | 21 +++++++-------- .../gtest-1.8.1/fused-src/gtest/gtest.h | 6 +++++ util/status.cc | 27 +++++-------------- utilities/ttl/ttl_test.cc | 1 + 7 files changed, 34 insertions(+), 43 deletions(-) diff --git a/Makefile b/Makefile index c4b8dc57f..ca6a48984 100644 --- a/Makefile +++ b/Makefile @@ -1221,7 +1221,7 @@ analyze_incremental: $(CLANG_SCAN_BUILD) --use-analyzer=$(CLANG_ANALYZER) \ --use-c++=$(CXX) --use-cc=$(CC) --status-bugs \ -o $(CURDIR)/scan_build_report \ - $(MAKE) dbg + $(MAKE) SKIP_LINK=1 dbg CLEAN_FILES += unity.cc unity.cc: Makefile util/build_version.cc.in diff --git a/db/error_handler.cc b/db/error_handler.cc index ba8a8a91f..5b5b08f75 100644 --- a/db/error_handler.cc +++ b/db/error_handler.cc @@ -9,6 +9,7 @@ #include "db/event_helpers.h" #include "file/sst_file_manager_impl.h" #include "logging/logging.h" +#include "port/lang.h" namespace ROCKSDB_NAMESPACE { @@ -251,6 +252,8 @@ void ErrorHandler::CancelErrorRecovery() { #endif } +STATIC_AVOID_DESTRUCTION(const Status, kOkStatus){Status::OK()}; + // This is the main function for looking at an error during a background // operation and deciding the severity, and error recovery strategy. The high // level algorithm is as follows - @@ -273,7 +276,7 @@ const Status& ErrorHandler::SetBGError(const Status& bg_err, BackgroundErrorReason reason) { db_mutex_->AssertHeld(); if (bg_err.ok()) { - return bg_err; + return kOkStatus; } if (bg_error_stats_ != nullptr) { @@ -383,7 +386,7 @@ const Status& ErrorHandler::SetBGError(const IOStatus& bg_io_err, BackgroundErrorReason reason) { db_mutex_->AssertHeld(); if (bg_io_err.ok()) { - return bg_io_err; + return kOkStatus; } ROCKS_LOG_WARN(db_options_.info_log, "Background IO error %s", bg_io_err.ToString().c_str()); @@ -625,7 +628,7 @@ const Status& ErrorHandler::StartRecoverFromRetryableBGIOError( if (bg_error_.ok()) { return bg_error_; } else if (io_error.ok()) { - return io_error; + return kOkStatus; } else if (db_options_.max_bgerror_resume_count <= 0 || recovery_in_prog_) { // Auto resume BG error is not enabled, directly return bg_error_. return bg_error_; diff --git a/include/rocksdb/io_status.h b/include/rocksdb/io_status.h index ea13d3bc5..e7290e438 100644 --- a/include/rocksdb/io_status.h +++ b/include/rocksdb/io_status.h @@ -171,7 +171,7 @@ inline IOStatus::IOStatus(Code _code, SubCode _subcode, const Slice& msg, memcpy(result + len1 + 2, msg2.data(), len2); } result[size] = '\0'; // null terminator for C style string - state_ = result; + state_.reset(result); } inline IOStatus::IOStatus(const IOStatus& s) : Status(s.code_, s.subcode_) { @@ -181,7 +181,7 @@ inline IOStatus::IOStatus(const IOStatus& s) : Status(s.code_, s.subcode_) { retryable_ = s.retryable_; data_loss_ = s.data_loss_; scope_ = s.scope_; - state_ = (s.state_ == nullptr) ? nullptr : CopyState(s.state_); + state_ = (s.state_ == nullptr) ? nullptr : CopyState(s.state_.get()); } inline IOStatus& IOStatus::operator=(const IOStatus& s) { // The following condition catches both aliasing (when this == &s), @@ -196,8 +196,7 @@ inline IOStatus& IOStatus::operator=(const IOStatus& s) { retryable_ = s.retryable_; data_loss_ = s.data_loss_; scope_ = s.scope_; - delete[] state_; - state_ = (s.state_ == nullptr) ? nullptr : CopyState(s.state_); + state_ = (s.state_ == nullptr) ? nullptr : CopyState(s.state_.get()); } return *this; } @@ -228,9 +227,7 @@ inline IOStatus& IOStatus::operator=(IOStatus&& s) data_loss_ = s.data_loss_; scope_ = s.scope_; s.scope_ = kIOErrorScopeFileSystem; - delete[] state_; - state_ = nullptr; - std::swap(state_, s.state_); + state_ = std::move(s.state_); } return *this; } diff --git a/include/rocksdb/status.h b/include/rocksdb/status.h index 6f6f09c5c..5550fa828 100644 --- a/include/rocksdb/status.h +++ b/include/rocksdb/status.h @@ -21,6 +21,7 @@ #include #endif +#include #include #ifdef ROCKSDB_ASSERT_STATUS_CHECKED @@ -43,7 +44,6 @@ class Status { abort(); } #endif // ROCKSDB_ASSERT_STATUS_CHECKED - delete[] state_; } // Copy the specified status. @@ -144,7 +144,7 @@ class Status { // Returns a C style string indicating the message of the Status const char* getState() const { MarkChecked(); - return state_; + return state_.get(); } // Return a success status. @@ -456,20 +456,20 @@ class Status { Severity sev_; // A nullptr state_ (which is at least the case for OK) means the extra // message is empty. - const char* state_; + std::unique_ptr state_; #ifdef ROCKSDB_ASSERT_STATUS_CHECKED mutable bool checked_ = false; #endif // ROCKSDB_ASSERT_STATUS_CHECKED explicit Status(Code _code, SubCode _subcode = kNone) - : code_(_code), subcode_(_subcode), sev_(kNoError), state_(nullptr) {} + : code_(_code), subcode_(_subcode), sev_(kNoError) {} Status(Code _code, SubCode _subcode, const Slice& msg, const Slice& msg2, Severity sev = kNoError); Status(Code _code, const Slice& msg, const Slice& msg2) : Status(_code, kNone, msg, msg2) {} - static const char* CopyState(const char* s); + static std::unique_ptr CopyState(const char* s); inline void MarkChecked() const { #ifdef ROCKSDB_ASSERT_STATUS_CHECKED @@ -481,12 +481,12 @@ class Status { inline Status::Status(const Status& s) : code_(s.code_), subcode_(s.subcode_), sev_(s.sev_) { s.MarkChecked(); - state_ = (s.state_ == nullptr) ? nullptr : CopyState(s.state_); + state_ = (s.state_ == nullptr) ? nullptr : CopyState(s.state_.get()); } inline Status::Status(const Status& s, Severity sev) : code_(s.code_), subcode_(s.subcode_), sev_(sev) { s.MarkChecked(); - state_ = (s.state_ == nullptr) ? nullptr : CopyState(s.state_); + state_ = (s.state_ == nullptr) ? nullptr : CopyState(s.state_.get()); } inline Status& Status::operator=(const Status& s) { if (this != &s) { @@ -495,8 +495,7 @@ inline Status& Status::operator=(const Status& s) { code_ = s.code_; subcode_ = s.subcode_; sev_ = s.sev_; - delete[] state_; - state_ = (s.state_ == nullptr) ? nullptr : CopyState(s.state_); + state_ = (s.state_ == nullptr) ? nullptr : CopyState(s.state_.get()); } return *this; } @@ -524,9 +523,7 @@ inline Status& Status::operator=(Status&& s) s.subcode_ = kNone; sev_ = std::move(s.sev_); s.sev_ = kNoError; - delete[] state_; - state_ = nullptr; - std::swap(state_, s.state_); + state_ = std::move(s.state_); } return *this; } diff --git a/third-party/gtest-1.8.1/fused-src/gtest/gtest.h b/third-party/gtest-1.8.1/fused-src/gtest/gtest.h index ebb16db7b..2d82d8e4d 100644 --- a/third-party/gtest-1.8.1/fused-src/gtest/gtest.h +++ b/third-party/gtest-1.8.1/fused-src/gtest/gtest.h @@ -53,6 +53,7 @@ #define GTEST_INCLUDE_GTEST_GTEST_H_ #include +#include #include #include @@ -2442,6 +2443,10 @@ GTEST_API_ bool IsTrue(bool condition); // Defines scoped_ptr. +// RocksDB: use unique_ptr to work around some clang-analyze false reports +template +using scoped_ptr = std::unique_ptr; +/* // This implementation of scoped_ptr is PARTIAL - it only contains // enough stuff to satisfy Google Test's need. template @@ -2481,6 +2486,7 @@ class scoped_ptr { GTEST_DISALLOW_COPY_AND_ASSIGN_(scoped_ptr); }; +*/ // Defines RE. diff --git a/util/status.cc b/util/status.cc index efd83b0ae..025c47d31 100644 --- a/util/status.cc +++ b/util/status.cc @@ -17,24 +17,11 @@ namespace ROCKSDB_NAMESPACE { -const char* Status::CopyState(const char* state) { -#ifdef OS_WIN - const size_t cch = std::strlen(state) + 1; // +1 for the null terminator - char* result = new char[cch]; - errno_t ret -#if defined(_MSC_VER) - ; -#else - __attribute__((__unused__)); -#endif - ret = strncpy_s(result, cch, state, cch - 1); - result[cch - 1] = '\0'; - assert(ret == 0); - return result; -#else - const size_t cch = std::strlen(state) + 1; // +1 for the null terminator - return std::strncpy(new char[cch], state, cch); -#endif +std::unique_ptr Status::CopyState(const char* s) { + const size_t cch = std::strlen(s) + 1; // +1 for the null terminator + char* rv = new char[cch]; + std::strncpy(rv, s, cch); + return std::unique_ptr(rv); } static const char* msgs[static_cast(Status::kMaxSubCode)] = { @@ -72,7 +59,7 @@ Status::Status(Code _code, SubCode _subcode, const Slice& msg, memcpy(result + len1 + 2, msg2.data(), len2); } result[size] = '\0'; // null terminator for C style string - state_ = result; + state_.reset(result); } std::string Status::ToString() const { @@ -152,7 +139,7 @@ std::string Status::ToString() const { if (subcode_ != kNone) { result.append(": "); } - result.append(state_); + result.append(state_.get()); } return result; } diff --git a/utilities/ttl/ttl_test.cc b/utilities/ttl/ttl_test.cc index 3daef74ca..0a0f69255 100644 --- a/utilities/ttl/ttl_test.cc +++ b/utilities/ttl/ttl_test.cc @@ -185,6 +185,7 @@ class TtlTest : public testing::Test { // Runs a manual compaction Status ManualCompact(ColumnFamilyHandle* cf = nullptr) { + assert(db_ttl_); if (cf == nullptr) { return db_ttl_->CompactRange(CompactRangeOptions(), nullptr, nullptr); } else {