From 2adb7e37683e3a0f8d19ccf61ce8e7008bd3be1e Mon Sep 17 00:00:00 2001 From: Zhichao Cao Date: Tue, 2 Jun 2020 15:02:44 -0700 Subject: [PATCH] Fix potential overflow of unsigned type in for loop (#6902) Summary: x.size() -1 or y - 1 can overflow to an extremely large value when x.size() pr y is 0 when they are unsigned type. The end condition of i in the for loop will be extremely large, potentially causes segment fault. Fix them. Pull Request resolved: https://github.com/facebook/rocksdb/pull/6902 Test Plan: pass make asan_check Reviewed By: ajkr Differential Revision: D21843767 Pulled By: zhichao-cao fbshipit-source-id: 5b8b88155ac5a93d86246d832e89905a783bb5a1 --- db/compaction/compaction_job.cc | 2 +- db/compaction/compaction_picker_universal.cc | 4 ++-- db/db_basic_test.cc | 6 +++--- db/db_block_cache_test.cc | 8 ++++---- db/db_flush_test.cc | 4 ++-- db/db_range_del_test.cc | 2 +- db/external_sst_file_ingestion_job.cc | 2 +- db_stress_tool/db_stress_tool.cc | 2 +- env/env_test.cc | 2 +- table/cuckoo/cuckoo_table_builder_test.cc | 2 +- util/coding_test.cc | 4 ++-- utilities/blob_db/blob_dump_tool.cc | 2 +- utilities/transactions/transaction_test.cc | 2 +- utilities/transactions/write_prepared_transaction_test.cc | 2 +- 14 files changed, 22 insertions(+), 22 deletions(-) diff --git a/db/compaction/compaction_job.cc b/db/compaction/compaction_job.cc index 06dd520fc..b9d3fa313 100644 --- a/db/compaction/compaction_job.cc +++ b/db/compaction/compaction_job.cc @@ -542,7 +542,7 @@ void CompactionJob::GenSubcompactionBoundaries() { // Greedily add ranges to the subcompaction until the sum of the ranges' // sizes becomes >= the expected mean size of a subcompaction sum = 0; - for (size_t i = 0; i < ranges.size() - 1; i++) { + for (size_t i = 0; i + 1 < ranges.size(); i++) { sum += ranges[i].size; if (subcompactions == 1) { // If there's only one left to schedule then it goes to the end so no diff --git a/db/compaction/compaction_picker_universal.cc b/db/compaction/compaction_picker_universal.cc index 2cdd5962a..af5c45e5b 100644 --- a/db/compaction/compaction_picker_universal.cc +++ b/db/compaction/compaction_picker_universal.cc @@ -765,7 +765,7 @@ Compaction* UniversalCompactionBuilder::PickCompactionToReduceSizeAmp() { } // Skip files that are already being compacted - for (size_t loop = 0; loop < sorted_runs_.size() - 1; loop++) { + for (size_t loop = 0; loop + 1 < sorted_runs_.size(); loop++) { sr = &sorted_runs_[loop]; if (!sr->being_compacted) { start_index = loop; // Consider this as the first candidate. @@ -793,7 +793,7 @@ Compaction* UniversalCompactionBuilder::PickCompactionToReduceSizeAmp() { } // keep adding up all the remaining files - for (size_t loop = start_index; loop < sorted_runs_.size() - 1; loop++) { + for (size_t loop = start_index; loop + 1 < sorted_runs_.size(); loop++) { sr = &sorted_runs_[loop]; if (sr->being_compacted) { char file_num_buf[kFormatFileNumberBufSize]; diff --git a/db/db_basic_test.cc b/db/db_basic_test.cc index cdb18b66d..4af4f6ee5 100644 --- a/db/db_basic_test.cc +++ b/db/db_basic_test.cc @@ -1900,13 +1900,13 @@ TEST_F(DBBasicTest, GetAllKeyVersions) { ASSERT_EQ(kNumInserts + kNumDeletes + kNumUpdates, key_versions.size()); // Check non-default column family - for (size_t i = 0; i != kNumInserts - 1; ++i) { + for (size_t i = 0; i + 1 != kNumInserts; ++i) { ASSERT_OK(Put(1, std::to_string(i), "value")); } - for (size_t i = 0; i != kNumUpdates - 1; ++i) { + for (size_t i = 0; i + 1 != kNumUpdates; ++i) { ASSERT_OK(Put(1, std::to_string(i), "value1")); } - for (size_t i = 0; i != kNumDeletes - 1; ++i) { + for (size_t i = 0; i + 1 != kNumDeletes; ++i) { ASSERT_OK(Delete(1, std::to_string(i))); } ASSERT_OK(ROCKSDB_NAMESPACE::GetAllKeyVersions( diff --git a/db/db_block_cache_test.cc b/db/db_block_cache_test.cc index d565be09e..0841a655e 100644 --- a/db/db_block_cache_test.cc +++ b/db/db_block_cache_test.cc @@ -187,7 +187,7 @@ TEST_F(DBBlockCacheTest, TestWithoutCompressedBlockCache) { Iterator* iter = nullptr; // Load blocks into cache. - for (size_t i = 0; i < kNumBlocks - 1; i++) { + for (size_t i = 0; i + 1 < kNumBlocks; i++) { iter = db_->NewIterator(read_options); iter->Seek(ToString(i)); ASSERT_OK(iter->status()); @@ -209,12 +209,12 @@ TEST_F(DBBlockCacheTest, TestWithoutCompressedBlockCache) { iter = nullptr; // Release iterators and access cache again. - for (size_t i = 0; i < kNumBlocks - 1; i++) { + for (size_t i = 0; i + 1 < kNumBlocks; i++) { iterators[i].reset(); CheckCacheCounters(options, 0, 0, 0, 0); } ASSERT_EQ(0, cache->GetPinnedUsage()); - for (size_t i = 0; i < kNumBlocks - 1; i++) { + for (size_t i = 0; i + 1 < kNumBlocks; i++) { iter = db_->NewIterator(read_options); iter->Seek(ToString(i)); ASSERT_OK(iter->status()); @@ -243,7 +243,7 @@ TEST_F(DBBlockCacheTest, TestWithCompressedBlockCache) { Iterator* iter = nullptr; // Load blocks into cache. - for (size_t i = 0; i < kNumBlocks - 1; i++) { + for (size_t i = 0; i + 1 < kNumBlocks; i++) { iter = db_->NewIterator(read_options); iter->Seek(ToString(i)); ASSERT_OK(iter->status()); diff --git a/db/db_flush_test.cc b/db/db_flush_test.cc index 45a2c43eb..89c9e7f2e 100644 --- a/db/db_flush_test.cc +++ b/db/db_flush_test.cc @@ -499,13 +499,13 @@ TEST_P(DBAtomicFlushTest, AtomicFlushTriggeredByMemTableFull) { TEST_SYNC_POINT( "DBAtomicFlushTest::AtomicFlushTriggeredByMemTableFull:BeforeCheck"); if (options.atomic_flush) { - for (size_t i = 0; i != num_cfs - 1; ++i) { + for (size_t i = 0; i + 1 != num_cfs; ++i) { auto cfh = static_cast(handles_[i]); ASSERT_EQ(0, cfh->cfd()->imm()->NumNotFlushed()); ASSERT_TRUE(cfh->cfd()->mem()->IsEmpty()); } } else { - for (size_t i = 0; i != num_cfs - 1; ++i) { + for (size_t i = 0; i + 1 != num_cfs; ++i) { auto cfh = static_cast(handles_[i]); ASSERT_EQ(0, cfh->cfd()->imm()->NumNotFlushed()); ASSERT_FALSE(cfh->cfd()->mem()->IsEmpty()); diff --git a/db/db_range_del_test.cc b/db/db_range_del_test.cc index b8421cf3f..83fb27ff3 100644 --- a/db/db_range_del_test.cc +++ b/db/db_range_del_test.cc @@ -189,7 +189,7 @@ TEST_F(DBRangeDelTest, MaxCompactionBytesCutsOutputFiles) { std::vector> files; dbfull()->TEST_GetFilesMetaData(db_->DefaultColumnFamily(), &files); - for (size_t i = 0; i < files[1].size() - 1; ++i) { + for (size_t i = 0; i + 1 < files[1].size(); ++i) { ASSERT_TRUE(InternalKeyComparator(opts.comparator) .Compare(files[1][i].largest, files[1][i + 1].smallest) < 0); diff --git a/db/external_sst_file_ingestion_job.cc b/db/external_sst_file_ingestion_job.cc index b07c7d378..2c5c2804e 100644 --- a/db/external_sst_file_ingestion_job.cc +++ b/db/external_sst_file_ingestion_job.cc @@ -68,7 +68,7 @@ Status ExternalSstFileIngestionJob::Prepare( info2->smallest_internal_key) < 0; }); - for (size_t i = 0; i < num_files - 1; i++) { + for (size_t i = 0; i + 1 < num_files; i++) { if (sstableKeyCompare(ucmp, sorted_files[i]->largest_internal_key, sorted_files[i + 1]->smallest_internal_key) >= 0) { files_overlap_ = true; diff --git a/db_stress_tool/db_stress_tool.cc b/db_stress_tool/db_stress_tool.cc index d7c4fcbac..2b4ef7f8e 100644 --- a/db_stress_tool/db_stress_tool.cc +++ b/db_stress_tool/db_stress_tool.cc @@ -245,7 +245,7 @@ int db_stress_tool(int argc, char** argv) { } } else { uint64_t keys_per_level = key_gen_ctx.window / levels; - for (unsigned int level = 0; level < levels - 1; ++level) { + for (unsigned int level = 0; level + 1 < levels; ++level) { key_gen_ctx.weights.emplace_back(keys_per_level); } key_gen_ctx.weights.emplace_back(key_gen_ctx.window - diff --git a/env/env_test.cc b/env/env_test.cc index 103b2440b..412d7ffc6 100644 --- a/env/env_test.cc +++ b/env/env_test.cc @@ -1242,7 +1242,7 @@ TEST_F(EnvPosixTest, MultiReadNonAlignedLargeNum) { for (int so: start_offsets) { offsets.push_back(so); } - for (size_t i = 0; i < offsets.size() - 1; i++) { + for (size_t i = 0; i + 1 < offsets.size(); i++) { lens.push_back(static_cast(rnd.Uniform(static_cast(offsets[i + 1] - offsets[i])) + 1)); } lens.push_back(static_cast(rnd.Uniform(static_cast(kTotalSize - offsets.back())) + 1)); diff --git a/table/cuckoo/cuckoo_table_builder_test.cc b/table/cuckoo/cuckoo_table_builder_test.cc index fbddded57..fd6aaef1d 100644 --- a/table/cuckoo/cuckoo_table_builder_test.cc +++ b/table/cuckoo/cuckoo_table_builder_test.cc @@ -110,7 +110,7 @@ class CuckooBuilderTest : public testing::Test { // Check contents of the bucket. std::vector keys_found(keys.size(), false); size_t bucket_size = expected_unused_bucket.size(); - for (uint32_t i = 0; i < table_size + cuckoo_block_size - 1; ++i) { + for (uint32_t i = 0; i + 1 < table_size + cuckoo_block_size; ++i) { Slice read_slice; ASSERT_OK(file_reader->Read(IOOptions(), i * bucket_size, bucket_size, &read_slice, nullptr, nullptr)); diff --git a/util/coding_test.cc b/util/coding_test.cc index 383e3f514..ed1d680f7 100644 --- a/util/coding_test.cc +++ b/util/coding_test.cc @@ -161,7 +161,7 @@ TEST(Coding, Varint32Truncation) { std::string s; PutVarint32(&s, large_value); uint32_t result; - for (unsigned int len = 0; len < s.size() - 1; len++) { + for (unsigned int len = 0; len + 1 < s.size(); len++) { ASSERT_TRUE(GetVarint32Ptr(s.data(), s.data() + len, &result) == nullptr); } ASSERT_TRUE( @@ -181,7 +181,7 @@ TEST(Coding, Varint64Truncation) { std::string s; PutVarint64(&s, large_value); uint64_t result; - for (unsigned int len = 0; len < s.size() - 1; len++) { + for (unsigned int len = 0; len + 1 < s.size(); len++) { ASSERT_TRUE(GetVarint64Ptr(s.data(), s.data() + len, &result) == nullptr); } ASSERT_TRUE( diff --git a/utilities/blob_db/blob_dump_tool.cc b/utilities/blob_db/blob_dump_tool.cc index ccaee5845..8f425f730 100644 --- a/utilities/blob_db/blob_dump_tool.cc +++ b/utilities/blob_db/blob_dump_tool.cc @@ -255,7 +255,7 @@ void BlobDumpTool::DumpSlice(const Slice s, DisplayType type) { snprintf(buf + j * 3 + 16, 2, "%x", c & 0xf); snprintf(buf + j + 65, 2, "%c", (0x20 <= c && c <= 0x7e) ? c : '.'); } - for (size_t p = 0; p < sizeof(buf) - 1; p++) { + for (size_t p = 0; p + 1 < sizeof(buf); p++) { if (buf[p] == 0) { buf[p] = ' '; } diff --git a/utilities/transactions/transaction_test.cc b/utilities/transactions/transaction_test.cc index c356f4965..ee8eca9ca 100644 --- a/utilities/transactions/transaction_test.cc +++ b/utilities/transactions/transaction_test.cc @@ -722,7 +722,7 @@ TEST_P(TransactionStressTest, DeadlockCycle) { // We want the last transaction in the chain to block and hold everyone // back. std::vector threads; - for (uint32_t i = 0; i < len - 1; i++) { + for (uint32_t i = 0; i + 1 < len; i++) { std::function blocking_thread = [&, i] { auto s = txns[i]->GetForUpdate(read_options, ToString(i + 1), nullptr); ASSERT_OK(s); diff --git a/utilities/transactions/write_prepared_transaction_test.cc b/utilities/transactions/write_prepared_transaction_test.cc index fd75707d9..f8ee8cd0e 100644 --- a/utilities/transactions/write_prepared_transaction_test.cc +++ b/utilities/transactions/write_prepared_transaction_test.cc @@ -1107,7 +1107,7 @@ TEST_P(WritePreparedTransactionTest, CheckAgainstSnapshots) { 355l, 450l, 455l, 550l, 555l, 650l, 655l, 750l, 755l, 850l, 855l, 950l, 955l}; assert(seqs.size() > 1); - for (size_t i = 0; i < seqs.size() - 1; i++) { + for (size_t i = 0; i + 1 < seqs.size(); i++) { wp_db->old_commit_map_empty_ = true; // reset CommitEntry commit_entry = {seqs[i], seqs[i + 1]}; wp_db->CheckAgainstSnapshots(commit_entry);