From 11ee1ad90a6ed5c69ab15862fa1f8557fa0c0b46 Mon Sep 17 00:00:00 2001 From: Kevin Kuehler Date: Wed, 19 Feb 2020 16:30:35 -0800 Subject: [PATCH 1/5] Refactor code assuming buffer is 32 bytes This is a safe assumption to make. https://support.google.com/chrome/a/answer/7532015?hl=en --- extension_whitelist_data.cc | 4 ++-- extension_whitelist_data.h | 41 +++++++++++------------------------ extension_whitelist_parser.cc | 32 ++++++++++----------------- extension_whitelist_parser.h | 1 + 4 files changed, 28 insertions(+), 50 deletions(-) diff --git a/extension_whitelist_data.cc b/extension_whitelist_data.cc index df020a0..30683d3 100644 --- a/extension_whitelist_data.cc +++ b/extension_whitelist_data.cc @@ -8,9 +8,9 @@ static HashFn sHashFn(19); uint64_t ST_EXTENSION_WHITELIST_DATA::GetHash() const { - if (!sExtensionID) { + if ('\0' == *sExtensionID) { return 0; } - return sHashFn(sExtensionID, static_cast(strlen(sExtensionID))); + return sHashFn(sExtensionID, EXTENSION_ID_LEN); } diff --git a/extension_whitelist_data.h b/extension_whitelist_data.h index e7a3bfe..63af11d 100644 --- a/extension_whitelist_data.h +++ b/extension_whitelist_data.h @@ -5,42 +5,35 @@ #ifndef EXTENSION_WHITELIST_DATA_H_ #define EXTENSION_WHITELIST_DATA_H_ +#include #include #include #include +#define EXTENSION_ID_LEN 32 + struct ST_EXTENSION_WHITELIST_DATA { public: ST_EXTENSION_WHITELIST_DATA(): - sExtensionID(nullptr) { + sExtensionID() { + *sExtensionID = '\0'; } ST_EXTENSION_WHITELIST_DATA(const ST_EXTENSION_WHITELIST_DATA &other) { - if (nullptr == other.sExtensionID) { - return; - } - - sExtensionID = new char[strlen(other.sExtensionID) + 1]; - strcpy(sExtensionID, other.sExtensionID); + strncpy(sExtensionID, other.sExtensionID, EXTENSION_ID_LEN + 1); } - ~ST_EXTENSION_WHITELIST_DATA() { - if (nullptr != sExtensionID) { - delete []sExtensionID; - } + ST_EXTENSION_WHITELIST_DATA(const char *other) { + assert(strlen(other) == EXTENSION_ID_LEN); + strncpy(sExtensionID, other, EXTENSION_ID_LEN + 1); } + ~ST_EXTENSION_WHITELIST_DATA() {} + uint64_t GetHash() const; bool operator==(const ST_EXTENSION_WHITELIST_DATA &rhs) const { - int extensionLen = static_cast(strlen(sExtensionID)); - int rhsExtensionIDLen = static_cast(strlen(rhs.sExtensionID)); - - if (extensionLen != rhsExtensionIDLen) { - return false; - } - - return !memcmp(sExtensionID, rhs.sExtensionID, extensionLen); + return !memcmp(sExtensionID, rhs.sExtensionID, EXTENSION_ID_LEN); } // Nothing needs to be updated when an extension is added multiple times @@ -51,7 +44,6 @@ struct ST_EXTENSION_WHITELIST_DATA { char sz[32]; uint32_t dataLenSize = 1 + snprintf(sz, sizeof(sz), "%x", (unsigned int)strlen(sExtensionID)); - if (buffer) { memcpy(buffer + size, sz, dataLenSize); } @@ -73,14 +65,7 @@ struct ST_EXTENSION_WHITELIST_DATA { } unsigned int extensionLength = 0; sscanf(buffer, "%x", &extensionLength); - if (sExtensionID) { - delete []sExtensionID; - } size = static_cast(strlen(buffer) + 1); - sExtensionID = new char[extensionLength + 1]; - if (!sExtensionID) { - return size; - } memcpy(sExtensionID, buffer + size, extensionLength); sExtensionID[extensionLength] = '\0'; size += extensionLength; @@ -88,7 +73,7 @@ struct ST_EXTENSION_WHITELIST_DATA { return size; } - char* sExtensionID; + char sExtensionID[EXTENSION_ID_LEN + 1]; }; #endif // EXTENSION_WHITELIST_DATA_H_ diff --git a/extension_whitelist_parser.cc b/extension_whitelist_parser.cc index f8d8b45..61fa226 100644 --- a/extension_whitelist_parser.cc +++ b/extension_whitelist_parser.cc @@ -14,42 +14,34 @@ ExtensionWhitelistParser::~ExtensionWhitelistParser() { } void ExtensionWhitelistParser::addToBlacklist(const char *extensionID) { - if (nullptr == extensionID) + if (!extensionID || strlen(extensionID) != EXTENSION_ID_LEN) return; - ST_EXTENSION_WHITELIST_DATA extensionData; - extensionData.sExtensionID = new char[strlen(extensionID) + 1]; - if (nullptr == extensionData.sExtensionID) - return; - strcpy(extensionData.sExtensionID, extensionID); + + ST_EXTENSION_WHITELIST_DATA extensionData(extensionID); mBlacklist->Add(extensionData); } void ExtensionWhitelistParser::addToWhitelist(const char *extensionID) { - if (nullptr == extensionID) - return; - ST_EXTENSION_WHITELIST_DATA extensionData; - extensionData.sExtensionID = new char[strlen(extensionID) + 1]; - if (nullptr == extensionData.sExtensionID) + if (!extensionID || strlen(extensionID) != EXTENSION_ID_LEN) return; - strcpy(extensionData.sExtensionID, extensionID); + + ST_EXTENSION_WHITELIST_DATA extensionData(extensionID); mWhitelist->Add(extensionData); } bool ExtensionWhitelistParser::isBlacklisted(const char *extensionID) { - ST_EXTENSION_WHITELIST_DATA extensionData; - extensionData.sExtensionID = new char[strlen(extensionID) + 1]; - if (nullptr == extensionData.sExtensionID) + if (!extensionID || strlen(extensionID) != EXTENSION_ID_LEN) return false; - strcpy(extensionData.sExtensionID, extensionID); + + ST_EXTENSION_WHITELIST_DATA extensionData(extensionID); return mBlacklist->Exists(extensionData); } bool ExtensionWhitelistParser::isWhitelisted(const char *extensionID) { - ST_EXTENSION_WHITELIST_DATA extensionData; - extensionData.sExtensionID = new char[strlen(extensionID) + 1]; - if (nullptr == extensionData.sExtensionID) + if (!extensionID || strlen(extensionID) != EXTENSION_ID_LEN) return false; - strcpy(extensionData.sExtensionID, extensionID); + + ST_EXTENSION_WHITELIST_DATA extensionData(extensionID); return mWhitelist->Exists(extensionData); } diff --git a/extension_whitelist_parser.h b/extension_whitelist_parser.h index d2835c9..3155457 100644 --- a/extension_whitelist_parser.h +++ b/extension_whitelist_parser.h @@ -6,6 +6,7 @@ #define EXTENSION_WHITELIST_PARSER_H_ #include +#include #include "./extension_whitelist_data.h" From 3d484e1312bcc9ec4a0eebb76169f8ccc2483885 Mon Sep 17 00:00:00 2001 From: Kevin Kuehler Date: Wed, 19 Feb 2020 17:40:38 -0800 Subject: [PATCH 2/5] Simplify whitelist data serialization Every extension ID that needs be serialized is 32 bytes. No longer necessary to embed the length of each extension in the serialized buffer. --- extension_whitelist_data.h | 32 +++++++------------------------- 1 file changed, 7 insertions(+), 25 deletions(-) diff --git a/extension_whitelist_data.h b/extension_whitelist_data.h index 63af11d..a2a3b37 100644 --- a/extension_whitelist_data.h +++ b/extension_whitelist_data.h @@ -40,37 +40,19 @@ struct ST_EXTENSION_WHITELIST_DATA { void Update(const ST_EXTENSION_WHITELIST_DATA &) {} uint32_t Serialize(char* buffer) { - uint32_t size = 0; - - char sz[32]; - uint32_t dataLenSize = 1 + snprintf(sz, sizeof(sz), "%x", (unsigned int)strlen(sExtensionID)); - if (buffer) { - memcpy(buffer + size, sz, dataLenSize); - } - size += dataLenSize; - if (buffer) { - memcpy(buffer + size, sExtensionID, strlen(sExtensionID)); + memcpy(buffer, sExtensionID, EXTENSION_ID_LEN); } - size += static_cast(strlen(sExtensionID)); - - return size; + return EXTENSION_ID_LEN; } uint32_t Deserialize(char *buffer, uint32_t bufferSize) { - uint32_t size = 0; - - if (!buffer || 0 == bufferSize) { - return size; + if (!buffer || 0 == buffer) { + return 0; } - unsigned int extensionLength = 0; - sscanf(buffer, "%x", &extensionLength); - size = static_cast(strlen(buffer) + 1); - memcpy(sExtensionID, buffer + size, extensionLength); - sExtensionID[extensionLength] = '\0'; - size += extensionLength; - - return size; + memcpy(sExtensionID, buffer, EXTENSION_ID_LEN); + sExtensionID[EXTENSION_ID_LEN] = '\0'; + return EXTENSION_ID_LEN; } char sExtensionID[EXTENSION_ID_LEN + 1]; From 4aee15558397d1acac34a285f0cc1fde958fe11e Mon Sep 17 00:00:00 2001 From: Kevin Kuehler Date: Wed, 19 Feb 2020 23:55:42 -0800 Subject: [PATCH 3/5] extension_whitelist_parser: safely serialize uint32_t Include inttypes.h to format width-based integral types. This is safer than %x, because %x may read an unbounded amount of characters. --- extension_whitelist_parser.cc | 15 ++++++--------- extension_whitelist_parser.h | 8 ++++++++ 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/extension_whitelist_parser.cc b/extension_whitelist_parser.cc index 61fa226..ad32a08 100644 --- a/extension_whitelist_parser.cc +++ b/extension_whitelist_parser.cc @@ -52,8 +52,8 @@ char* ExtensionWhitelistParser::serialize(unsigned int* totalSize) { char* blacklist = mBlacklist->Serialize(&blacklistSize); uint32_t whitelistSize = 0; char* whitelist = mWhitelist->Serialize(&whitelistSize); - *totalSize = sizeof(blacklistSize) + blacklistSize + 1 + - sizeof(whitelistSize) + whitelistSize + 1; + *totalSize = DECIMAL_STR_MAX(uint32_t) + blacklistSize + 1 + + DECIMAL_STR_MAX(uint32_t) + whitelistSize + 1; unsigned int pos = 0; char* result = new char[*totalSize]; if (!result) { @@ -62,14 +62,11 @@ char* ExtensionWhitelistParser::serialize(unsigned int* totalSize) { return nullptr; } memset(result, 0, *totalSize); - char sz[32]; - uint32_t dataLenSize = 1 + snprintf(sz, sizeof(sz), "%x", blacklistSize); - memcpy(result + pos, sz, dataLenSize); + uint32_t dataLenSize = 1 + sprintf(result + pos, "%" PRIu32, blacklistSize); pos += dataLenSize; memcpy(result + pos, blacklist, blacklistSize); pos += blacklistSize; - dataLenSize = 1 + snprintf(sz, sizeof(sz), "%x", whitelistSize); - memcpy(result + pos, sz, dataLenSize); + dataLenSize = 1 + sprintf(result + pos, "%" PRIu32, whitelistSize); pos += dataLenSize; memcpy(result + pos, whitelist, whitelistSize); delete []blacklist; @@ -86,13 +83,13 @@ bool ExtensionWhitelistParser::deserialize(char *buffer) { return false; uint32_t blacklistSize = 0; unsigned int pos = 0; - sscanf(buffer, "%x", &blacklistSize); + sscanf(buffer, "%" SCNu32, &blacklistSize); pos += static_cast(strlen(buffer) + 1); if (!mBlacklist->Deserialize(buffer + pos, blacklistSize)) return false; pos += blacklistSize; uint32_t whitelistSize = 0; - sscanf(buffer + pos, "%x", &whitelistSize); + sscanf(buffer + pos, "%" SCNu32, &whitelistSize); pos += static_cast(strlen(buffer + pos) + 1); if (!mWhitelist->Deserialize(buffer + pos, whitelistSize)) { return false; diff --git a/extension_whitelist_parser.h b/extension_whitelist_parser.h index 3155457..af32e13 100644 --- a/extension_whitelist_parser.h +++ b/extension_whitelist_parser.h @@ -6,6 +6,7 @@ #define EXTENSION_WHITELIST_PARSER_H_ #include +#include #include #include "./extension_whitelist_data.h" @@ -13,6 +14,13 @@ #define EXTENSION_DAT_FILE "ExtensionWhitelist.dat" #define EXTENSION_DAT_FILE_VERSION "1" +/* Taken from systemd, src/basic/macro.h */ +#define DECIMAL_STR_MAX(type) \ + (2+(sizeof(type) <= 1 ? 3 : \ + sizeof(type) <= 2 ? 5 : \ + sizeof(type) <= 4 ? 10 : \ + sizeof(type) <= 8 ? 20 : sizeof(int[-2*(sizeof(type) > 8)]))) + template class HashSet; From ec8ed6e27d75e7aad4d36cf882c957b946519dc7 Mon Sep 17 00:00:00 2001 From: Kevin Kuehler Date: Sun, 5 Apr 2020 01:59:41 -0700 Subject: [PATCH 4/5] Refactor with std functions * Replace ST_EXTENSION_WHITELIST_DATA with std::string * Replace hash-set.cpp with std::unordered_set --- binding.gyp | 13 +++----- brave/BUILD.gn | 5 ++- extension_set.cc | 40 +++++++++++++++++++++++ extension_set.h | 28 ++++++++++++++++ extension_whitelist_data.cc | 16 --------- extension_whitelist_data.h | 61 ----------------------------------- extension_whitelist_parser.cc | 22 ++++++------- extension_whitelist_parser.h | 11 ++++--- package-lock.json | 5 --- package.json | 2 -- 10 files changed, 89 insertions(+), 114 deletions(-) create mode 100644 extension_set.cc create mode 100644 extension_set.h delete mode 100644 extension_whitelist_data.cc delete mode 100644 extension_whitelist_data.h diff --git a/binding.gyp b/binding.gyp index c8ed28e..cd0cceb 100644 --- a/binding.gyp +++ b/binding.gyp @@ -5,15 +5,13 @@ "sources": [ "extension_whitelist_parser.cc", "extension_whitelist_parser.h", - "extension_whitelist_data.cc", - "extension_whitelist_data.h", + "extension_set.cc", + "extension_set.h", ], "include_dirs": [ ".", - './node_modules/hashset-cpp' ], "dependencies": [ - "./node_modules/hashset-cpp/binding.gyp:hashset-cpp" ], "conditions": [ ['OS=="win"', { @@ -34,17 +32,14 @@ "sources": [ "extension_whitelist_parser.cc", "extension_whitelist_parser.h", - "extension_whitelist_data.cc", - "extension_whitelist_data.h", + "extension_set.cc", + "extension_set.h", "./node_addon/EWParserWrap.h", "./node_addon/EWParserWrap.cc", "./node_addon/addon.cpp", - "./node_modules/hashset-cpp/hashFn.cc", - "./node_modules/hashset-cpp/hashFn.h" ], "include_dirs": [ ".", - './node_modules/hashset-cpp' ], "conditions": [ ['OS=="win"', { diff --git a/brave/BUILD.gn b/brave/BUILD.gn index 66b0765..d6e171b 100644 --- a/brave/BUILD.gn +++ b/brave/BUILD.gn @@ -18,11 +18,10 @@ source_set("extension-whitelist") { sources = [ "../extension_whitelist_parser.cc", "../extension_whitelist_parser.h", - "../extension_whitelist_data.cc", - "../extension_whitelist_data.h", + "../extension_set.cc", + "../extension_set.h", ] deps = [ - rebase_path("hashset-cpp/brave:hashset-cpp", dep_base), ] } diff --git a/extension_set.cc b/extension_set.cc new file mode 100644 index 0000000..20025e8 --- /dev/null +++ b/extension_set.cc @@ -0,0 +1,40 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#include "extension_set.h" +#include + +ExtensionSet::ExtensionSet() {} +ExtensionSet::~ExtensionSet() {} + +void ExtensionSet::Add(std::string key) { + if (key.length() != EXTENSION_ID_LEN) { + throw std::runtime_error("Invalid extension length given!"); + } + set.insert(key); +} + +bool ExtensionSet::Exists(std::string key) { + return set.find(key) != set.end(); +} + +char* ExtensionSet::Serialize(uint32_t* size) { + *size = set.size() * EXTENSION_ID_LEN; + char* buffer = new char[*size]; + size_t offset = 0; + for (std::string e : set) { + memcpy(buffer + offset, e.c_str(), EXTENSION_ID_LEN); + offset += EXTENSION_ID_LEN; + } + return buffer; +} + +void ExtensionSet::Deserialize(char* buffer, uint32_t size) { + set.clear(); + for (uint32_t i = 0; i < size; i++) { + std::string s(buffer, EXTENSION_ID_LEN); + set.insert(s); + buffer += EXTENSION_ID_LEN; + } +} diff --git a/extension_set.h b/extension_set.h new file mode 100644 index 0000000..f857bf8 --- /dev/null +++ b/extension_set.h @@ -0,0 +1,28 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#ifndef EXTENSION_SET_H_ +#define EXTENSION_SET_H_ + +#include +#include +#include +#include +#include + +#define EXTENSION_ID_LEN 32 + +class ExtensionSet { + private: + std::unordered_set set; + public: + ExtensionSet(); + ~ExtensionSet(); + void Add(std::string); + bool Exists(std::string); + char* Serialize(uint32_t* size); + void Deserialize(char* buffer, uint32_t size); +}; + +#endif // EXTENSION_SET_H_ diff --git a/extension_whitelist_data.cc b/extension_whitelist_data.cc deleted file mode 100644 index 30683d3..0000000 --- a/extension_whitelist_data.cc +++ /dev/null @@ -1,16 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -#include "./extension_whitelist_data.h" -#include "hashFn.h" - -static HashFn sHashFn(19); - -uint64_t ST_EXTENSION_WHITELIST_DATA::GetHash() const { - if ('\0' == *sExtensionID) { - return 0; - } - - return sHashFn(sExtensionID, EXTENSION_ID_LEN); -} diff --git a/extension_whitelist_data.h b/extension_whitelist_data.h deleted file mode 100644 index a2a3b37..0000000 --- a/extension_whitelist_data.h +++ /dev/null @@ -1,61 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -#ifndef EXTENSION_WHITELIST_DATA_H_ -#define EXTENSION_WHITELIST_DATA_H_ - -#include -#include -#include -#include - -#define EXTENSION_ID_LEN 32 - -struct ST_EXTENSION_WHITELIST_DATA { -public: - ST_EXTENSION_WHITELIST_DATA(): - sExtensionID() { - *sExtensionID = '\0'; - } - - ST_EXTENSION_WHITELIST_DATA(const ST_EXTENSION_WHITELIST_DATA &other) { - strncpy(sExtensionID, other.sExtensionID, EXTENSION_ID_LEN + 1); - } - - ST_EXTENSION_WHITELIST_DATA(const char *other) { - assert(strlen(other) == EXTENSION_ID_LEN); - strncpy(sExtensionID, other, EXTENSION_ID_LEN + 1); - } - - ~ST_EXTENSION_WHITELIST_DATA() {} - - uint64_t GetHash() const; - - bool operator==(const ST_EXTENSION_WHITELIST_DATA &rhs) const { - return !memcmp(sExtensionID, rhs.sExtensionID, EXTENSION_ID_LEN); - } - - // Nothing needs to be updated when an extension is added multiple times - void Update(const ST_EXTENSION_WHITELIST_DATA &) {} - - uint32_t Serialize(char* buffer) { - if (buffer) { - memcpy(buffer, sExtensionID, EXTENSION_ID_LEN); - } - return EXTENSION_ID_LEN; - } - - uint32_t Deserialize(char *buffer, uint32_t bufferSize) { - if (!buffer || 0 == buffer) { - return 0; - } - memcpy(sExtensionID, buffer, EXTENSION_ID_LEN); - sExtensionID[EXTENSION_ID_LEN] = '\0'; - return EXTENSION_ID_LEN; - } - - char sExtensionID[EXTENSION_ID_LEN + 1]; -}; - -#endif // EXTENSION_WHITELIST_DATA_H_ diff --git a/extension_whitelist_parser.cc b/extension_whitelist_parser.cc index ad32a08..1cfae49 100644 --- a/extension_whitelist_parser.cc +++ b/extension_whitelist_parser.cc @@ -2,12 +2,11 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -#include "./extension_whitelist_parser.h" -#include "hash_set.h" +#include "extension_whitelist_parser.h" ExtensionWhitelistParser::ExtensionWhitelistParser() { - mBlacklist.reset(new HashSet(256, false)); - mWhitelist.reset(new HashSet(256, false)); + mBlacklist = std::make_unique(); + mWhitelist = std::make_unique(); } ExtensionWhitelistParser::~ExtensionWhitelistParser() { @@ -17,7 +16,7 @@ void ExtensionWhitelistParser::addToBlacklist(const char *extensionID) { if (!extensionID || strlen(extensionID) != EXTENSION_ID_LEN) return; - ST_EXTENSION_WHITELIST_DATA extensionData(extensionID); + std::string extensionData(extensionID); mBlacklist->Add(extensionData); } @@ -25,7 +24,7 @@ void ExtensionWhitelistParser::addToWhitelist(const char *extensionID) { if (!extensionID || strlen(extensionID) != EXTENSION_ID_LEN) return; - ST_EXTENSION_WHITELIST_DATA extensionData(extensionID); + std::string extensionData(extensionID); mWhitelist->Add(extensionData); } @@ -33,7 +32,7 @@ bool ExtensionWhitelistParser::isBlacklisted(const char *extensionID) { if (!extensionID || strlen(extensionID) != EXTENSION_ID_LEN) return false; - ST_EXTENSION_WHITELIST_DATA extensionData(extensionID); + std::string extensionData(extensionID); return mBlacklist->Exists(extensionData); } @@ -41,7 +40,7 @@ bool ExtensionWhitelistParser::isWhitelisted(const char *extensionID) { if (!extensionID || strlen(extensionID) != EXTENSION_ID_LEN) return false; - ST_EXTENSION_WHITELIST_DATA extensionData(extensionID); + std::string extensionData(extensionID); return mWhitelist->Exists(extensionData); } @@ -85,14 +84,11 @@ bool ExtensionWhitelistParser::deserialize(char *buffer) { unsigned int pos = 0; sscanf(buffer, "%" SCNu32, &blacklistSize); pos += static_cast(strlen(buffer) + 1); - if (!mBlacklist->Deserialize(buffer + pos, blacklistSize)) - return false; + mBlacklist->Deserialize(buffer + pos, blacklistSize); pos += blacklistSize; uint32_t whitelistSize = 0; sscanf(buffer + pos, "%" SCNu32, &whitelistSize); pos += static_cast(strlen(buffer + pos) + 1); - if (!mWhitelist->Deserialize(buffer + pos, whitelistSize)) { - return false; - } + mWhitelist->Deserialize(buffer + pos, whitelistSize); return true; } diff --git a/extension_whitelist_parser.h b/extension_whitelist_parser.h index af32e13..0162d5c 100644 --- a/extension_whitelist_parser.h +++ b/extension_whitelist_parser.h @@ -5,11 +5,12 @@ #ifndef EXTENSION_WHITELIST_PARSER_H_ #define EXTENSION_WHITELIST_PARSER_H_ -#include -#include #include +#include +#include +#include -#include "./extension_whitelist_data.h" +#include "extension_set.h" #define EXTENSION_DAT_FILE "ExtensionWhitelist.dat" #define EXTENSION_DAT_FILE_VERSION "1" @@ -44,8 +45,8 @@ class ExtensionWhitelistParser { bool deserialize(char *buffer, size_t); private: - std::unique_ptr > mBlacklist; - std::unique_ptr > mWhitelist; + std::unique_ptr mBlacklist; + std::unique_ptr mWhitelist; }; #endif // EXTENSION_WHITELIST_PARSER_H_ diff --git a/package-lock.json b/package-lock.json index fd8c83a..d0622be 100644 --- a/package-lock.json +++ b/package-lock.json @@ -586,11 +586,6 @@ "resolved": "https://registry.npmjs.org/has-unicode/-/has-unicode-2.0.1.tgz", "integrity": "sha1-4Ob+aijPUROIVeCG0Wkedx3iqLk=" }, - "hashset-cpp": { - "version": "2.2.1", - "resolved": "https://registry.npmjs.org/hashset-cpp/-/hashset-cpp-2.2.1.tgz", - "integrity": "sha512-pO16ApkT0SEtXmCy9FgcwwPnTUbdRzL000gcpOtJyIY3OOSYvh4eQaZdYjlU+Y1AOzy3am6msgHZZ5Yok19aLw==" - }, "he": { "version": "1.2.0", "resolved": "https://registry.npmjs.org/he/-/he-1.2.0.tgz", diff --git a/package.json b/package.json index 63c708f..e35bca0 100644 --- a/package.json +++ b/package.json @@ -8,7 +8,6 @@ }, "scripts": { "install": "node-gyp rebuild", - "preinstall": "npm install hashset-cpp", "data-files": "node ./scripts/gen_data_file.js", "test": "mocha" }, @@ -24,7 +23,6 @@ }, "homepage": "https://github.com/brave/extension-whitelist#readme", "dependencies": { - "hashset-cpp": "^2.2.1", "mkdirp": "^1.0.3", "node-gyp": "^5.0.3" }, From 752fd7bd59c1a2754f9931431a90ebfe71fd882f Mon Sep 17 00:00:00 2001 From: Kevin Kuehler Date: Tue, 21 Apr 2020 14:47:47 -0700 Subject: [PATCH 5/5] Review fixups * Use strings where possible * Replace DECIMAL_STRING_MAX WITH UINT32_SERIALIZE_MAX since we are only serializing uint32_t * Remove extension_set class and make Serialize and Deserialize free functions --- binding.gyp | 4 -- brave/BUILD.gn | 2 - extension_set.cc | 40 -------------- extension_set.h | 28 ---------- extension_whitelist_parser.cc | 100 ++++++++++++++++++++++------------ extension_whitelist_parser.h | 25 ++++----- 6 files changed, 76 insertions(+), 123 deletions(-) delete mode 100644 extension_set.cc delete mode 100644 extension_set.h diff --git a/binding.gyp b/binding.gyp index cd0cceb..5936db2 100644 --- a/binding.gyp +++ b/binding.gyp @@ -5,8 +5,6 @@ "sources": [ "extension_whitelist_parser.cc", "extension_whitelist_parser.h", - "extension_set.cc", - "extension_set.h", ], "include_dirs": [ ".", @@ -32,8 +30,6 @@ "sources": [ "extension_whitelist_parser.cc", "extension_whitelist_parser.h", - "extension_set.cc", - "extension_set.h", "./node_addon/EWParserWrap.h", "./node_addon/EWParserWrap.cc", "./node_addon/addon.cpp", diff --git a/brave/BUILD.gn b/brave/BUILD.gn index d6e171b..95b4c67 100644 --- a/brave/BUILD.gn +++ b/brave/BUILD.gn @@ -18,8 +18,6 @@ source_set("extension-whitelist") { sources = [ "../extension_whitelist_parser.cc", "../extension_whitelist_parser.h", - "../extension_set.cc", - "../extension_set.h", ] deps = [ diff --git a/extension_set.cc b/extension_set.cc deleted file mode 100644 index 20025e8..0000000 --- a/extension_set.cc +++ /dev/null @@ -1,40 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -#include "extension_set.h" -#include - -ExtensionSet::ExtensionSet() {} -ExtensionSet::~ExtensionSet() {} - -void ExtensionSet::Add(std::string key) { - if (key.length() != EXTENSION_ID_LEN) { - throw std::runtime_error("Invalid extension length given!"); - } - set.insert(key); -} - -bool ExtensionSet::Exists(std::string key) { - return set.find(key) != set.end(); -} - -char* ExtensionSet::Serialize(uint32_t* size) { - *size = set.size() * EXTENSION_ID_LEN; - char* buffer = new char[*size]; - size_t offset = 0; - for (std::string e : set) { - memcpy(buffer + offset, e.c_str(), EXTENSION_ID_LEN); - offset += EXTENSION_ID_LEN; - } - return buffer; -} - -void ExtensionSet::Deserialize(char* buffer, uint32_t size) { - set.clear(); - for (uint32_t i = 0; i < size; i++) { - std::string s(buffer, EXTENSION_ID_LEN); - set.insert(s); - buffer += EXTENSION_ID_LEN; - } -} diff --git a/extension_set.h b/extension_set.h deleted file mode 100644 index f857bf8..0000000 --- a/extension_set.h +++ /dev/null @@ -1,28 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -#ifndef EXTENSION_SET_H_ -#define EXTENSION_SET_H_ - -#include -#include -#include -#include -#include - -#define EXTENSION_ID_LEN 32 - -class ExtensionSet { - private: - std::unordered_set set; - public: - ExtensionSet(); - ~ExtensionSet(); - void Add(std::string); - bool Exists(std::string); - char* Serialize(uint32_t* size); - void Deserialize(char* buffer, uint32_t size); -}; - -#endif // EXTENSION_SET_H_ diff --git a/extension_whitelist_parser.cc b/extension_whitelist_parser.cc index 1cfae49..5509adc 100644 --- a/extension_whitelist_parser.cc +++ b/extension_whitelist_parser.cc @@ -3,21 +3,44 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ #include "extension_whitelist_parser.h" +#include +#include -ExtensionWhitelistParser::ExtensionWhitelistParser() { - mBlacklist = std::make_unique(); - mWhitelist = std::make_unique(); +namespace { +std::string Serialize(std::unordered_set& set, uint32_t* size) { + *size = set.size() * EXTENSION_ID_LEN; + std::string buffer; + size_t offset = 0; + for (std::string e : set) + buffer += e; + + return buffer; } -ExtensionWhitelistParser::~ExtensionWhitelistParser() { +void Deserialize(std::unordered_set& set, const char* buffer, uint32_t size) { + set.clear(); + const char *p = buffer; + for (uint32_t i = 0; i < size / EXTENSION_ID_LEN; i++) { + std::string s(p, EXTENSION_ID_LEN); + set.insert(s); + p += EXTENSION_ID_LEN; + } } +} + +ExtensionWhitelistParser::ExtensionWhitelistParser() {} + +ExtensionWhitelistParser::~ExtensionWhitelistParser() {} void ExtensionWhitelistParser::addToBlacklist(const char *extensionID) { if (!extensionID || strlen(extensionID) != EXTENSION_ID_LEN) return; std::string extensionData(extensionID); - mBlacklist->Add(extensionData); + if (extensionData.length() != EXTENSION_ID_LEN) { + throw std::runtime_error("Invalid extension length given!"); + } + mBlacklist.insert(extensionData); } void ExtensionWhitelistParser::addToWhitelist(const char *extensionID) { @@ -25,7 +48,10 @@ void ExtensionWhitelistParser::addToWhitelist(const char *extensionID) { return; std::string extensionData(extensionID); - mWhitelist->Add(extensionData); + if (extensionData.length() != EXTENSION_ID_LEN) { + throw std::runtime_error("Invalid extension length given!"); + } + mWhitelist.insert(extensionData); } bool ExtensionWhitelistParser::isBlacklisted(const char *extensionID) { @@ -33,7 +59,7 @@ bool ExtensionWhitelistParser::isBlacklisted(const char *extensionID) { return false; std::string extensionData(extensionID); - return mBlacklist->Exists(extensionData); + return mBlacklist.find(extensionData) != mBlacklist.end(); } bool ExtensionWhitelistParser::isWhitelisted(const char *extensionID) { @@ -41,54 +67,58 @@ bool ExtensionWhitelistParser::isWhitelisted(const char *extensionID) { return false; std::string extensionData(extensionID); - return mWhitelist->Exists(extensionData); + return mWhitelist.find(extensionData) != mWhitelist.end(); } // Returns a newly allocated buffer, caller must manually delete[] the buffer char* ExtensionWhitelistParser::serialize(unsigned int* totalSize) { *totalSize = 0; uint32_t blacklistSize = 0; - char* blacklist = mBlacklist->Serialize(&blacklistSize); + std::string blacklist = ::Serialize(mBlacklist, &blacklistSize); uint32_t whitelistSize = 0; - char* whitelist = mWhitelist->Serialize(&whitelistSize); - *totalSize = DECIMAL_STR_MAX(uint32_t) + blacklistSize + 1 + - DECIMAL_STR_MAX(uint32_t) + whitelistSize + 1; + std::string whitelist = ::Serialize(mWhitelist, &whitelistSize); + *totalSize = UINT32_SERIALIZE_MAX + blacklistSize + 1 + + UINT32_SERIALIZE_MAX + whitelistSize + 1; unsigned int pos = 0; char* result = new char[*totalSize]; - if (!result) { - delete []blacklist; - delete []whitelist; - return nullptr; - } memset(result, 0, *totalSize); uint32_t dataLenSize = 1 + sprintf(result + pos, "%" PRIu32, blacklistSize); pos += dataLenSize; - memcpy(result + pos, blacklist, blacklistSize); - pos += blacklistSize; + memcpy(result + pos, blacklist.c_str(), blacklistSize); + pos += 1 + blacklistSize; dataLenSize = 1 + sprintf(result + pos, "%" PRIu32, whitelistSize); pos += dataLenSize; - memcpy(result + pos, whitelist, whitelistSize); - delete []blacklist; - delete []whitelist; - return result; + memcpy(result + pos, whitelist.c_str(), whitelistSize); + return result; } -bool ExtensionWhitelistParser::deserialize(char *buffer, size_t) { +bool ExtensionWhitelistParser::deserialize(const char* buffer, size_t) { return deserialize(buffer); } -bool ExtensionWhitelistParser::deserialize(char *buffer) { +bool ExtensionWhitelistParser::deserialize(const char* buffer) { if (!buffer) return false; - uint32_t blacklistSize = 0; - unsigned int pos = 0; - sscanf(buffer, "%" SCNu32, &blacklistSize); - pos += static_cast(strlen(buffer) + 1); - mBlacklist->Deserialize(buffer + pos, blacklistSize); - pos += blacklistSize; - uint32_t whitelistSize = 0; - sscanf(buffer + pos, "%" SCNu32, &whitelistSize); - pos += static_cast(strlen(buffer + pos) + 1); - mWhitelist->Deserialize(buffer + pos, whitelistSize); + const char *p = buffer; + uint32_t blacklistSize = 0, whitelistSize = 0; + int x, r; + + r = sscanf(p, "%" SCNu32 "%n", &blacklistSize, &x); + if (r != 1 || r == EOF) + return false; + if (blacklistSize % EXTENSION_ID_LEN != 0) + throw std::runtime_error("deserialize: blacklist not divisible by 32!"); + p += x + 1; + ::Deserialize(mBlacklist, p, blacklistSize); + p += blacklistSize + 1; + + r = sscanf(p, "%" SCNu32 "%n", &whitelistSize, &x); + if (r != 1 || r == EOF) + return false; + if (whitelistSize % EXTENSION_ID_LEN != 0) + throw std::runtime_error("deserialize: whitelist not divisible by 32!"); + p += x + 1; + ::Deserialize(mWhitelist, p, whitelistSize); + return true; } diff --git a/extension_whitelist_parser.h b/extension_whitelist_parser.h index 0162d5c..9ea1148 100644 --- a/extension_whitelist_parser.h +++ b/extension_whitelist_parser.h @@ -6,24 +6,21 @@ #define EXTENSION_WHITELIST_PARSER_H_ #include +#include #include #include #include -#include "extension_set.h" - #define EXTENSION_DAT_FILE "ExtensionWhitelist.dat" #define EXTENSION_DAT_FILE_VERSION "1" +#define EXTENSION_ID_LEN 32 -/* Taken from systemd, src/basic/macro.h */ -#define DECIMAL_STR_MAX(type) \ - (2+(sizeof(type) <= 1 ? 3 : \ - sizeof(type) <= 2 ? 5 : \ - sizeof(type) <= 4 ? 10 : \ - sizeof(type) <= 8 ? 20 : sizeof(int[-2*(sizeof(type) > 8)]))) +#define UINT32_SERIALIZE_MAX 10 -template -class HashSet; +namespace { +std::string Serialize(std::unordered_set& set, uint32_t* size); +void Deserialize(std::unordered_set& set, const char* buffer, uint32_t size); +} class ExtensionWhitelistParser { public: @@ -41,12 +38,12 @@ class ExtensionWhitelistParser { // Deserializes the buffer, a size is not needed since a serialized // buffer is self described - bool deserialize(char *buffer); - bool deserialize(char *buffer, size_t); + bool deserialize(const char* buffer); + bool deserialize(const char* buffer, size_t); private: - std::unique_ptr mBlacklist; - std::unique_ptr mWhitelist; + std::unordered_set mBlacklist; + std::unordered_set mWhitelist; }; #endif // EXTENSION_WHITELIST_PARSER_H_