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 <[email protected]> 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 <[email protected]> Authored: Fri Aug 4 16:31:56 2017 -0700 Committer: Impala Public Jenkins <[email protected]> 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; + } }; }
