Fix the potential issues found by Coverity scan

Signed-off-by: Wu, Jiayu <[email protected]>

CMakeLists.txt

@@ -112,17 +112,17 @@ add_executable(bench benchmark/bench.cpp)
 target_link_libraries(bench PUBLIC engine)
 target_include_directories(bench PUBLIC ./include ./extern ./)
 
-option(BUILD_TESTING "Build the tests" ON)
+option(BUILD_TESTING "Build the tests" OFF)
 if (BUILD_TESTING)
     add_subdirectory(${CMAKE_SOURCE_DIR}/extern/gtest)
     add_subdirectory(tests)
 endif ()
 
 
-option(BUILD_TUTORIAL "Build the tutorial" ON)
+option(BUILD_TUTORIAL "Build the tutorial" OFF)
 add_subdirectory(examples/tutorial)
 
-option(BUILD_GRAPH_SIMULATOR "Build the graph simulator" ON)
+option(BUILD_GRAPH_SIMULATOR "Build the graph simulator" OFF)
 add_subdirectory(examples/graph_sim)
 
 add_custom_target(checkers ALL)

README.md

@@ -19,7 +19,7 @@
 * C/C++/Java APIs
 
 # Limitations
-*  The maximum supported key-value size is 64KB-4GB.
+*  The maximum supported key-value size is 64KB-4GB according to configs.
 *  No approach to key-value compression.
 *  Users can't expand the persistent memory space on the fly.
 

benchmark/bench.cpp

