From 20c7d7c58a57931b2a3e50c931f7a91f82385ff7 Mon Sep 17 00:00:00 2001 From: Akanksha Mahajan Date: Mon, 7 Dec 2020 13:42:27 -0800 Subject: [PATCH] Handling misuse of snprintf return value (#7686) Summary: Handle misuse of snprintf return value to avoid Out of bound read/write. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7686 Test Plan: make check -j64 Reviewed By: riversand963 Differential Revision: D25030831 Pulled By: akankshamahajan15 fbshipit-source-id: 1a1d181c067c78b94d720323ae00b79566b57cfa --- db/internal_stats.cc | 2 + utilities/backupable/backupable_db.cc | 63 ++++----------------------- utilities/blob_db/blob_db_impl.cc | 29 +++++------- 3 files changed, 23 insertions(+), 71 deletions(-) diff --git a/db/internal_stats.cc b/db/internal_stats.cc index 87ed9c60f..7537abdd2 100644 --- a/db/internal_stats.cc +++ b/db/internal_stats.cc @@ -61,6 +61,7 @@ void PrintLevelStatsHeader(char* buf, size_t len, const std::string& cf_name, const std::string& group_by) { int written_size = snprintf(buf, len, "\n** Compaction Stats [%s] **\n", cf_name.c_str()); + written_size = std::min(written_size, static_cast(len)); auto hdr = [](LevelStatType t) { return InternalStats::compaction_level_stats.at(t).header_name.c_str(); }; @@ -80,6 +81,7 @@ void PrintLevelStatsHeader(char* buf, size_t len, const std::string& cf_name, hdr(LevelStatType::KEY_DROP)); written_size += line_size; + written_size = std::min(written_size, static_cast(len)); snprintf(buf + written_size, len - written_size, "%s\n", std::string(line_size, '-').c_str()); } diff --git a/utilities/backupable/backupable_db.cc b/utilities/backupable/backupable_db.cc index 2d21234c1..885038d1e 100644 --- a/utilities/backupable/backupable_db.cc +++ b/utilities/backupable/backupable_db.cc @@ -2245,70 +2245,25 @@ Status BackupEngineImpl::BackupMeta::StoreToFile(bool sync) { return s; } - std::unique_ptr buf(new char[max_backup_meta_file_size_]); - size_t len = 0, buf_size = max_backup_meta_file_size_; - len += snprintf(buf.get(), buf_size, "%" PRId64 "\n", timestamp_); - len += snprintf(buf.get() + len, buf_size - len, "%" PRIu64 "\n", - sequence_number_); + std::ostringstream buf; + buf << timestamp_ << "\n"; + buf << sequence_number_ << "\n"; + if (!app_metadata_.empty()) { std::string hex_encoded_metadata = Slice(app_metadata_).ToString(/* hex */ true); - - // +1 to accommodate newline character - size_t hex_meta_strlen = - kMetaDataPrefix.ToString().length() + hex_encoded_metadata.length() + 1; - if (hex_meta_strlen >= buf_size) { - return Status::Corruption("Buffer too small to fit backup metadata"); - } - else if (len + hex_meta_strlen >= buf_size) { - backup_meta_file->Append(Slice(buf.get(), len)); - buf.reset(); - std::unique_ptr new_reset_buf( - new char[max_backup_meta_file_size_]); - buf.swap(new_reset_buf); - len = 0; - } - len += snprintf(buf.get() + len, buf_size - len, "%s%s\n", - kMetaDataPrefix.ToString().c_str(), - hex_encoded_metadata.c_str()); - } - - char writelen_temp[19]; - if (len + snprintf(writelen_temp, sizeof(writelen_temp), - "%" ROCKSDB_PRIszt "\n", files_.size()) >= buf_size) { - backup_meta_file->Append(Slice(buf.get(), len)); - buf.reset(); - std::unique_ptr new_reset_buf(new char[max_backup_meta_file_size_]); - buf.swap(new_reset_buf); - len = 0; - } - { - const char *const_write = writelen_temp; - len += snprintf(buf.get() + len, buf_size - len, "%s", const_write); + buf << kMetaDataPrefix.ToString() << hex_encoded_metadata << "\n"; } + buf << files_.size() << "\n"; for (const auto& file : files_) { // use crc32c for now, switch to something else if needed // WART: The checksums are crc32c, not original crc32 - - size_t newlen = - len + file->filename.length() + - snprintf(writelen_temp, sizeof(writelen_temp), " crc32 %u\n", - ChecksumHexToInt32(file->checksum_hex)); - const char* const_write = writelen_temp; - if (newlen >= buf_size) { - backup_meta_file->Append(Slice(buf.get(), len)); - buf.reset(); - std::unique_ptr new_reset_buf( - new char[max_backup_meta_file_size_]); - buf.swap(new_reset_buf); - len = 0; - } - len += snprintf(buf.get() + len, buf_size - len, "%s%s", - file->filename.c_str(), const_write); + buf << file->filename << " crc32 " << ChecksumHexToInt32(file->checksum_hex) + << "\n"; } - s = backup_meta_file->Append(Slice(buf.get(), len)); + s = backup_meta_file->Append(Slice(buf.str())); if (s.ok() && sync) { s = backup_meta_file->Sync(); } diff --git a/utilities/blob_db/blob_db_impl.cc b/utilities/blob_db/blob_db_impl.cc index 6f904f22b..d8e77316e 100644 --- a/utilities/blob_db/blob_db_impl.cc +++ b/utilities/blob_db/blob_db_impl.cc @@ -1686,35 +1686,30 @@ std::pair BlobDBImpl::SanityCheck(bool aborted) { for (auto blob_file_pair : blob_files_) { auto blob_file = blob_file_pair.second; - char buf[1000]; - int pos = snprintf(buf, sizeof(buf), - "Blob file %" PRIu64 ", size %" PRIu64 - ", blob count %" PRIu64 ", immutable %d", - blob_file->BlobFileNumber(), blob_file->GetFileSize(), - blob_file->BlobCount(), blob_file->Immutable()); + std::ostringstream buf; + + buf << "Blob file " << blob_file->BlobFileNumber() << ", size " + << blob_file->GetFileSize() << ", blob count " << blob_file->BlobCount() + << ", immutable " << blob_file->Immutable(); + if (blob_file->HasTTL()) { ExpirationRange expiration_range; - { ReadLock file_lock(&blob_file->mutex_); expiration_range = blob_file->GetExpirationRange(); } + buf << ", expiration range (" << expiration_range.first << ", " + << expiration_range.second << ")"; - pos += snprintf(buf + pos, sizeof(buf) - pos, - ", expiration range (%" PRIu64 ", %" PRIu64 ")", - expiration_range.first, expiration_range.second); if (!blob_file->Obsolete()) { - pos += snprintf(buf + pos, sizeof(buf) - pos, - ", expire in %" PRIu64 " seconds", - expiration_range.second - now); + buf << ", expire in " << (expiration_range.second - now) << "seconds"; } } if (blob_file->Obsolete()) { - pos += snprintf(buf + pos, sizeof(buf) - pos, ", obsolete at %" PRIu64, - blob_file->GetObsoleteSequence()); + buf << ", obsolete at " << blob_file->GetObsoleteSequence(); } - snprintf(buf + pos, sizeof(buf) - pos, "."); - ROCKS_LOG_INFO(db_options_.info_log, "%s", buf); + buf << "."; + ROCKS_LOG_INFO(db_options_.info_log, "%s", buf.str().c_str()); } // reschedule -- GitLab