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
main
Peter Dillinger 3 years ago committed by Facebook GitHub Bot
parent bbe4763ee4
commit 5cb137a860
  1. 2
      Makefile
  2. 9
      db/error_handler.cc
  3. 11
      include/rocksdb/io_status.h
  4. 21
      include/rocksdb/status.h
  5. 6
      third-party/gtest-1.8.1/fused-src/gtest/gtest.h
  6. 27
      util/status.cc
  7. 1
      utilities/ttl/ttl_test.cc

@ -1221,7 +1221,7 @@ analyze_incremental:
$(CLANG_SCAN_BUILD) --use-analyzer=$(CLANG_ANALYZER) \ $(CLANG_SCAN_BUILD) --use-analyzer=$(CLANG_ANALYZER) \
--use-c++=$(CXX) --use-cc=$(CC) --status-bugs \ --use-c++=$(CXX) --use-cc=$(CC) --status-bugs \
-o $(CURDIR)/scan_build_report \ -o $(CURDIR)/scan_build_report \
$(MAKE) dbg $(MAKE) SKIP_LINK=1 dbg
CLEAN_FILES += unity.cc CLEAN_FILES += unity.cc
unity.cc: Makefile util/build_version.cc.in unity.cc: Makefile util/build_version.cc.in

@ -9,6 +9,7 @@
#include "db/event_helpers.h" #include "db/event_helpers.h"
#include "file/sst_file_manager_impl.h" #include "file/sst_file_manager_impl.h"
#include "logging/logging.h" #include "logging/logging.h"
#include "port/lang.h"
namespace ROCKSDB_NAMESPACE { namespace ROCKSDB_NAMESPACE {
@ -251,6 +252,8 @@ void ErrorHandler::CancelErrorRecovery() {
#endif #endif
} }
STATIC_AVOID_DESTRUCTION(const Status, kOkStatus){Status::OK()};
// This is the main function for looking at an error during a background // This is the main function for looking at an error during a background
// operation and deciding the severity, and error recovery strategy. The high // operation and deciding the severity, and error recovery strategy. The high
// level algorithm is as follows - // level algorithm is as follows -
@ -273,7 +276,7 @@ const Status& ErrorHandler::SetBGError(const Status& bg_err,
BackgroundErrorReason reason) { BackgroundErrorReason reason) {
db_mutex_->AssertHeld(); db_mutex_->AssertHeld();
if (bg_err.ok()) { if (bg_err.ok()) {
return bg_err; return kOkStatus;
} }
if (bg_error_stats_ != nullptr) { if (bg_error_stats_ != nullptr) {
@ -383,7 +386,7 @@ const Status& ErrorHandler::SetBGError(const IOStatus& bg_io_err,
BackgroundErrorReason reason) { BackgroundErrorReason reason) {
db_mutex_->AssertHeld(); db_mutex_->AssertHeld();
if (bg_io_err.ok()) { if (bg_io_err.ok()) {
return bg_io_err; return kOkStatus;
} }
ROCKS_LOG_WARN(db_options_.info_log, "Background IO error %s", ROCKS_LOG_WARN(db_options_.info_log, "Background IO error %s",
bg_io_err.ToString().c_str()); bg_io_err.ToString().c_str());
@ -625,7 +628,7 @@ const Status& ErrorHandler::StartRecoverFromRetryableBGIOError(
if (bg_error_.ok()) { if (bg_error_.ok()) {
return bg_error_; return bg_error_;
} else if (io_error.ok()) { } else if (io_error.ok()) {
return io_error; return kOkStatus;
} else if (db_options_.max_bgerror_resume_count <= 0 || recovery_in_prog_) { } else if (db_options_.max_bgerror_resume_count <= 0 || recovery_in_prog_) {
// Auto resume BG error is not enabled, directly return bg_error_. // Auto resume BG error is not enabled, directly return bg_error_.
return bg_error_; return bg_error_;

@ -171,7 +171,7 @@ inline IOStatus::IOStatus(Code _code, SubCode _subcode, const Slice& msg,
memcpy(result + len1 + 2, msg2.data(), len2); memcpy(result + len1 + 2, msg2.data(), len2);
} }
result[size] = '\0'; // null terminator for C style string 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_) { 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_; retryable_ = s.retryable_;
data_loss_ = s.data_loss_; data_loss_ = s.data_loss_;
scope_ = s.scope_; 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) { inline IOStatus& IOStatus::operator=(const IOStatus& s) {
// The following condition catches both aliasing (when this == &s), // The following condition catches both aliasing (when this == &s),
@ -196,8 +196,7 @@ inline IOStatus& IOStatus::operator=(const IOStatus& s) {
retryable_ = s.retryable_; retryable_ = s.retryable_;
data_loss_ = s.data_loss_; data_loss_ = s.data_loss_;
scope_ = s.scope_; scope_ = s.scope_;
delete[] state_; state_ = (s.state_ == nullptr) ? nullptr : CopyState(s.state_.get());
state_ = (s.state_ == nullptr) ? nullptr : CopyState(s.state_);
} }
return *this; return *this;
} }
@ -228,9 +227,7 @@ inline IOStatus& IOStatus::operator=(IOStatus&& s)
data_loss_ = s.data_loss_; data_loss_ = s.data_loss_;
scope_ = s.scope_; scope_ = s.scope_;
s.scope_ = kIOErrorScopeFileSystem; s.scope_ = kIOErrorScopeFileSystem;
delete[] state_; state_ = std::move(s.state_);
state_ = nullptr;
std::swap(state_, s.state_);
} }
return *this; return *this;
} }

@ -21,6 +21,7 @@
#include <stdlib.h> #include <stdlib.h>
#endif #endif
#include <memory>
#include <string> #include <string>
#ifdef ROCKSDB_ASSERT_STATUS_CHECKED #ifdef ROCKSDB_ASSERT_STATUS_CHECKED
@ -43,7 +44,6 @@ class Status {
abort(); abort();
} }
#endif // ROCKSDB_ASSERT_STATUS_CHECKED #endif // ROCKSDB_ASSERT_STATUS_CHECKED
delete[] state_;
} }
// Copy the specified status. // Copy the specified status.
@ -144,7 +144,7 @@ class Status {
// Returns a C style string indicating the message of the Status // Returns a C style string indicating the message of the Status
const char* getState() const { const char* getState() const {
MarkChecked(); MarkChecked();
return state_; return state_.get();
} }
// Return a success status. // Return a success status.
@ -456,20 +456,20 @@ class Status {
Severity sev_; Severity sev_;
// A nullptr state_ (which is at least the case for OK) means the extra // A nullptr state_ (which is at least the case for OK) means the extra
// message is empty. // message is empty.
const char* state_; std::unique_ptr<const char[]> state_;
#ifdef ROCKSDB_ASSERT_STATUS_CHECKED #ifdef ROCKSDB_ASSERT_STATUS_CHECKED
mutable bool checked_ = false; mutable bool checked_ = false;
#endif // ROCKSDB_ASSERT_STATUS_CHECKED #endif // ROCKSDB_ASSERT_STATUS_CHECKED
explicit Status(Code _code, SubCode _subcode = kNone) 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, Status(Code _code, SubCode _subcode, const Slice& msg, const Slice& msg2,
Severity sev = kNoError); Severity sev = kNoError);
Status(Code _code, const Slice& msg, const Slice& msg2) Status(Code _code, const Slice& msg, const Slice& msg2)
: Status(_code, kNone, msg, msg2) {} : Status(_code, kNone, msg, msg2) {}
static const char* CopyState(const char* s); static std::unique_ptr<const char[]> CopyState(const char* s);
inline void MarkChecked() const { inline void MarkChecked() const {
#ifdef ROCKSDB_ASSERT_STATUS_CHECKED #ifdef ROCKSDB_ASSERT_STATUS_CHECKED
@ -481,12 +481,12 @@ class Status {
inline Status::Status(const Status& s) inline Status::Status(const Status& s)
: code_(s.code_), subcode_(s.subcode_), sev_(s.sev_) { : code_(s.code_), subcode_(s.subcode_), sev_(s.sev_) {
s.MarkChecked(); 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) inline Status::Status(const Status& s, Severity sev)
: code_(s.code_), subcode_(s.subcode_), sev_(sev) { : code_(s.code_), subcode_(s.subcode_), sev_(sev) {
s.MarkChecked(); 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) { inline Status& Status::operator=(const Status& s) {
if (this != &s) { if (this != &s) {
@ -495,8 +495,7 @@ inline Status& Status::operator=(const Status& s) {
code_ = s.code_; code_ = s.code_;
subcode_ = s.subcode_; subcode_ = s.subcode_;
sev_ = s.sev_; sev_ = s.sev_;
delete[] state_; state_ = (s.state_ == nullptr) ? nullptr : CopyState(s.state_.get());
state_ = (s.state_ == nullptr) ? nullptr : CopyState(s.state_);
} }
return *this; return *this;
} }
@ -524,9 +523,7 @@ inline Status& Status::operator=(Status&& s)
s.subcode_ = kNone; s.subcode_ = kNone;
sev_ = std::move(s.sev_); sev_ = std::move(s.sev_);
s.sev_ = kNoError; s.sev_ = kNoError;
delete[] state_; state_ = std::move(s.state_);
state_ = nullptr;
std::swap(state_, s.state_);
} }
return *this; return *this;
} }

@ -53,6 +53,7 @@
#define GTEST_INCLUDE_GTEST_GTEST_H_ #define GTEST_INCLUDE_GTEST_GTEST_H_
#include <limits> #include <limits>
#include <memory>
#include <ostream> #include <ostream>
#include <vector> #include <vector>
@ -2442,6 +2443,10 @@ GTEST_API_ bool IsTrue(bool condition);
// Defines scoped_ptr. // Defines scoped_ptr.
// RocksDB: use unique_ptr to work around some clang-analyze false reports
template <typename T>
using scoped_ptr = std::unique_ptr<T>;
/*
// This implementation of scoped_ptr is PARTIAL - it only contains // This implementation of scoped_ptr is PARTIAL - it only contains
// enough stuff to satisfy Google Test's need. // enough stuff to satisfy Google Test's need.
template <typename T> template <typename T>
@ -2481,6 +2486,7 @@ class scoped_ptr {
GTEST_DISALLOW_COPY_AND_ASSIGN_(scoped_ptr); GTEST_DISALLOW_COPY_AND_ASSIGN_(scoped_ptr);
}; };
*/
// Defines RE. // Defines RE.

@ -17,24 +17,11 @@
namespace ROCKSDB_NAMESPACE { namespace ROCKSDB_NAMESPACE {
const char* Status::CopyState(const char* state) { std::unique_ptr<const char[]> Status::CopyState(const char* s) {
#ifdef OS_WIN const size_t cch = std::strlen(s) + 1; // +1 for the null terminator
const size_t cch = std::strlen(state) + 1; // +1 for the null terminator char* rv = new char[cch];
char* result = new char[cch]; std::strncpy(rv, s, cch);
errno_t ret return std::unique_ptr<const char[]>(rv);
#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
} }
static const char* msgs[static_cast<int>(Status::kMaxSubCode)] = { static const char* msgs[static_cast<int>(Status::kMaxSubCode)] = {
@ -72,7 +59,7 @@ Status::Status(Code _code, SubCode _subcode, const Slice& msg,
memcpy(result + len1 + 2, msg2.data(), len2); memcpy(result + len1 + 2, msg2.data(), len2);
} }
result[size] = '\0'; // null terminator for C style string result[size] = '\0'; // null terminator for C style string
state_ = result; state_.reset(result);
} }
std::string Status::ToString() const { std::string Status::ToString() const {
@ -152,7 +139,7 @@ std::string Status::ToString() const {
if (subcode_ != kNone) { if (subcode_ != kNone) {
result.append(": "); result.append(": ");
} }
result.append(state_); result.append(state_.get());
} }
return result; return result;
} }

@ -185,6 +185,7 @@ class TtlTest : public testing::Test {
// Runs a manual compaction // Runs a manual compaction
Status ManualCompact(ColumnFamilyHandle* cf = nullptr) { Status ManualCompact(ColumnFamilyHandle* cf = nullptr) {
assert(db_ttl_);
if (cf == nullptr) { if (cf == nullptr) {
return db_ttl_->CompactRange(CompactRangeOptions(), nullptr, nullptr); return db_ttl_->CompactRange(CompactRangeOptions(), nullptr, nullptr);
} else { } else {

Loading…
Cancel
Save