Repository: incubator-impala
Updated Branches:
  refs/heads/master a98b90bd3 -> 6b7425a57


IMPALA-5031: unsafe random number generation in buffer-pool-test

The bug is that ConcurrentRegistration shares one random number
generator between all the threads. This isn't safe and UBSAN was
unhappy with it.

The fix is to create one RNG per thread. We already do that in a
different test so the code is factored out into a utility function.

Testing:
Ran buffer-pool-test under UBSAN

Change-Id: I0eb3bec152a58d9ec39413780cb2c431dd8d4fa8
Reviewed-on: http://gerrit.cloudera.org:8080/7596
Reviewed-by: Sailesh Mukil <sail...@cloudera.com>
Tested-by: Impala Public Jenkins


Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/6b7425a5
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/6b7425a5
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/6b7425a5

Branch: refs/heads/master
Commit: 6b7425a574b7d5c37562c6fb0d3d1a148cf21ea0
Parents: a98b90b
Author: Tim Armstrong <tarmstr...@cloudera.com>
Authored: Fri Aug 4 16:31:56 2017 -0700
Committer: Impala Public Jenkins <impala-public-jenk...@gerrit.cloudera.org>
Committed: Sat Aug 5 04:40:01 2017 +0000

----------------------------------------------------------------------
 be/src/runtime/bufferpool/buffer-pool-test.cc | 22 ++++++++++++----------
 be/src/testutil/rand-util.h                   | 11 ++++++++++-
 2 files changed, 22 insertions(+), 11 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/6b7425a5/be/src/runtime/bufferpool/buffer-pool-test.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/bufferpool/buffer-pool-test.cc 
b/be/src/runtime/bufferpool/buffer-pool-test.cc
index c98ba74..14446e7 100644
--- a/be/src/runtime/bufferpool/buffer-pool-test.cc
+++ b/be/src/runtime/bufferpool/buffer-pool-test.cc
@@ -98,9 +98,10 @@ class BufferPoolTest : public ::testing::Test {
   const static int64_t TEST_BUFFER_LEN = 1024;
 
   /// Test helper to simulate registering then deregistering a number of 
queries with
-  /// the given initial reservation and reservation limit.
+  /// the given initial reservation and reservation limit. 'rng' is used to 
generate
+  /// any random numbers needed.
   void RegisterQueriesAndClients(BufferPool* pool, int query_id_hi, int 
num_queries,
-      int64_t initial_query_reservation, int64_t query_reservation_limit);
+      int64_t initial_query_reservation, int64_t query_reservation_limit, 
mt19937* rng);
 
   /// Create and destroy a page multiple times.
   void CreatePageLoop(BufferPool* pool, TmpFileMgr::FileGroup* file_group,
@@ -389,7 +390,8 @@ class BufferPoolTest : public ::testing::Test {
 const int64_t BufferPoolTest::TEST_BUFFER_LEN;
 
 void BufferPoolTest::RegisterQueriesAndClients(BufferPool* pool, int 
query_id_hi,
-    int num_queries, int64_t initial_query_reservation, int64_t 
query_reservation_limit) {
+    int num_queries, int64_t initial_query_reservation, int64_t 
query_reservation_limit,
+    mt19937* rng) {
   Status status;
 
   int clients_per_query = 32;
@@ -416,7 +418,7 @@ void BufferPoolTest::RegisterQueriesAndClients(BufferPool* 
pool, int query_id_hi
           initial_query_reservation / clients_per_query + j
           < initial_query_reservation % clients_per_query;
       // Reservation limit can be anything greater or equal to the initial 
reservation.
-      int64_t client_reservation_limit = initial_client_reservation + rng_() % 
100000;
+      int64_t client_reservation_limit = initial_client_reservation + (*rng)() 
% 100000;
       string name = Substitute("Client $0 for query $1", j, query_id);
       EXPECT_OK(pool->RegisterClient(name, NULL, query_reservation, NULL,
           client_reservation_limit, NewProfile(), &clients[i][j]));
@@ -453,8 +455,8 @@ TEST_F(BufferPoolTest, BasicRegistration) {
 
   BufferPool pool(TEST_BUFFER_LEN, total_mem);
 
-  RegisterQueriesAndClients(
-      &pool, 0, num_concurrent_queries, sum_initial_reservations, 
reservation_limit);
+  RegisterQueriesAndClients(&pool, 0, num_concurrent_queries, 
sum_initial_reservations,
+      reservation_limit, &rng_);
 
   ASSERT_EQ(global_reservations_.GetUsedReservation(), 0);
   ASSERT_EQ(global_reservations_.GetChildReservations(), 0);
@@ -475,12 +477,13 @@ TEST_F(BufferPoolTest, ConcurrentRegistration) {
   global_reservations_.InitRootTracker(NewProfile(), total_mem);
 
   BufferPool pool(TEST_BUFFER_LEN, total_mem);
-
+  vector<mt19937> thread_rngs = 
RandTestUtil::CreateThreadLocalRngs(num_threads, &rng_);
   // Launch threads, each with a different set of query IDs.
   thread_group workers;
   for (int i = 0; i < num_threads; ++i) {
     workers.add_thread(new 
thread(bind(&BufferPoolTest::RegisterQueriesAndClients, this,
-        &pool, i, queries_per_thread, sum_initial_reservations, 
reservation_limit)));
+        &pool, i, queries_per_thread, sum_initial_reservations, 
reservation_limit,
+        &thread_rngs[i])));
   }
   workers.join_all();
 
@@ -1767,9 +1770,8 @@ void BufferPoolTest::TestRandomInternalMulti(
   MemTracker global_tracker(TOTAL_MEM);
   FileGroup* shared_file_group = NewFileGroup();
   thread_group workers;
-  vector<mt19937> rngs(num_threads);
+  vector<mt19937> rngs = RandTestUtil::CreateThreadLocalRngs(num_threads, 
&rng_);
   for (int i = 0; i < num_threads; ++i) {
-    rngs[i].seed(rng_()); // Seed the thread-local rngs.
     workers.add_thread(new thread(
         [this, &pool, shared_file_group, &global_tracker, &rngs, i, 
multiple_pins]() {
           TestRandomInternalImpl(

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/6b7425a5/be/src/testutil/rand-util.h
----------------------------------------------------------------------
diff --git a/be/src/testutil/rand-util.h b/be/src/testutil/rand-util.h
index 6cdbed4..255fcb0 100644
--- a/be/src/testutil/rand-util.h
+++ b/be/src/testutil/rand-util.h
@@ -37,11 +37,20 @@ class RandTestUtil {
     if (seed_str != nullptr) {
       seed = atoi(seed_str);
     } else {
-      seed = time(nullptr);
+      seed = std::random_device()();
     }
     LOG(INFO) << "Random seed (overridable with " << env_var << "): " << seed;
     rng->seed(seed);
   }
+
+  /// Create 'num_threads' rngs, seeded using 'seed_rng'. Used when we want 
each thread
+  /// to have its own RNG (since they are not thread-safe).
+  static vector<std::mt19937> CreateThreadLocalRngs(
+      int num_threads, std::mt19937* seed_rng) {
+    vector<std::mt19937> rngs(num_threads);
+    for (std::mt19937& rng : rngs) rng.seed((*seed_rng)());
+    return rngs;
+  }
 };
 }
 

Reply via email to