From fa908926c7ab9270037e2ab71036f8f6f87e9850 Mon Sep 17 00:00:00 2001 From: Chris Kennelly Date: Wed, 23 Apr 2014 00:56:17 -0700 Subject: [PATCH] Partially resolve google/benchmark#17 by fixing regular expression leak. This adds a unit test to validate the wrapper without running the entirety of benchmark_test. --- src/CMakeLists.txt | 4 ++- src/benchmark.cc | 21 +++++----------- src/re.cc | 59 +++++++++++++++++++++++++++++++++++++++++++++ src/re.h | 50 ++++++++++++++++++++++++++++++++++++++ test/CMakeLists.txt | 5 ++++ test/re_test.cc | 39 ++++++++++++++++++++++++++++++ 6 files changed, 162 insertions(+), 16 deletions(-) create mode 100644 src/re.cc create mode 100644 src/re.h create mode 100644 test/re_test.cc diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 6c5857ce..5c6a3908 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -1,6 +1,8 @@ set(SOURCE_FILES "benchmark.cc" "colorprint.cc" "commandlineflags.cc" "sleep.cc" "sysinfo.cc" "walltime.cc") +set(RE_FILES "re.cc") -add_library(benchmark STATIC ${SOURCE_FILES}) +add_library(benchmark_re STATIC ${RE_FILES}) +add_library(benchmark STATIC ${SOURCE_FILES} ${RE_FILES}) # Install target (will install the library to specified CMAKE_INSTALL_PREFIX variable) install( diff --git a/src/benchmark.cc b/src/benchmark.cc index 7e1357f6..e3055a78 100644 --- a/src/benchmark.cc +++ b/src/benchmark.cc @@ -17,6 +17,7 @@ #include "colorprint.h" #include "commandlineflags.h" #include "mutex_lock.h" +#include "re.h" #include "sleep.h" #include "stat.h" #include "sysinfo.h" @@ -27,12 +28,6 @@ #include #include -#if defined OS_FREEBSD -#include -#else -#include -#endif - #include #include #include @@ -745,14 +740,10 @@ std::vector Benchmark::CreateBenchmarkInstances( void Benchmark::FindBenchmarks(const std::string& spec, std::vector* benchmarks) { // Make regular expression out of command-line flag - regex_t re; - int ec = regcomp(&re, spec.c_str(), REG_EXTENDED | REG_NOSUB); - if (ec != 0) { - size_t needed = regerror(ec, &re, NULL, 0); - char* errbuf = new char[needed]; - regerror(ec, &re, errbuf, needed); - std::cerr << "Could not compile benchmark re: " << errbuf << "\n"; - delete[] errbuf; + Regex re; + std::string re_error; + if (!re.Init(spec, &re_error)) { + std::cerr << "Could not compile benchmark re: " << re_error << std::endl; return; } @@ -762,7 +753,7 @@ void Benchmark::FindBenchmarks(const std::string& spec, if (family == nullptr) continue; // Family was deleted // Match against filter. - if (regexec(&re, family->name_.c_str(), 0, NULL, 0) != 0) { + if (!re.Match(family->name_)) { #ifdef DEBUG std::cout << "Skipping " << family->name_ << "\n"; #endif diff --git a/src/re.cc b/src/re.cc new file mode 100644 index 00000000..e51e2b61 --- /dev/null +++ b/src/re.cc @@ -0,0 +1,59 @@ +// Copyright 2014 Google Inc. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include "re.h" + +namespace benchmark { + +Regex::Regex() : init_(false) { } + +bool Regex::Init(const std::string& spec, std::string* error) { + int ec = regcomp(&re_, spec.c_str(), REG_EXTENDED | REG_NOSUB); + if (ec != 0) { + if (error) { + size_t needed = regerror(ec, &re_, NULL, 0); + char* errbuf = new char[needed]; + regerror(ec, &re_, errbuf, needed); + + // regerror returns the number of bytes necessary to null terminate + // the string, so we move that when assigning to error. + CHECK_NE(needed, 0); + error->assign(errbuf, needed - 1); + + delete[] errbuf; + } + + return false; + } + + init_ = true; + return true; +} + +Regex::~Regex() { + if (init_) { + regfree(&re_); + } +} + +bool Regex::Match(const std::string& str) { + if (!init_) { + return false; + } + + return regexec(&re_, str.c_str(), 0, NULL, 0) == 0; +} + +} // end namespace benchmark diff --git a/src/re.h b/src/re.h new file mode 100644 index 00000000..b179bb2a --- /dev/null +++ b/src/re.h @@ -0,0 +1,50 @@ +// Copyright 2014 Google Inc. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef BENCHMARK_RE_H_ +#define BENCHMARK_RE_H_ + +#if defined OS_FREEBSD +#include +#else +#include +#endif +#include + +namespace benchmark { + +// A wrapper around the POSIX regular expression API that provides automatic +// cleanup +class Regex { + public: + Regex(); + ~Regex(); + + // Compile a regular expression matcher from spec. Returns true on success. + // + // On failure (and if error is not NULL), error is populated with a human + // readable error message if an error occurs. + bool Init(const std::string& spec, std::string* error); + + // Returns whether str matches the compiled regular expression. + bool Match(const std::string& str); + private: + bool init_; + // Underlying regular expression object + regex_t re_; +}; + +} // end namespace benchmark + +#endif // BENCHMARK_RE_H_ diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index f725365a..2999c2c9 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -1,3 +1,8 @@ # Demonstration executable add_executable(benchmark_test benchmark_test.cc) target_link_libraries(benchmark_test benchmark ${CMAKE_THREAD_LIBS_INIT}) + +# Test harness for regex wrapper +add_executable(re_test ${RE_FILES} "re_test.cc") +target_link_libraries(re_test benchmark_re ${CMAKE_THREAD_LIBS_INIT} gtest gtest_main) +add_test(regex re_test) diff --git a/test/re_test.cc b/test/re_test.cc new file mode 100644 index 00000000..c5ff9aa6 --- /dev/null +++ b/test/re_test.cc @@ -0,0 +1,39 @@ +// Copyright 2014 Google Inc. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include "re.h" + +TEST(Regex, RegexSimple) { + benchmark::Regex re; + EXPECT_TRUE(re.Init("a+", NULL)); + + EXPECT_FALSE(re.Match("")); + EXPECT_TRUE(re.Match("a")); + EXPECT_TRUE(re.Match("aa")); + EXPECT_FALSE(re.Match("b")); +} + +TEST(Regex, InvalidNoErrorMessage) { + benchmark::Regex re; + EXPECT_FALSE(re.Init("[", NULL)); +} + +TEST(Regex, Invalid) { + std::string error; + benchmark::Regex re; + EXPECT_FALSE(re.Init("[", &error)); + + EXPECT_NE("", error); +}