From 2bdfc52f975f131e81ddb5becb8111c5addcc7fc Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Fri, 16 Sep 2022 13:46:05 -0700 Subject: [PATCH] Fix heap use after free (#8189) Previously, we had Ref types outliving the arena's that owned them, specifically encryptDomains in the getResolution actor. Refactor to use Standalone's, which both fixes the memory error and makes this easier to reason about. Also fix a potential ODR violation. --- fdbclient/include/fdbclient/GetEncryptCipherKeys.actor.h | 6 +++--- fdbserver/BlobWorker.actor.cpp | 4 ++-- fdbserver/CommitProxyServer.actor.cpp | 6 +++--- flow/EncryptUtils.cpp | 4 ++++ flow/include/flow/EncryptUtils.h | 6 +++--- 5 files changed, 15 insertions(+), 11 deletions(-) diff --git a/fdbclient/include/fdbclient/GetEncryptCipherKeys.actor.h b/fdbclient/include/fdbclient/GetEncryptCipherKeys.actor.h index 1e91659f4a..e82cd363c3 100644 --- a/fdbclient/include/fdbclient/GetEncryptCipherKeys.actor.h +++ b/fdbclient/include/fdbclient/GetEncryptCipherKeys.actor.h @@ -90,7 +90,7 @@ Future getUncachedLatestEncryptCipherKeys(Refer ACTOR template Future>> getLatestEncryptCipherKeys( Reference const> db, - std::unordered_map domains, + std::unordered_map domains, BlobCipherMetrics::UsageType usageType) { state Reference cipherKeyCache = BlobCipherKeyCache::getInstance(); state std::unordered_map> cipherKeys; @@ -265,9 +265,9 @@ struct TextAndHeaderCipherKeys { ACTOR template Future getLatestEncryptCipherKeysForDomain(Reference const> db, EncryptCipherDomainId domainId, - EncryptCipherDomainNameRef domainName, + EncryptCipherDomainName domainName, BlobCipherMetrics::UsageType usageType) { - std::unordered_map domains; + std::unordered_map domains; domains[domainId] = domainName; domains[ENCRYPT_HEADER_DOMAIN_ID] = FDB_ENCRYPT_HEADER_DOMAIN_NAME; std::unordered_map> cipherKeys = diff --git a/fdbserver/BlobWorker.actor.cpp b/fdbserver/BlobWorker.actor.cpp index 0b8567b237..454d26f1d4 100644 --- a/fdbserver/BlobWorker.actor.cpp +++ b/fdbserver/BlobWorker.actor.cpp @@ -357,8 +357,8 @@ ACTOR Future getLatestGranuleCipherKeys(Reference domains; - domains.emplace(tenantData->entry.id, StringRef(*arena, tenantData->name)); + std::unordered_map domains; + domains.emplace(tenantData->entry.id, tenantData->name); std::unordered_map> domainKeyMap = wait(getLatestEncryptCipherKeys(bwData->dbInfo, domains, BlobCipherMetrics::BLOB_GRANULE)); diff --git a/fdbserver/CommitProxyServer.actor.cpp b/fdbserver/CommitProxyServer.actor.cpp index 51719c3177..512bb0e469 100644 --- a/fdbserver/CommitProxyServer.actor.cpp +++ b/fdbserver/CommitProxyServer.actor.cpp @@ -995,19 +995,19 @@ ACTOR Future getResolution(CommitBatchContext* self) { // Fetch cipher keys if needed. state Future>> getCipherKeys; if (pProxyCommitData->isEncryptionEnabled) { - static std::unordered_map defaultDomains = { + static std::unordered_map defaultDomains = { { SYSTEM_KEYSPACE_ENCRYPT_DOMAIN_ID, FDB_SYSTEM_KEYSPACE_ENCRYPT_DOMAIN_NAME }, { ENCRYPT_HEADER_DOMAIN_ID, FDB_ENCRYPT_HEADER_DOMAIN_NAME }, { FDB_DEFAULT_ENCRYPT_DOMAIN_ID, FDB_DEFAULT_ENCRYPT_DOMAIN_NAME } }; - std::unordered_map encryptDomains = defaultDomains; + std::unordered_map encryptDomains = defaultDomains; for (int t = 0; t < trs.size(); t++) { TenantInfo const& tenantInfo = trs[t].tenantInfo; int64_t tenantId = tenantInfo.tenantId; Optional const& tenantName = tenantInfo.name; if (tenantId != TenantInfo::INVALID_TENANT) { ASSERT(tenantName.present()); - encryptDomains[tenantId] = tenantName.get(); + encryptDomains[tenantId] = Standalone(tenantName.get(), tenantInfo.arena); } else { for (auto m : trs[t].transaction.mutations) { std::pair details = diff --git a/flow/EncryptUtils.cpp b/flow/EncryptUtils.cpp index 55ae6c4d1c..ee79aee5c4 100644 --- a/flow/EncryptUtils.cpp +++ b/flow/EncryptUtils.cpp @@ -24,6 +24,10 @@ #include #include +const EncryptCipherDomainName FDB_SYSTEM_KEYSPACE_ENCRYPT_DOMAIN_NAME = "FdbSystemKeyspaceEncryptDomain"_sr; +const EncryptCipherDomainName FDB_DEFAULT_ENCRYPT_DOMAIN_NAME = "FdbDefaultEncryptDomain"_sr; +const EncryptCipherDomainName FDB_ENCRYPT_HEADER_DOMAIN_NAME = "FdbEncryptHeaderDomain"_sr; + EncryptCipherMode encryptModeFromString(const std::string& modeStr) { if (modeStr == "NONE") { return ENCRYPT_CIPHER_MODE_NONE; diff --git a/flow/include/flow/EncryptUtils.h b/flow/include/flow/EncryptUtils.h index 8ac82e6de4..800817d410 100644 --- a/flow/include/flow/EncryptUtils.h +++ b/flow/include/flow/EncryptUtils.h @@ -46,9 +46,9 @@ constexpr const EncryptCipherBaseKeyId INVALID_ENCRYPT_CIPHER_KEY_ID = 0; constexpr const EncryptCipherRandomSalt INVALID_ENCRYPT_RANDOM_SALT = 0; -const EncryptCipherDomainNameRef FDB_SYSTEM_KEYSPACE_ENCRYPT_DOMAIN_NAME = "FdbSystemKeyspaceEncryptDomain"_sr; -const EncryptCipherDomainNameRef FDB_DEFAULT_ENCRYPT_DOMAIN_NAME = "FdbDefaultEncryptDomain"_sr; -const EncryptCipherDomainNameRef FDB_ENCRYPT_HEADER_DOMAIN_NAME = "FdbEncryptHeaderDomain"_sr; +extern const EncryptCipherDomainName FDB_SYSTEM_KEYSPACE_ENCRYPT_DOMAIN_NAME; +extern const EncryptCipherDomainName FDB_DEFAULT_ENCRYPT_DOMAIN_NAME; +extern const EncryptCipherDomainName FDB_ENCRYPT_HEADER_DOMAIN_NAME; typedef enum { ENCRYPT_CIPHER_MODE_NONE = 0,