andreacavalli/rocksdb
0
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Remove deprecated ObjectLibrary::Register() (and Regex public API) (#9439)

Browse Source 
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🅱️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
This commit is contained in:
Peter Dillinger 2022-01-26 16:06:46 -08:00 committed by Facebook GitHub Bot
parent c27ca23644
commit 449029f865
11 changed files with 2 additions and 202 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
CMakeLists.txt
View File

@ -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

1
HISTORY.md
View File

@ -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.

2
TARGETS
View File

@ -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",

41
include/rocksdb/utilities/object_registry.h
View File

@ -16,7 +16,6 @@
#include <vector>
#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, &regex_).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 <typename T>
const FactoryFunc<T>& Register(const std::string& pattern,
const FactoryFunc<T>& factory) {
std::unique_ptr<Entry> entry(
new FactoryEntry<T>(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 <typename T>
@ -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 <typename T>
const FactoryFunc<T>& AddFactory(const PatternEntry& entry,
const FactoryFunc<T>& func) {

48
include/rocksdb/utilities/regex.h
View File

@ -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 <memory>
#include <string>
#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> impl_; // shared_ptr for simple implementation
};
} // namespace ROCKSDB_NAMESPACE
#endif // ROCKSDB_LITE

1
src.mk
View File

@ -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 \

2
test_util/testharness.cc
View File

@ -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;

1
test_util/testharness.h
View File

@ -14,7 +14,6 @@
#else
#include <gtest/gtest.h>
#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

50
util/regex.cc
View File

@ -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 <cassert>
#include <regex>
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

30
util/slice_test.cc
View File

@ -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) {

27
utilities/object_registry_test.cc
View File

@ -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", &regex).ok()) {
registry->AddLibrary("XYZ")->Register<Env>(
"XYZ",
[](const std::string& /*uri*/, std::unique_ptr<Env>* /*env_guard*/,
std::string* /* errmsg */) { return Env::Default(); });
ASSERT_NOK(registry->NewStaticObject<Env>("X", &env));
ASSERT_OK(registry->NewStaticObject<Env>("XYZ", &env));
ASSERT_EQ(env, Env::Default());
}
if (Regex::Parse("ABC://.*", &regex).ok()) {
registry->AddLibrary("ABC")->Register<Env>(
"ABC://.*",
[](const std::string& /*uri*/, std::unique_ptr<Env>* /*env_guard*/,
std::string* /* errmsg */) { return Env::Default(); });
ASSERT_NOK(registry->NewStaticObject<Env>("ABC", &env));
ASSERT_OK(registry->NewStaticObject<Env>("ABC://123", &env));
ASSERT_EQ(env, Env::Default());
ASSERT_OK(registry->NewStaticObject<Env>("ABC://", &env));
ASSERT_EQ(env, Env::Default());
}
}
class PatternEntryTest : public testing::Test {};
TEST_F(PatternEntryTest, TestSimpleEntry) {