From 449029f865171507130290a9a4886e890f3cc39c Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Wed, 26 Jan 2022 16:06:46 -0800 Subject: [PATCH] Remove deprecated ObjectLibrary::Register() (and Regex public API) (#9439) Summary: Regexes are considered potentially problematic for use in registering RocksDB extensions, so we are removing ObjectLibrary::Register() and the Regex public API it depended on (now unused). In reference to https://github.com/facebook/rocksdb/issues/9389 Why? * The power of Regexes can make it hard to reason about which extension will match what. (The replacement API isn't perfect, but we are at least "holding the line" on patterns we have seen in practice.) * It is easy to make regexes that don't quite mean what you think they mean, such as forgetting that the `.` in `foo.bar` can match any character or that matching is nondeterministic, as in `a:b:42` matching `.*:[0-9]+`. * Some regexes and implementations can have disastrously bad performance. This might not be much practical concern for ObjectLibray here, but we don't want to encourage potentially dangerous further use in production code. (Testing code is fine. See TestRegex.) Pull Request resolved: https://github.com/facebook/rocksdb/pull/9439 Test Plan: CI Reviewed By: mrambacher Differential Revision: D33792342 Pulled By: pdillinger fbshipit-source-id: 4f64dcb04764e639162c8977a5fa196f67754cec --- CMakeLists.txt | 1 - HISTORY.md | 1 + TARGETS | 2 - include/rocksdb/utilities/object_registry.h | 41 +---------------- include/rocksdb/utilities/regex.h | 48 -------------------- src.mk | 1 - test_util/testharness.cc | 2 - test_util/testharness.h | 1 - util/regex.cc | 50 --------------------- util/slice_test.cc | 30 ------------- utilities/object_registry_test.cc | 27 ----------- 11 files changed, 2 insertions(+), 202 deletions(-) delete mode 100644 include/rocksdb/utilities/regex.h delete mode 100644 util/regex.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index 0d8d6a0b75..43e1480b56 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -848,7 +848,6 @@ set(SOURCES util/random.cc util/rate_limiter.cc util/ribbon_config.cc - util/regex.cc util/slice.cc util/file_checksum_helper.cc util/status.cc diff --git a/HISTORY.md b/HISTORY.md index b08c4005bf..092a4333ef 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -4,6 +4,7 @@ * Remove HDFS support from main repo. * Remove librados support from main repo. * Remove deprecated API DB::AddFile from main repo. +* Remove deprecated API ObjectLibrary::Register() and the (now obsolete) Regex public API. Use ObjectLibrary::AddFactory() with PatternEntry instead. ## 6.29.0 (01/21/2022) Note: The next release will be major release 7.0. See https://github.com/facebook/rocksdb/issues/9390 for more info. diff --git a/TARGETS b/TARGETS index 146c47a9c7..325efd514e 100644 --- a/TARGETS +++ b/TARGETS @@ -368,7 +368,6 @@ cpp_library( "util/murmurhash.cc", "util/random.cc", "util/rate_limiter.cc", - "util/regex.cc", "util/ribbon_config.cc", "util/slice.cc", "util/status.cc", @@ -697,7 +696,6 @@ cpp_library( "util/murmurhash.cc", "util/random.cc", "util/rate_limiter.cc", - "util/regex.cc", "util/ribbon_config.cc", "util/slice.cc", "util/status.cc", diff --git a/include/rocksdb/utilities/object_registry.h b/include/rocksdb/utilities/object_registry.h index a5c4b36c50..05e438bcd2 100644 --- a/include/rocksdb/utilities/object_registry.h +++ b/include/rocksdb/utilities/object_registry.h @@ -16,7 +16,6 @@ #include #include "rocksdb/status.h" -#include "rocksdb/utilities/regex.h" namespace ROCKSDB_NAMESPACE { class Customizable; @@ -51,28 +50,6 @@ class ObjectLibrary { virtual const char* Name() const = 0; }; - // A class that implements an Entry based on Regex. - // - // WARNING: some regexes are problematic for std::regex; see - // https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61582 for example - // - // This class is deprecated and will be removed in a future release - class RegexEntry : public Entry { - public: - explicit RegexEntry(const std::string& name) : name_(name) { - Regex::Parse(name, ®ex_).PermitUncheckedError(); - } - - bool Matches(const std::string& target) const override { - return regex_.Matches(target); - } - const char* Name() const override { return name_.c_str(); } - - private: - std::string name_; - Regex regex_; // The pattern for this entry - }; - public: // Class for matching target strings to a pattern. // Entries consist of a name that starts the pattern and attributes @@ -240,23 +217,6 @@ class ObjectLibrary { void Dump(Logger* logger) const; - // Registers the factory with the library for the regular expression pattern. - // If the pattern matches, the factory may be used to create a new object. - // - // WARNING: some regexes are problematic for std::regex; see - // https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61582 for example - // - // Deprecated. Will be removed in a major release. Code should use AddFactory - // instead. - template - const FactoryFunc& Register(const std::string& pattern, - const FactoryFunc& factory) { - std::unique_ptr entry( - new FactoryEntry(new RegexEntry(pattern), factory)); - AddFactoryEntry(T::Type(), std::move(entry)); - return factory; - } - // Registers the factory with the library for the name. // If name==target, the factory may be used to create a new object. template @@ -272,6 +232,7 @@ class ObjectLibrary { // If the entry matches the target, the factory may be used to create a new // object. // @see PatternEntry for the matching rules. + // NOTE: This function replaces the old ObjectLibrary::Register() template const FactoryFunc& AddFactory(const PatternEntry& entry, const FactoryFunc& func) { diff --git a/include/rocksdb/utilities/regex.h b/include/rocksdb/utilities/regex.h deleted file mode 100644 index ac24312370..0000000000 --- a/include/rocksdb/utilities/regex.h +++ /dev/null @@ -1,48 +0,0 @@ -// Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved. -// This source code is licensed under both the GPLv2 (found in the -// COPYING file in the root directory) and Apache 2.0 License -// (found in the LICENSE.Apache file in the root directory). - -#pragma once - -#ifndef ROCKSDB_LITE - -#include -#include - -#include "rocksdb/status.h" - -namespace ROCKSDB_NAMESPACE { - -// A wrapper for parsed regular expressions. The regex syntax and matching is -// compatible with std::regex. -// -// !!!!!! WARNING !!!!!!: The implementation currently uses std::regex, which -// has terrible performance in some cases, including possible crash due to -// stack overflow. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61582 -// for example. Avoid use in production as much as possible. -// -// Internal note: see also TestRegex -class Regex { - public: - // Note: Cannot be constructed with a pattern, so that syntax errors can - // be handled without using exceptions. - - // Parse returns OK and saves to `out` when the pattern is valid regex - // syntax (modified ECMAScript), or else returns InvalidArgument. - // See https://en.cppreference.com/w/cpp/regex/ecmascript - static Status Parse(const char *pattern, Regex *out); - static Status Parse(const std::string &pattern, Regex *out); - - // Checks that the whole of str is matched by this regex. If called on a - // default-constructed Regex, will trigger assertion failure in DEBUG build - // or return false in release build. - bool Matches(const std::string &str) const; - - private: - class Impl; - std::shared_ptr impl_; // shared_ptr for simple implementation -}; -} // namespace ROCKSDB_NAMESPACE - -#endif // ROCKSDB_LITE diff --git a/src.mk b/src.mk index a450d0b82d..a0c0191b6c 100644 --- a/src.mk +++ b/src.mk @@ -223,7 +223,6 @@ LIB_SOURCES = \ util/random.cc \ util/rate_limiter.cc \ util/ribbon_config.cc \ - util/regex.cc \ util/slice.cc \ util/file_checksum_helper.cc \ util/status.cc \ diff --git a/test_util/testharness.cc b/test_util/testharness.cc index 899d39cc7c..32d8a07d7f 100644 --- a/test_util/testharness.cc +++ b/test_util/testharness.cc @@ -69,8 +69,6 @@ TestRegex::TestRegex(const char* pattern) const std::string& TestRegex::GetPattern() const { return pattern_; } -// Sorry about code duplication with regex.cc, but it doesn't support LITE -// due to exception handling class TestRegex::Impl : public std::regex { public: using std::regex::basic_regex; diff --git a/test_util/testharness.h b/test_util/testharness.h index e5db00dc6e..8576eea622 100644 --- a/test_util/testharness.h +++ b/test_util/testharness.h @@ -14,7 +14,6 @@ #else #include #endif -#include "rocksdb/utilities/regex.h" // A "skipped" test has a specific meaning in Facebook infrastructure: the // test is in good shape and should be run, but something about the diff --git a/util/regex.cc b/util/regex.cc deleted file mode 100644 index 7aa4c52ae2..0000000000 --- a/util/regex.cc +++ /dev/null @@ -1,50 +0,0 @@ -// Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved. -// This source code is licensed under both the GPLv2 (found in the -// COPYING file in the root directory) and Apache 2.0 License -// (found in the LICENSE.Apache file in the root directory). - -// LITE not supported here in part because of exception handling -#ifndef ROCKSDB_LITE - -#include "rocksdb/utilities/regex.h" - -#include -#include - -namespace ROCKSDB_NAMESPACE { - -// This section would change for alternate underlying implementations other -// than std::regex. -#if 1 -class Regex::Impl : public std::regex { - public: - using std::regex::basic_regex; -}; - -bool Regex::Matches(const std::string &str) const { - if (impl_) { - return std::regex_match(str, *impl_); - } else { - // Should not call Matches on unset Regex - assert(false); - return false; - } -} - -Status Regex::Parse(const std::string &pattern, Regex *out) { - try { - out->impl_.reset(new Impl(pattern)); - return Status::OK(); - } catch (const std::regex_error &e) { - return Status::InvalidArgument(e.what()); - } -} -#endif - -Status Regex::Parse(const char *pattern, Regex *out) { - return Parse(std::string(pattern), out); -} - -} // namespace ROCKSDB_NAMESPACE - -#endif // ROCKSDB_LITE diff --git a/util/slice_test.cc b/util/slice_test.cc index e3997ca331..6fea7e622d 100644 --- a/util/slice_test.cc +++ b/util/slice_test.cc @@ -11,7 +11,6 @@ #include "port/stack_trace.h" #include "rocksdb/data_structure.h" #include "rocksdb/types.h" -#include "rocksdb/utilities/regex.h" #include "test_util/testharness.h" #include "test_util/testutil.h" @@ -177,35 +176,6 @@ TEST_F(SmallEnumSetTest, SmallSetTest) { ASSERT_FALSE(fs.Contains(FileType::kDBLockFile)); } -// ***************************************************************** // -// Unit test for Regex -#ifndef ROCKSDB_LITE -TEST(RegexTest, ParseEtc) { - Regex r; - ASSERT_OK(Regex::Parse("[abc]{5}", &r)); - ASSERT_TRUE(r.Matches("abcba")); - ASSERT_FALSE(r.Matches("abcb")); // too short - ASSERT_FALSE(r.Matches("abcbaa")); // too long - - ASSERT_OK(Regex::Parse(".*foo.*", &r)); - ASSERT_TRUE(r.Matches("123forfoodie456")); - ASSERT_FALSE(r.Matches("123forfodie456")); - // Ensure copy operator - Regex r2; - r2 = r; - ASSERT_TRUE(r2.Matches("123forfoodie456")); - ASSERT_FALSE(r2.Matches("123forfodie456")); - // Ensure copy constructor - Regex r3{r}; - ASSERT_TRUE(r3.Matches("123forfoodie456")); - ASSERT_FALSE(r3.Matches("123forfodie456")); - - ASSERT_TRUE(Regex::Parse("*foo.*", &r).IsInvalidArgument()); - ASSERT_TRUE(Regex::Parse("[abc", &r).IsInvalidArgument()); - ASSERT_TRUE(Regex::Parse("[abc]{1", &r).IsInvalidArgument()); -} -#endif // ROCKSDB_LITE - } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) { diff --git a/utilities/object_registry_test.cc b/utilities/object_registry_test.cc index 77bf2ec1c3..7880703f05 100644 --- a/utilities/object_registry_test.cc +++ b/utilities/object_registry_test.cc @@ -8,7 +8,6 @@ #include "rocksdb/utilities/object_registry.h" #include "rocksdb/customizable.h" -#include "rocksdb/utilities/regex.h" #include "test_util/testharness.h" namespace ROCKSDB_NAMESPACE { @@ -443,32 +442,6 @@ TEST_F(ObjRegistryTest, TestGetOrCreateManagedObject) { ASSERT_EQ(2, obj.use_count()); } -TEST_F(ObjRegistryTest, TestDeprecatedRegex) { - Regex regex; - Env* env = nullptr; - auto registry = ObjectRegistry::NewInstance(); - if (Regex::Parse("XYZ", ®ex).ok()) { - registry->AddLibrary("XYZ")->Register( - "XYZ", - [](const std::string& /*uri*/, std::unique_ptr* /*env_guard*/, - std::string* /* errmsg */) { return Env::Default(); }); - ASSERT_NOK(registry->NewStaticObject("X", &env)); - ASSERT_OK(registry->NewStaticObject("XYZ", &env)); - ASSERT_EQ(env, Env::Default()); - } - if (Regex::Parse("ABC://.*", ®ex).ok()) { - registry->AddLibrary("ABC")->Register( - "ABC://.*", - [](const std::string& /*uri*/, std::unique_ptr* /*env_guard*/, - std::string* /* errmsg */) { return Env::Default(); }); - ASSERT_NOK(registry->NewStaticObject("ABC", &env)); - ASSERT_OK(registry->NewStaticObject("ABC://123", &env)); - ASSERT_EQ(env, Env::Default()); - ASSERT_OK(registry->NewStaticObject("ABC://", &env)); - ASSERT_EQ(env, Env::Default()); - } -} - class PatternEntryTest : public testing::Test {}; TEST_F(PatternEntryTest, TestSimpleEntry) {