qqiangwu / cppsafe

Cpp lifetime safety profile static analyzer
MIT License
45 stars 1 forks source link

rocksdb: container elements move gives false positives #17

Closed qqiangwu closed 5 months ago

qqiangwu commented 7 months ago
    for (size_t i = 0; i < child_fnames.size(); ++i) {
      const std::string path = dir + "/" + child_fnames[i];
      if (!(s = GetFileSize(path, options, &(*result)[result_size].size_bytes,
                            dbg))
               .ok()) {
        if (FileExists(path, options, dbg).IsNotFound()) {
          // The file may have been deleted since we listed the directory
          continue;
        }
        return s;
      }
      (*result)[result_size].name = std::move(child_fnames[i]);
      result_size++;
    }

gives

/Users/wuqq/dev/rocksdb-main/include/rocksdb/file_system.h:469:28: warning: use a moved-from object
  469 |     for (size_t i = 0; i < child_fnames.size(); ++i) {
      |                            ^~~~~~~~~~~~
/Users/wuqq/dev/rocksdb-main/include/rocksdb/file_system.h:480:37: note: moved here
  480 |       (*result)[result_size].name = std::move(child_fnames[i]);
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/wuqq/dev/rocksdb-main/include/rocksdb/file_system.h:470:44: warning: use a moved-from object
  470 |       const std::string path = dir + "/" + child_fnames[i];
      |                                            ^~~~~~~~~~~~
/Users/wuqq/dev/rocksdb-main/include/rocksdb/file_system.h:480:37: note: moved here
  480 |       (*result)[result_size].name = std::move(child_fnames[i]);
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/wuqq/dev/rocksdb-main/include/rocksdb/file_system.h:480:47: warning: use a moved-from object
  480 |       (*result)[result_size].name = std::move(child_fnames[i]);
      |                                               ^~~~~~~~~~~~
/Users/wuqq/dev/rocksdb-main/include/rocksdb/file_system.h:480:37: note: moved here
  480 |       (*result)[result_size].name = std::move(child_fnames[i]);
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~
3 warnings generated.
qqiangwu commented 7 months ago

move subobjects is not always a non trivial task, we prefer to ban it.

if you really needs it, maybe we can use a wrapper to be this:

// pset(container) = {*container}
move_each(container, [](auto&& elem){
    // use elem
});
// pset(container) = {invalid}
qqiangwu commented 7 months ago
#include <vector>
#include <memory>
#include <utility>

void foo(std::vector<std::unique_ptr<int>>& cont, std::unique_ptr<int>& p)
{
    for (auto& x : cont) {
        p = std::move(x);
    }
}

void bar(std::vector<std::unique_ptr<int>>& cont, std::unique_ptr<int>& p)
{
    for (size_t i = 0; i < cont.size(); ++i) {
        p = std::move(cont[i]);
    }
}

gives

/Users/wuqq/b.cc:7:18: warning: passing a dangling pointer as argument
    7 |     for (auto& x : cont) {
      |                  ^
/Users/wuqq/b.cc:8:13: note: moved here
    8 |         p = std::move(x);
      |             ^~~~~~~~~~~~
/Users/wuqq/b.cc:8:23: warning: dereferencing a possibly dangling pointer
    8 |         p = std::move(x);
      |                       ^
/Users/wuqq/b.cc:8:13: note: moved here
    8 |         p = std::move(x);
      |             ^~~~~~~~~~~~
/Users/wuqq/b.cc:14:28: warning: use a moved-from object
   14 |     for (size_t i = 0; i < cont.size(); ++i) {
      |                            ^~~~
/Users/wuqq/b.cc:15:13: note: moved here
   15 |         p = std::move(cont[i]);
      |             ^~~~~~~~~~~~~~~~~~
/Users/wuqq/b.cc:15:23: warning: use a moved-from object
   15 |         p = std::move(cont[i]);
      |                       ^~~~
/Users/wuqq/b.cc:15:13: note: moved here
   15 |         p = std::move(cont[i]);
      |             ^~~~~~~~~~~~~~~~~~
4 warnings generated.
qqiangwu commented 7 months ago

Yet another case:

assert(!flush_queue_.empty());
FlushRequest flush_req = std::move(flush_queue_.front());
flush_queue_.pop_front();

gives:

/Users/wuqq/dev/rocksdb-main/db/db_impl/db_impl_compaction_flush.cc:2956:3: warning: use a moved-from object
 2956 |   flush_queue_.pop_front();
      |   ^~~~~~~~~~~~
/Users/wuqq/dev/rocksdb-main/db/db_impl/db_impl_compaction_flush.cc:2955:28: note: moved here
 2955 |   FlushRequest flush_req = std::move(flush_queue_.front());
      |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
qqiangwu commented 6 months ago

Yet another example:

  for (const auto& c : file_ingesting_compactions_) {
    cfd_->compaction_picker()->UnregisterCompaction(c);
    delete c;
  }

yields:

/Users/wuqq/dev/rocksdb-main/db/external_sst_file_ingestion_job.cc:558:53: warning: passing a possibly dangling pointer as argument
  558 |     cfd_->compaction_picker()->UnregisterCompaction(c);
      |                                                     ^
/Users/wuqq/dev/rocksdb-main/db/external_sst_file_ingestion_job.cc:559:5: note: deleted here
  559 |     delete c;
      |     ^~~~~~~~
1 warning generated.
qqiangwu commented 6 months ago
      for (size_t i = 0; i < file_numbers.size(); i++) {
        assert(i < file_numbers.size());
        assert(i < checksums.size());
        assert(i < checksum_func_names.size());
        std::string checksum;
        if (is_checksum_hex_) {
          checksum = StringToHex(checksums[i]);
        } else {
          checksum = std::move(checksums[i]);
        }
        fprintf(stdout, "%" PRId64 ", %s, %s\n", file_numbers[i],
                checksum_func_names[i].c_str(), checksum.c_str());
      }