From e86af4b7e56ed5b7050cb4f41ae534f54748598c Mon Sep 17 00:00:00 2001 From: Nicholas Marriott Date: Mon, 9 Apr 2012 10:46:32 -0400 Subject: [PATCH 1/2] Change evutil_weakrand_() to avoid platform random() This change allows us to avoid perturbing the platform's random(), and to avoid hitting locks on random() in the platform's libc. evutil_weakrand_() is, well, weak, so we choose here an algorithm that favors speed over a number of other possibly desirable properties. We're using a linear congruential generator, and taking our parameters from those shared by the OpenBSD random() implementation, and Glibc's fastest random() implementation. The low bits of a LCG of modulus 2^32 are (notoriously) less random than the higher bits. So to generate a random value in a range, using the % operator is no good; we ought to divide. We add an evutil_weakrand_range_() function to do that. This code also changes the interface of evutil_weakrand_() so that it now manipulates an explicit seed, rather than having the seed in a static variable. This change enables us to use existing locks to achieve thread-safety, rather than having to rely on an additional lock. (Patch by Nicholas Marriott; commit message by Nick Mathewson.) --- bufferevent-internal.h | 4 ++++ bufferevent_ratelim.c | 2 +- event-internal.h | 3 +++ evutil.c | 23 ++++++++++++++++------- poll.c | 2 +- select.c | 2 +- test/regress_buffer.c | 3 ++- test/regress_util.c | 5 +++-- util-internal.h | 3 ++- win32select.c | 9 ++++++--- 10 files changed, 39 insertions(+), 17 deletions(-) diff --git a/bufferevent-internal.h b/bufferevent-internal.h index 400c6d96..ad2c25c7 100644 --- a/bufferevent-internal.h +++ b/bufferevent-internal.h @@ -104,6 +104,10 @@ struct bufferevent_rate_limit_group { /** Timeout event that goes off once a tick, when the bucket is ready * to refill. */ struct event master_refill_event; + + /** Seed for weak random number generator. */ + ev_uint32_t weakrand_seed; + /** Lock to protect the members of this group. This lock should nest * within every bufferevent lock: if you are holding this lock, do * not assume you can lock another bufferevent. */ diff --git a/bufferevent_ratelim.c b/bufferevent_ratelim.c index 02c50228..4272c0da 100644 --- a/bufferevent_ratelim.c +++ b/bufferevent_ratelim.c @@ -452,7 +452,7 @@ bev_group_random_element_(struct bufferevent_rate_limit_group *group) EVUTIL_ASSERT(! LIST_EMPTY(&group->members)); - which = evutil_weakrand_() % group->n_members; + which = evutil_weakrand_range_(&group->weakrand_seed, group->n_members); bev = LIST_FIRST(&group->members); while (which--) diff --git a/event-internal.h b/event-internal.h index f840058d..4c882056 100644 --- a/event-internal.h +++ b/event-internal.h @@ -290,6 +290,9 @@ struct event_base { struct event th_notify; /** A function used to wake up the main thread from another thread. */ int (*th_notify_fn)(struct event_base *base); + + /* Saved seed for weak random number generator. */ + ev_uint32_t weakrand_seed; }; struct event_config_entry { diff --git a/evutil.c b/evutil.c index abe15604..a87b5246 100644 --- a/evutil.c +++ b/evutil.c @@ -2273,14 +2273,23 @@ evutil_getenv_(const char *varname) return getenv(varname); } -long -evutil_weakrand_(void) +ev_uint32_t +evutil_weakrand_(ev_uint32_t* seed) { -#ifdef _WIN32 - return rand(); -#else - return random(); -#endif + *seed = ((*seed) * 1103515245 + 12345) & 0x7fffffff; + return (*seed); +} + +ev_uint32_t +evutil_weakrand_range_(ev_uint32_t* seed, ev_uint32_t top) +{ + ev_uint32_t divisor, result; + + divisor = EV_INT32_MAX / top; + do + result = evutil_weakrand_(seed) / divisor; + while (result > top); + return result; } int diff --git a/poll.c b/poll.c index 744c7141..3ac427ac 100644 --- a/poll.c +++ b/poll.c @@ -183,7 +183,7 @@ poll_dispatch(struct event_base *base, struct timeval *tv) if (res == 0 || nfds == 0) return (0); - i = random() % nfds; + i = evutil_weakrand_range_(&base->weakrand_seed, nfds); for (j = 0; j < nfds; j++) { int what; if (++i == nfds) diff --git a/select.c b/select.c index e1d4987c..f9a0c206 100644 --- a/select.c +++ b/select.c @@ -186,7 +186,7 @@ select_dispatch(struct event_base *base, struct timeval *tv) event_debug(("%s: select reports %d", __func__, res)); check_selectop(sop); - i = random() % nfds; + i = evutil_weakrand_range_(&base->weakrand_seed, nfds); for (j = 0; j < nfds; ++j) { if (++i >= nfds) i = 0; diff --git a/test/regress_buffer.c b/test/regress_buffer.c index dfb680b5..2e4ef176 100644 --- a/test/regress_buffer.c +++ b/test/regress_buffer.c @@ -699,6 +699,7 @@ test_evbuffer_add_file(void *ptr) struct event *rev=NULL, *wev=NULL; struct event_base *base = testdata->base; evutil_socket_t pair[2] = {-1, -1}; + static ev_uint32_t seed = 123456789U; /* This test is highly parameterized based on substrings of its * argument. The strings are: */ @@ -757,7 +758,7 @@ test_evbuffer_add_file(void *ptr) data = malloc(1024*512); tt_assert(data); for (i = 0; i < datalen; ++i) - data[i] = (char)evutil_weakrand_(); + data[i] = (char)evutil_weakrand_(&seed); } else { data = strdup("here is a relatively small string."); tt_assert(data); diff --git a/test/regress_util.c b/test/regress_util.c index e7662e0f..2d53ba8d 100644 --- a/test/regress_util.c +++ b/test/regress_util.c @@ -829,6 +829,7 @@ test_evutil_rand(void *arg) char buf2[32]; int counts[256]; int i, j, k, n=0; + static ev_uint32_t seed = 12346789U; memset(buf2, 0, sizeof(buf2)); memset(counts, 0, sizeof(counts)); @@ -836,8 +837,8 @@ test_evutil_rand(void *arg) for (k=0;k<32;++k) { /* Try a few different start and end points; try to catch * the various misaligned cases of arc4random_buf */ - int startpoint = evutil_weakrand_() % 4; - int endpoint = 32 - (evutil_weakrand_() % 4); + int startpoint = evutil_weakrand_(&seed) % 4; + int endpoint = 32 - (evutil_weakrand_(&seed) % 4); memset(buf2, 0, sizeof(buf2)); diff --git a/util-internal.h b/util-internal.h index 508eae92..291feb8a 100644 --- a/util-internal.h +++ b/util-internal.h @@ -254,7 +254,8 @@ int evutil_resolve_(int family, const char *hostname, struct sockaddr *sa, const char *evutil_getenv_(const char *name); -long evutil_weakrand_(void); +ev_uint32_t evutil_weakrand_(ev_uint32_t* seed); +ev_uint32_t evutil_weakrand_range_(ev_uint32_t* seed, ev_uint32_t top); /* Evaluates to the same boolean value as 'p', and hints to the compiler that * we expect this value to be false. */ diff --git a/win32select.c b/win32select.c index 441222f1..ce2d525f 100644 --- a/win32select.c +++ b/win32select.c @@ -326,7 +326,8 @@ win32_dispatch(struct event_base *base, struct timeval *tv) } if (win32op->readset_out->fd_count) { - i = rand() % win32op->readset_out->fd_count; + i = evutil_weakrand_range_(&base->weakrand_seed, + win32op->readset_out->fd_count); for (j=0; jreadset_out->fd_count; ++j) { if (++i >= win32op->readset_out->fd_count) i = 0; @@ -335,7 +336,8 @@ win32_dispatch(struct event_base *base, struct timeval *tv) } } if (win32op->exset_out->fd_count) { - i = rand() % win32op->exset_out->fd_count; + i = evutil_weakrand_range_(&base->weakrand_seed, + win32op->exset_out->fd_count); for (j=0; jexset_out->fd_count; ++j) { if (++i >= win32op->exset_out->fd_count) i = 0; @@ -345,7 +347,8 @@ win32_dispatch(struct event_base *base, struct timeval *tv) } if (win32op->writeset_out->fd_count) { SOCKET s; - i = rand() % win32op->writeset_out->fd_count; + i = evutil_weakrand_range_(&base->weakrand_seed, + win32op->writeset_out->fd_count); for (j=0; jwriteset_out->fd_count; ++j) { if (++i >= win32op->writeset_out->fd_count) i = 0; From 3aa44159c53af1d6459a85dc1841c7c494d52464 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 9 Apr 2012 11:30:46 -0400 Subject: [PATCH 2/2] Tweak the new evutil_weakrand_() code Make its state actually get seeded. Document it more thoroughly. Turn its state into a structure. Fix a bug in evutil_weakrand_range_() where it could return the top of the range. Change its return type to ev_int32_t. Add a quick unit test to make sure that the value of evutil_weakrand_range_() is in range. --- bufferevent-internal.h | 4 ++-- bufferevent_ratelim.c | 8 ++++++- event-internal.h | 5 +++-- evutil.c | 49 +++++++++++++++++++++++++++++++++--------- poll.c | 2 ++ select.c | 2 ++ test/regress_buffer.c | 2 +- test/regress_util.c | 9 +++++++- util-internal.h | 29 +++++++++++++++++++++++-- win32select.c | 2 ++ 10 files changed, 93 insertions(+), 19 deletions(-) diff --git a/bufferevent-internal.h b/bufferevent-internal.h index ad2c25c7..461c46e7 100644 --- a/bufferevent-internal.h +++ b/bufferevent-internal.h @@ -105,8 +105,8 @@ struct bufferevent_rate_limit_group { * to refill. */ struct event master_refill_event; - /** Seed for weak random number generator. */ - ev_uint32_t weakrand_seed; + /** Seed for weak random number generator. Protected by 'lock' */ + struct evutil_weakrand_state weakrand_seed; /** Lock to protect the members of this group. This lock should nest * within every bufferevent lock: if you are holding this lock, do diff --git a/bufferevent_ratelim.c b/bufferevent_ratelim.c index 4272c0da..f7de86a9 100644 --- a/bufferevent_ratelim.c +++ b/bufferevent_ratelim.c @@ -438,7 +438,10 @@ bev_refill_callback_(evutil_socket_t fd, short what, void *arg) BEV_UNLOCK(&bev->bev); } -/** Helper: grab a random element from a bufferevent group. */ +/** Helper: grab a random element from a bufferevent group. + * + * Requires that we hold the lock on the group. + */ static struct bufferevent_private * bev_group_random_element_(struct bufferevent_rate_limit_group *group) { @@ -660,6 +663,9 @@ bufferevent_rate_limit_group_new(struct event_base *base, bufferevent_rate_limit_group_set_min_share(g, 64); + evutil_weakrand_seed_(&g->weakrand_seed, + (ev_uint32_t) ((now.tv_sec + now.tv_usec) + (ev_intptr_t)g)); + return g; } diff --git a/event-internal.h b/event-internal.h index 4c882056..78b3fe63 100644 --- a/event-internal.h +++ b/event-internal.h @@ -291,8 +291,9 @@ struct event_base { /** A function used to wake up the main thread from another thread. */ int (*th_notify_fn)(struct event_base *base); - /* Saved seed for weak random number generator. */ - ev_uint32_t weakrand_seed; + /** Saved seed for weak random number generator. Some backends use + * this to produce fairness among sockets. Protected by th_base_lock. */ + struct evutil_weakrand_state weakrand_seed; }; struct event_config_entry { diff --git a/evutil.c b/evutil.c index a87b5246..9307d259 100644 --- a/evutil.c +++ b/evutil.c @@ -2274,21 +2274,50 @@ evutil_getenv_(const char *varname) } ev_uint32_t -evutil_weakrand_(ev_uint32_t* seed) +evutil_weakrand_seed_(struct evutil_weakrand_state *state, ev_uint32_t seed) { - *seed = ((*seed) * 1103515245 + 12345) & 0x7fffffff; - return (*seed); + if (seed == 0) { + struct timeval tv; + evutil_gettimeofday(&tv, NULL); + seed = (ev_uint32_t)tv.tv_sec + (ev_uint32_t)tv.tv_usec; +#ifdef _WIN32 + seed += (ev_uint32_t) _getpid(); +#else + seed += (ev_uint32_t) getpid(); +#endif + } + state->seed = seed; + return seed; } -ev_uint32_t -evutil_weakrand_range_(ev_uint32_t* seed, ev_uint32_t top) +ev_int32_t +evutil_weakrand_(struct evutil_weakrand_state *state) { - ev_uint32_t divisor, result; + /* This RNG implementation is a linear congruential generator, with + * modulus 2^31, multiplier 1103515245, and addend 12345. It's also + * used by OpenBSD, and by Glibc's TYPE_0 RNG. + * + * The linear congruential generator is not an industrial-strength + * RNG! It's fast, but it can have higher-order patterns. Notably, + * the low bits tend to have periodicity. + */ + state->seed = ((state->seed) * 1103515245 + 12345) & 0x7fffffff; + return (ev_int32_t)(state->seed); +} - divisor = EV_INT32_MAX / top; - do - result = evutil_weakrand_(seed) / divisor; - while (result > top); +ev_int32_t +evutil_weakrand_range_(struct evutil_weakrand_state *state, ev_int32_t top) +{ + ev_int32_t divisor, result; + + /* We can't just do weakrand() % top, since the low bits of the LCG + * are less random than the high ones. (Specifically, since the LCG + * modulus is 2^N, every 2^m for m= top); return result; } diff --git a/poll.c b/poll.c index 3ac427ac..01144854 100644 --- a/poll.c +++ b/poll.c @@ -93,6 +93,8 @@ poll_init(struct event_base *base) evsig_init_(base); + evutil_weakrand_seed_(&base->weakrand_seed, 0); + return (pollop); } diff --git a/select.c b/select.c index f9a0c206..8ae53cc1 100644 --- a/select.c +++ b/select.c @@ -121,6 +121,8 @@ select_init(struct event_base *base) evsig_init_(base); + evutil_weakrand_seed_(&base->weakrand_seed, 0); + return (sop); } diff --git a/test/regress_buffer.c b/test/regress_buffer.c index 2e4ef176..d183b4f9 100644 --- a/test/regress_buffer.c +++ b/test/regress_buffer.c @@ -699,7 +699,7 @@ test_evbuffer_add_file(void *ptr) struct event *rev=NULL, *wev=NULL; struct event_base *base = testdata->base; evutil_socket_t pair[2] = {-1, -1}; - static ev_uint32_t seed = 123456789U; + struct evutil_weakrand_state seed = { 123456789U }; /* This test is highly parameterized based on substrings of its * argument. The strings are: */ diff --git a/test/regress_util.c b/test/regress_util.c index 2d53ba8d..6aaeeacf 100644 --- a/test/regress_util.c +++ b/test/regress_util.c @@ -829,7 +829,7 @@ test_evutil_rand(void *arg) char buf2[32]; int counts[256]; int i, j, k, n=0; - static ev_uint32_t seed = 12346789U; + struct evutil_weakrand_state seed = { 12346789U }; memset(buf2, 0, sizeof(buf2)); memset(counts, 0, sizeof(counts)); @@ -869,6 +869,13 @@ test_evutil_rand(void *arg) } } + evutil_weakrand_seed_(&seed, 0); + for (i = 0; i < 10000; ++i) { + ev_int32_t r = evutil_weakrand_range_(&seed, 9999); + tt_int_op(0, <=, r); + tt_int_op(r, <, 9999); + } + /* for (i=0;i<256;++i) { printf("%3d %2d\n", i, counts[i]); } */ end: ; diff --git a/util-internal.h b/util-internal.h index 291feb8a..5248ab21 100644 --- a/util-internal.h +++ b/util-internal.h @@ -254,8 +254,33 @@ int evutil_resolve_(int family, const char *hostname, struct sockaddr *sa, const char *evutil_getenv_(const char *name); -ev_uint32_t evutil_weakrand_(ev_uint32_t* seed); -ev_uint32_t evutil_weakrand_range_(ev_uint32_t* seed, ev_uint32_t top); +/* Structure to hold the state of our weak random number generator. + */ +struct evutil_weakrand_state { + ev_uint32_t seed; +}; + +#define EVUTIL_WEAKRAND_MAX EV_INT32_MAX + +/* Initialize the state of a week random number generator based on 'seed'. If + * the seed is 0, construct a new seed based on not-very-strong platform + * entropy, like the PID and the time of day. + * + * This function, and the other evutil_weakrand* functions, are meant for + * speed, not security or statistical strength. If you need a RNG which an + * attacker can't predict, or which passes strong statistical tests, use the + * evutil_secure_rng* functions instead. + */ +ev_uint32_t evutil_weakrand_seed_(struct evutil_weakrand_state *state, ev_uint32_t seed); +/* Return a pseudorandom value between 0 and EVUTIL_WEAKRAND_MAX inclusive. + * Updates the state in 'seed' as needed -- this value must be protected by a + * lock. + */ +ev_int32_t evutil_weakrand_(struct evutil_weakrand_state *seed); +/* Return a pseudorandom value x such that 0 <= x < top. top must be no more + * than EVUTIL_WEAKRAND_MAX. Updates the state in 'seed' as needed -- this + * value must be proteced by a lock */ +ev_int32_t evutil_weakrand_range_(struct evutil_weakrand_state *seed, ev_int32_t top); /* Evaluates to the same boolean value as 'p', and hints to the compiler that * we expect this value to be false. */ diff --git a/win32select.c b/win32select.c index ce2d525f..7be2389f 100644 --- a/win32select.c +++ b/win32select.c @@ -202,6 +202,8 @@ win32_init(struct event_base *base) if (evsig_init_(base) < 0) winop->signals_are_broken = 1; + evutil_weakrand_seed_(&base->weakrand_seed, 0); + return (winop); err: XFREE(winop->readset_in);