From 825e3b28fcbcf66adb4ee8bcd2b803ee63b7d40c Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Thu, 16 Aug 2018 15:20:50 -0700 Subject: [PATCH 1/4] Fix ID generation after fork. --- src/utility.cpp | 34 +++++++++++++++++++++++++++++++--- test/CMakeLists.txt | 1 + test/fork_id_test.cpp | 41 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 3 deletions(-) create mode 100644 test/fork_id_test.cpp diff --git a/src/utility.cpp b/src/utility.cpp index 0b6f1c21..e4ef21f8 100644 --- a/src/utility.cpp +++ b/src/utility.cpp @@ -2,13 +2,16 @@ #include #include #include +#include "lightstep-tracer-common/collector.pb.h" + #include #include #include #include #include #include -#include "lightstep-tracer-common/collector.pb.h" + +#include namespace lightstep { //------------------------------------------------------------------------------ @@ -25,12 +28,37 @@ google::protobuf::Timestamp ToTimestamp( return ts; } +//------------------------------------------------------------------------------ +// TlsRandomNumberGenerator +//------------------------------------------------------------------------------ +// Wraps a thread_local random number generator, but adds a fork handler so that +// the generator will be correctly seeded after forking. +// +// See https://stackoverflow.com/q/51882689/4447365 and +// https://github.com/opentracing-contrib/nginx-opentracing/issues/52 +namespace { +class TlsRandomNumberGenerator { + public: + TlsRandomNumberGenerator() { pthread_atfork(nullptr, nullptr, OnFork); } + + static std::mt19937_64& engine() { return base_generator_.engine(); } + + private: + static thread_local lightstep::randutils::mt19937_64_rng base_generator_; + + static void OnFork() { base_generator_.seed(); } +}; + +thread_local lightstep::randutils::mt19937_64_rng + TlsRandomNumberGenerator::base_generator_{}; +} // namespace + //------------------------------------------------------------------------------ // GenerateId //------------------------------------------------------------------------------ uint64_t GenerateId() { - static thread_local lightstep::randutils::mt19937_64_rng rand_source{}; - return rand_source.engine()(); + static TlsRandomNumberGenerator random_number_generator; + return random_number_generator.engine()(); } //------------------------------------------------------------------------------ diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 1794f269..f27c99a6 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -9,6 +9,7 @@ _lightstep_test(tracer_test tracer_test.cpp utility.cpp) _lightstep_test(utility_test utility_test.cpp) _lightstep_test(logger_test logger_test.cpp) +_lightstep_test(fork_id_test fork_id_test.cpp) _lightstep_test(propagation_test propagation_test.cpp) _lightstep_test(auto_recorder_test auto_recorder_test.cpp utility.cpp diff --git a/test/fork_id_test.cpp b/test/fork_id_test.cpp new file mode 100644 index 00000000..db1adf3e --- /dev/null +++ b/test/fork_id_test.cpp @@ -0,0 +1,41 @@ +// Verifies that IDs don't clash after forking the process. +// +// See https://github.com/opentracing-contrib/nginx-opentracing/issues/52 +// +#include "../src/utility.h" + +#include +#include +#include +#include +#include +#include +#include +using namespace lightstep; + +static uint64_t* child_id; + +int main() { + GenerateId(); + + // See https://stackoverflow.com/a/13274800/4447365 + child_id = static_cast(mmap(nullptr, sizeof(*child_id), + PROT_READ | PROT_WRITE, + MAP_SHARED | MAP_ANONYMOUS, -1, 0)); + *child_id = 0; + if (fork() == 0) { + *child_id = GenerateId(); + exit(EXIT_SUCCESS); + } else { + wait(nullptr); + auto parent_id = GenerateId(); + auto child_id_copy = *child_id; + munmap(static_cast(child_id), sizeof(*child_id)); + if (parent_id == child_id_copy) { + std::cerr << "Child and parent ids are the same value " << parent_id + << "\n"; + return -1; + } + } + return 0; +} From 50649ed6df2a73a86d5eb09110f8c2601e2ea867 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Thu, 16 Aug 2018 15:52:47 -0700 Subject: [PATCH 2/4] Fix clang-tidy errors. --- 3rd_party/randutils/include/lightstep/randutils.h | 6 +++--- src/utility.cpp | 4 ++-- src/utility.h | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/3rd_party/randutils/include/lightstep/randutils.h b/3rd_party/randutils/include/lightstep/randutils.h index a630f25c..4c1f2058 100644 --- a/3rd_party/randutils/include/lightstep/randutils.h +++ b/3rd_party/randutils/include/lightstep/randutils.h @@ -650,7 +650,7 @@ class random_generator { public: template - random_generator(Seeding&& seeding = default_seed_type{}) + random_generator(Seeding&& seeding = default_seed_type{}) noexcept : engine_{seed_seq_cast(std::forward(seeding))} { // Nothing (else) to do @@ -662,7 +662,7 @@ class random_generator { // https://llvm.org/bugs/show_bug.cgi?id=23029 template - random_generator(Seeding&& seeding, Params&&... params) + random_generator(Seeding&& seeding, Params&&... params) noexcept : engine_{seed_seq_cast(std::forward(seeding)), std::forward(params)...} { @@ -671,7 +671,7 @@ class random_generator { template - void seed(Seeding&& seeding = default_seed_type{}) + void seed(Seeding&& seeding = default_seed_type{}) noexcept { engine_.seed(seed_seq_cast(seeding)); } diff --git a/src/utility.cpp b/src/utility.cpp index e4ef21f8..f27b70b8 100644 --- a/src/utility.cpp +++ b/src/utility.cpp @@ -56,9 +56,9 @@ thread_local lightstep::randutils::mt19937_64_rng //------------------------------------------------------------------------------ // GenerateId //------------------------------------------------------------------------------ -uint64_t GenerateId() { +uint64_t GenerateId() noexcept { static TlsRandomNumberGenerator random_number_generator; - return random_number_generator.engine()(); + return TlsRandomNumberGenerator::engine()(); } //------------------------------------------------------------------------------ diff --git a/src/utility.h b/src/utility.h index ffa078ad..a9eb5c61 100644 --- a/src/utility.h +++ b/src/utility.h @@ -14,7 +14,7 @@ google::protobuf::Timestamp ToTimestamp( const std::chrono::system_clock::time_point& t); // Generates a random uint64_t. -uint64_t GenerateId(); +uint64_t GenerateId() noexcept; // Attempts to determine the name of the executable invoked. Returns // "c++-program" if unsuccessful. From 1afb624ee2c15a676d1595cb3bf5651a96fede69 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Thu, 16 Aug 2018 16:46:43 -0700 Subject: [PATCH 3/4] Make clang-tidy fixes. --- 3rd_party/randutils/include/lightstep/randutils.h | 4 ++-- test/fork_id_test.cpp | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/3rd_party/randutils/include/lightstep/randutils.h b/3rd_party/randutils/include/lightstep/randutils.h index 4c1f2058..92a808cc 100644 --- a/3rd_party/randutils/include/lightstep/randutils.h +++ b/3rd_party/randutils/include/lightstep/randutils.h @@ -540,13 +540,13 @@ class auto_seeded : public SeedSeq { return *this; } - auto_seeded(default_seeds seeds) + auto_seeded(default_seeds seeds) noexcept : SeedSeq(seeds.begin(), seeds.end()) { // Nothing else to do } - auto_seeded() + auto_seeded() noexcept : auto_seeded(local_entropy()) { // Nothing else to do diff --git a/test/fork_id_test.cpp b/test/fork_id_test.cpp index db1adf3e..517fd441 100644 --- a/test/fork_id_test.cpp +++ b/test/fork_id_test.cpp @@ -1,7 +1,6 @@ // Verifies that IDs don't clash after forking the process. // // See https://github.com/opentracing-contrib/nginx-opentracing/issues/52 -// #include "../src/utility.h" #include @@ -18,6 +17,8 @@ static uint64_t* child_id; int main() { GenerateId(); + // Set up shared memory to communicate between parent and child processes. + // // See https://stackoverflow.com/a/13274800/4447365 child_id = static_cast(mmap(nullptr, sizeof(*child_id), PROT_READ | PROT_WRITE, From 87a5ef528477b0147fb4fe0b6cb483029aeb9ad7 Mon Sep 17 00:00:00 2001 From: Ryan Burn Date: Thu, 16 Aug 2018 17:31:56 -0700 Subject: [PATCH 4/4] Don't use noexcept specifier. --- src/utility.cpp | 2 +- src/utility.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/utility.cpp b/src/utility.cpp index f27b70b8..9b23851e 100644 --- a/src/utility.cpp +++ b/src/utility.cpp @@ -56,7 +56,7 @@ thread_local lightstep::randutils::mt19937_64_rng //------------------------------------------------------------------------------ // GenerateId //------------------------------------------------------------------------------ -uint64_t GenerateId() noexcept { +uint64_t GenerateId() { static TlsRandomNumberGenerator random_number_generator; return TlsRandomNumberGenerator::engine()(); } diff --git a/src/utility.h b/src/utility.h index a9eb5c61..ffa078ad 100644 --- a/src/utility.h +++ b/src/utility.h @@ -14,7 +14,7 @@ google::protobuf::Timestamp ToTimestamp( const std::chrono::system_clock::time_point& t); // Generates a random uint64_t. -uint64_t GenerateId() noexcept; +uint64_t GenerateId(); // Attempts to determine the name of the executable invoked. Returns // "c++-program" if unsuccessful.