@@ -169,6 +169,9 @@ void DBWrite(int tid) {
   std::unique_ptr<WriteBatch> batch;
   if (engine != nullptr) {
     batch = engine->WriteBatchCreate();
+    if (batch == nullptr) return;
+  } else {
+    return;
   }
 
   for (size_t operations = 0; operations < operations_per_thread;
@@ -501,7 +504,7 @@ int main(int argc, char** argv) {
     if (s != Status::Ok) {
       throw std::runtime_error{
           std::string{"Fail to open KVDK instance. Status: "} +
-          KVDKStatusStrings[static_cast<int>(s)]};
+          ((s <= Status::Unknown) ? KVDKStatusStrings[s] : "")};
     }
   }
 
@@ -528,7 +531,7 @@ int main(int argc, char** argv) {
   switch (bench_data_type) {
     case DataType::Sorted: {
       printf("Create %ld Sorted Collections\n", FLAGS_num_collection);
-      for (auto col : collections) {
+      for (auto& col : collections) {
         SortedCollectionConfigs s_configs;
         Status s = engine->SortedCreate(col, s_configs);
         if (s != Status::Ok && s != Status::Existed) {
@@ -538,7 +541,7 @@ int main(int argc, char** argv) {
       break;
     }
     case DataType::Hashes: {
-      for (auto col : collections) {
+      for (auto& col : collections) {
         Status s = engine->HashCreate(col);
         if (s != Status::Ok && s != Status::Existed) {
           throw std::runtime_error{"Fail to create Hashset"};
@@ -547,7 +550,7 @@ int main(int argc, char** argv) {
       break;
     }
     case DataType::List: {
-      for (auto col : collections) {
+      for (auto& col : collections) {
         Status s = engine->ListCreate(col);
         if (s != Status::Ok && s != Status::Existed) {
           throw std::runtime_error{"Fail to create List"};

engine/backup_log.hpp

@@ -78,6 +78,7 @@ class BackupLog {
 
   BackupLog() = default;
   BackupLog(const BackupLog&) = delete;
+  BackupLog& operator=(const BackupLog& rhs) = delete;
 
   // Init a new backup log
   Status Init(const std::string& backup_log) {
@@ -141,7 +142,7 @@ class BackupLog {
       file_size_ = lseek(fd_, 0, SEEK_END);
       if (file_size_ < sizeof(BackupStage)) {
         GlobalLogger.Error(
-            "Open backup log file %s error: file size %lu smaller than "
+            "Open backup log file %lu error: file size %lu smaller than "
             "persisted "
             "stage flag",
             backup_log.size(), file_size_);
@@ -207,7 +208,7 @@ class BackupLog {
 
   // Close backup log file
   void Close() {
-    if (log_file_ != nullptr) {
+    if (log_file_ != nullptr && file_size_ > 0) {
       munmap(log_file_, file_size_);
     }
     if (fd_ >= 0) {
@@ -224,7 +225,9 @@ class BackupLog {
   // Destroy backup log file
   void Destroy() {
     Close();
-    remove(file_name_.c_str());
+    if (remove(file_name_.c_str()) == -1) {
+      GlobalLogger.Error("Fail to remove file %s\n", file_name_.c_str());
+    }
   }
 
  private:
@@ -272,8 +275,8 @@ class BackupLog {
   std::string file_name_{};
   std::string delta_{};
   void* log_file_{nullptr};
-  size_t file_size_{0};
+  uint64_t file_size_{0};
   int fd_{-1};
   BackupStage stage_{BackupStage::NotFinished};
 };
-}  // namespace KVDK_NAMESPACE
+}  // namespace KVDK_NAMESPACE

engine/c/kvdk_sorted.cpp

@@ -26,9 +26,6 @@ KVDKStatus KVDKSortedCreate(KVDKEngine* engine, const char* collection_name,
                             KVDKSortedCollectionConfigs* configs) {
   KVDKStatus s = engine->rep->SortedCreate(
       StringView(collection_name, collection_len), configs->rep);
-  if (s != KVDKStatus::Ok) {
-    return s;
-  }
   return s;
 }
 

engine/dl_list.hpp

@@ -39,6 +39,8 @@ class DLList {
 
     WriteArgs(WriteArgs&& args) = delete;
 
+    WriteArgs& operator=(const WriteArgs& rhs) = delete;
+
     StringView key;
     StringView val;
     RecordType type;
@@ -197,7 +199,8 @@ class DLListDataIterator {
   DLRecord* findValidVersion(DLRecord* pmem_record) {
     DLRecord* curr = pmem_record;
     TimestampType ts = snapshot_->GetTimestamp();
-    while (curr != nullptr && curr->GetTimestamp() > ts) {
+    while (curr != nullptr) {
+      if (curr->GetTimestamp() <= ts) break;
       curr = pmem_allocator_->offset2addr<DLRecord>(curr->old_version);
       kvdk_assert(curr == nullptr || curr->Validate(),
                   "Broken checkpoint: invalid older version sorted record");
@@ -346,4 +349,4 @@ class DLListRecoveryUtils {
  private:
   const PMEMAllocator* pmem_allocator_;
 };
-}  // namespace KVDK_NAMESPACE
+}  // namespace KVDK_NAMESPACE

engine/dram_allocator.cpp

@@ -15,6 +15,7 @@ void ChunkBasedAllocator::Free(const SpaceEntry&) {
 SpaceEntry ChunkBasedAllocator::Allocate(uint64_t size) {
   kvdk_assert(ThreadManager::ThreadID() >= 0, "");
   SpaceEntry entry;
+  memset(&entry, 0, sizeof(entry));
   auto& tc = dalloc_thread_cache_[ThreadManager::ThreadID() %
                                   dalloc_thread_cache_.size()];
   if (size > chunk_size_) {
@@ -43,4 +44,4 @@ SpaceEntry ChunkBasedAllocator::Allocate(uint64_t size) {
   tc.usable_bytes -= size;
   return entry;
 }
-}  // namespace KVDK_NAMESPACE
+}  // namespace KVDK_NAMESPACE

engine/dram_allocator.hpp

@@ -34,12 +34,16 @@ class ChunkBasedAllocator : Allocator {
       : dalloc_thread_cache_(max_access_threads) {}
   ChunkBasedAllocator(ChunkBasedAllocator const&) = delete;
   ChunkBasedAllocator(ChunkBasedAllocator&&) = delete;
+  ChunkBasedAllocator& operator=(const ChunkBasedAllocator& rhs) = delete;
   ~ChunkBasedAllocator() {
-    for (uint64_t i = 0; i < dalloc_thread_cache_.size(); i++) {
-      auto& tc = dalloc_thread_cache_[i];
-      for (void* chunk : tc.allocated_chunks) {
-        free(chunk);
+    try {
+      for (uint64_t i = 0; i < dalloc_thread_cache_.size(); i++) {
+        auto& tc = dalloc_thread_cache_[i];
+        for (void* chunk : tc.allocated_chunks) {
+          free(chunk);
+        }
       }
+    } catch (std::exception& err) {
     }
   }
 
@@ -52,9 +56,10 @@ class ChunkBasedAllocator : Allocator {
     DAllocThreadCache() = default;
     DAllocThreadCache(const DAllocThreadCache&) = delete;
     DAllocThreadCache(DAllocThreadCache&&) = delete;
+    DAllocThreadCache& operator=(const DAllocThreadCache& rhs) = delete;
   };
 
   const uint32_t chunk_size_ = (1 << 20);
   Array<DAllocThreadCache> dalloc_thread_cache_;
 };
-}  // namespace KVDK_NAMESPACE
+}  // namespace KVDK_NAMESPACE

engine/hash_collection/hash_list.cpp

@@ -123,7 +123,7 @@ HashList::WriteResult HashList::Modify(const StringView key,
 
 HashWriteArgs HashList::InitWriteArgs(const StringView& key,
                                       const StringView& value, WriteOp op) {
-  HashWriteArgs args;
+  HashWriteArgs args{};
   args.key = key;
   args.value = value;
   args.op = op;
@@ -423,4 +423,4 @@ HashList::WriteResult HashList::deletePrepared(
   return ret;
 }
 
-}  // namespace KVDK_NAMESPACE
+}  // namespace KVDK_NAMESPACE

engine/hash_collection/rebuilder.hpp

@@ -234,8 +234,8 @@ class HashListRebuilder {
 
     CollectionIDType id = HashList::FetchID(pmem_record);
     DLRecord* curr = pmem_record;
-    while (curr != nullptr &&
-           curr->GetTimestamp() > checkpoint_.CheckpointTS()) {
+    while (curr != nullptr) {
+      if (curr->GetTimestamp() <= checkpoint_.CheckpointTS()) break;
       curr = pmem_allocator_->offset2addr<DLRecord>(curr->old_version);
       kvdk_assert(curr == nullptr || curr->Validate(),
                   "Broken checkpoint: invalid older version sorted record");
@@ -353,11 +353,11 @@ class HashListRebuilder {
       invalid_hlists_;
   std::unordered_map<CollectionIDType, std::shared_ptr<HashList>>
       rebuild_hlists_;
-  CollectionIDType max_recovered_id_;
+  CollectionIDType max_recovered_id_{0};
 
   // We manually allocate recovery thread id for no conflict in multi-thread
   // recovering
   // Todo: do not hard code
   std::atomic<uint64_t> next_tid_{0};
 };
-}  // namespace KVDK_NAMESPACE
+}  // namespace KVDK_NAMESPACE

engine/hash_table.cpp

@@ -21,7 +21,7 @@ HashTable* HashTable::NewHashTable(uint64_t hash_bucket_num,
     table = new HashTable(hash_bucket_num, num_buckets_per_slot, pmem_allocator,
                           max_access_threads);
   } catch (std::bad_alloc& b) {
-    GlobalLogger.Error("No enough dram to create global hash table: b\n",
+    GlobalLogger.Error("No enough dram to create global hash table: %s\n",
                        b.what());
     table = nullptr;
   }
@@ -80,7 +80,7 @@ bool HashEntry::Match(const StringView& key, uint32_t hash_k_prefix,
       }
     }
 
-    if (data_entry_metadata != nullptr) {
+    if (data_entry_metadata != nullptr && pmem_record != nullptr) {
       memcpy(data_entry_metadata, pmem_record, sizeof(DataEntry));
     }
 

engine/hash_table.hpp

@@ -129,16 +129,18 @@ class HashTable {
     HashEntry* entry_ptr{nullptr};
 
     LookupResult& operator=(LookupResult const& other) {
-      s = other.s;
-      memcpy_16(&entry, &other.entry);
-      entry_ptr = other.entry_ptr;
-      key_hash_prefix = other.key_hash_prefix;
+      if (&other != this) {
+        s = other.s;
+        memcpy_16(&entry, &other.entry);
+        entry_ptr = other.entry_ptr;
+        key_hash_prefix = other.key_hash_prefix;
+      }
       return *this;
     }
 
    private:
     friend class HashTable;
-    uint32_t key_hash_prefix;
+    uint32_t key_hash_prefix{0};
   };
 
   static HashTable* NewHashTable(uint64_t hash_bucket_num,
@@ -254,7 +256,6 @@ class HashTable {
   Array<Slot> slots_;
   std::vector<uint64_t> hash_bucket_entries_;
   Array<HashBucket> hash_buckets_;
-  void* main_buckets_;
 };
 
 // Iterator all hash entries in a hash table bucket

engine/kv_engine.cpp

@@ -219,7 +219,7 @@ Status KVEngine::restoreData() {
   SpaceEntry segment_recovering;
   DataEntry data_entry_cached;
   uint64_t cnt = 0;
-  Status s;
+  Status s = Status::Ok;
   while (true) {
     if (segment_recovering.size == 0) {
       if (!pmem_allocator_->FetchSegment(&segment_recovering)) {
@@ -415,7 +415,8 @@ Status KVEngine::Backup(const pmem::obj::string_view backup_log,
       switch (slot_iter->GetRecordType()) {
         case RecordType::String: {
           StringRecord* record = slot_iter->GetIndex().string_record;
-          while (record != nullptr && record->GetTimestamp() > backup_ts) {
+          while (record != nullptr) {
+            if (record->GetTimestamp() <= backup_ts) break;
             record =
                 pmem_allocator_->offset2addr<StringRecord>(record->old_version);
           }
@@ -428,7 +429,8 @@ Status KVEngine::Backup(const pmem::obj::string_view backup_log,
         }
         case RecordType::SortedRecord: {
           DLRecord* header = slot_iter->GetIndex().skiplist->HeaderRecord();
-          while (header != nullptr && header->GetTimestamp() > backup_ts) {
+          while (header != nullptr) {
+            if (header->GetTimestamp() <= backup_ts) break;
             header =
                 pmem_allocator_->offset2addr<DLRecord>(header->old_version);
           }
@@ -458,7 +460,8 @@ Status KVEngine::Backup(const pmem::obj::string_view backup_log,
         }
         case RecordType::HashRecord: {
           DLRecord* header = slot_iter->GetIndex().hlist->HeaderRecord();
-          while (header != nullptr && header->GetTimestamp() > backup_ts) {
+          while (header != nullptr) {
+            if (header->GetTimestamp() <= backup_ts) break;
             header =
                 pmem_allocator_->offset2addr<DLRecord>(header->old_version);
           }
@@ -487,7 +490,8 @@ Status KVEngine::Backup(const pmem::obj::string_view backup_log,
         }
         case RecordType::ListRecord: {
           DLRecord* header = slot_iter->GetIndex().list->HeaderRecord();
-          while (header != nullptr && header->GetTimestamp() > backup_ts) {
+          while (header != nullptr) {
+            if (header->GetTimestamp() <= backup_ts) break;
             header =
                 pmem_allocator_->offset2addr<DLRecord>(header->old_version);
           }
@@ -838,7 +842,7 @@ Status KVEngine::checkConfigs(const Configs& configs) {
   if (configs.pmem_file_size % segment_size != 0) {
     GlobalLogger.Error(
         "pmem file size should align to segment "
-        "size(pmem_segment_blocks*pmem_block_size) (%d bytes)\n",
+        "size(pmem_segment_blocks*pmem_block_size) (%lu bytes)\n",
         segment_size);
     return Status::InvalidConfiguration;
   }
@@ -1162,6 +1166,7 @@ Status KVEngine::batchWriteRollbackLogs() {
                        strerror(errno));
     return Status::IOError;
   }
+  defer(closedir(dir));
   dirent* entry;
   while ((entry = readdir(dir)) != NULL) {
     std::string fname = std::string{entry->d_name};
@@ -1219,7 +1224,6 @@ Status KVEngine::batchWriteRollbackLogs() {
       return Status::PMemMapFileError;
     }
   }
-  closedir(dir);
   std::string cmd{"rm -rf " + batch_log_dir_ + "*"};
   [[gnu::unused]] int ret = system(cmd.c_str());
 
@@ -1432,7 +1436,8 @@ T* KVEngine::removeOutDatedVersion(T* record, TimestampType min_snapshot_ts) {
       "Invalid record type, should be StringRecord or DLRecord.");
   T* ret = nullptr;
   auto old_record = record;
-  while (old_record && old_record->GetTimestamp() > min_snapshot_ts) {
+  while (old_record) {
+    if (old_record->GetTimestamp() <= min_snapshot_ts) break;
     old_record =
         static_cast<T*>(pmem_allocator_->offset2addr(old_record->old_version));
   }

engine/kv_engine.hpp

@@ -659,6 +659,7 @@ class KVEngine : public Engine {
   struct BackgroundWorkSignals {
     BackgroundWorkSignals() = default;
     BackgroundWorkSignals(const BackgroundWorkSignals&) = delete;
+    BackgroundWorkSignals& operator=(const BackgroundWorkSignals& rhs) = delete;
 
     std::condition_variable_any pmem_usage_reporter_cv;
     std::condition_variable_any pmem_allocator_organizer_cv;