From a62346dec779c7262ded5afdf2fac17a5eac6674 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 15 Nov 2011 18:33:50 -0500 Subject: [PATCH 1/6] Revert "openssl bufferevent has the same issue with writing as prior commit." This reverts commit 7353663eb7c0b2a1caaaa5acd818515f156cf2ca. --- bufferevent_openssl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bufferevent_openssl.c b/bufferevent_openssl.c index 8f2c3dfd..3843b314 100644 --- a/bufferevent_openssl.c +++ b/bufferevent_openssl.c @@ -759,7 +759,7 @@ consider_writing(struct bufferevent_openssl *bev_ssl) target = bev_ssl->underlying->output; wm = &bev_ssl->underlying->wm_write; } - if ((bev_ssl->bev.bev.enabled & EV_WRITE) && + while ((bev_ssl->bev.bev.enabled & EV_WRITE) && (! bev_ssl->bev.write_suspended) && evbuffer_get_length(output) && (!target || (! wm->high || evbuffer_get_length(target) < wm->high))) { @@ -769,6 +769,8 @@ consider_writing(struct bufferevent_openssl *bev_ssl) else n_to_write = WRITE_FRAME; r = do_write(bev_ssl, n_to_write); + if (r <= 0) + break; } if (!bev_ssl->underlying) { From 6660c9a347d3557baee7b9b79d84ad4378be5009 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 15 Nov 2011 18:34:24 -0500 Subject: [PATCH 2/6] Revert "Avoid potential SSL read spinlocks" This reverts commit fc52dbac87f4937f8306759506d6a2ad15ca244c. --- bufferevent_openssl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bufferevent_openssl.c b/bufferevent_openssl.c index 3843b314..86a8619b 100644 --- a/bufferevent_openssl.c +++ b/bufferevent_openssl.c @@ -722,13 +722,15 @@ consider_reading(struct bufferevent_openssl *bev_ssl) } if (bev_ssl->write_blocked_on_read) return; - if ((bev_ssl->bev.bev.enabled & EV_READ) && + while ((bev_ssl->bev.bev.enabled & EV_READ) && (! bev_ssl->bev.read_suspended) && (! wm->high || evbuffer_get_length(input) < wm->high)) { int n_to_read = wm->high ? wm->high - evbuffer_get_length(input) : READ_DEFAULT; r = do_read(bev_ssl, n_to_read); + if (r <= 0) + break; } if (!bev_ssl->underlying) { From a186e7320054ed8f508a18876adc9f770c9c4e2e Mon Sep 17 00:00:00 2001 From: Mark Ellzey Date: Thu, 17 Nov 2011 11:45:49 -0500 Subject: [PATCH 3/6] Refactor amount-to-read calculations in buffervent_ssl consider_reading() Split up consider_reading()'s conditional checks into another function can_read() for simplicity sake. {Split into a separate patch by nickm} --- bufferevent_openssl.c | 43 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/bufferevent_openssl.c b/bufferevent_openssl.c index 86a8619b..7d822d9e 100644 --- a/bufferevent_openssl.c +++ b/bufferevent_openssl.c @@ -704,6 +704,38 @@ do_write(struct bufferevent_openssl *bev_ssl, int atmost) #define READ_DEFAULT 4096 +/* Try to figure out how many bytes to read; return 0 if we shouldn't be + * reading. */ +static int +bytes_to_read(struct bufferevent_openssl *bev) +{ + struct evbuffer *input = bev->bev.bev.input; + struct event_watermark *wm = &bev->bev.bev.wm_read; + + if (bev->write_blocked_on_read) { + return 0; + } + + if (! (bev->bev.bev.enabled & EV_READ)) { + return 0; + } + + if (bev->bev.read_suspended) { + return 0; + } + + if (wm->high) { + if (evbuffer_get_length(input) >= wm->high) { + return 0; + } + + return wm->high - evbuffer_get_length(input); + } + + return READ_DEFAULT; +} + + /* Things look readable. If write is blocked on read, write till it isn't. * Read from the underlying buffer until we block or we hit our high-water * mark. @@ -712,8 +744,7 @@ static void consider_reading(struct bufferevent_openssl *bev_ssl) { int r; - struct evbuffer *input = bev_ssl->bev.bev.input; - struct event_watermark *wm = &bev_ssl->bev.bev.wm_read; + int n_to_read; while (bev_ssl->write_blocked_on_read) { r = do_write(bev_ssl, WRITE_FRAME); @@ -722,12 +753,8 @@ consider_reading(struct bufferevent_openssl *bev_ssl) } if (bev_ssl->write_blocked_on_read) return; - while ((bev_ssl->bev.bev.enabled & EV_READ) && - (! bev_ssl->bev.read_suspended) && - (! wm->high || evbuffer_get_length(input) < wm->high)) { - int n_to_read = - wm->high ? wm->high - evbuffer_get_length(input) - : READ_DEFAULT; + + while ((n_to_read = bytes_to_read(bev_ssl)) != 0) { r = do_read(bev_ssl, n_to_read); if (r <= 0) break; From 96c562fa495ccd325b93a59affcba5d06de250ec Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 17 Nov 2011 11:54:07 -0500 Subject: [PATCH 4/6] Move SSL rate-limit enforcement into bytes_to_read() --- bufferevent_openssl.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/bufferevent_openssl.c b/bufferevent_openssl.c index 7d822d9e..07dcdf8e 100644 --- a/bufferevent_openssl.c +++ b/bufferevent_openssl.c @@ -711,6 +711,10 @@ bytes_to_read(struct bufferevent_openssl *bev) { struct evbuffer *input = bev->bev.bev.input; struct event_watermark *wm = &bev->bev.bev.wm_read; + int result = READ_DEFAULT; + ev_ssize_t limit; + /* XXX 99% of this is generic code that nearly all bufferevents will + * want. */ if (bev->write_blocked_on_read) { return 0; @@ -729,10 +733,18 @@ bytes_to_read(struct bufferevent_openssl *bev) return 0; } - return wm->high - evbuffer_get_length(input); + result = wm->high - evbuffer_get_length(input); + } else { + result = READ_DEFAULT; } - return READ_DEFAULT; + /* Respect the rate limit */ + limit = _bufferevent_get_read_max(&bev->bev); + if (result > limit) { + result = limit; + } + + return result; } From 2aa036fa0403977fa51812e711a1d5040c9fb5c9 Mon Sep 17 00:00:00 2001 From: Mark Ellzey Date: Thu, 17 Nov 2011 11:59:41 -0500 Subject: [PATCH 5/6] Avoid spinning on OpenSSL reads Previously, if some sender were generating data to read on an OpenSSL connection as fast as we could process it, we could easily wind up looping on an openssl do_read operation without ever considering other sockets. The difference between this and the original method in consider_reading() is that it only loops for a single completed *frame* instead of looping until fd is drained or an error condition was triggered. {Patch split out by nickm} --- bufferevent_openssl.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/bufferevent_openssl.c b/bufferevent_openssl.c index 07dcdf8e..53a4d686 100644 --- a/bufferevent_openssl.c +++ b/bufferevent_openssl.c @@ -766,10 +766,13 @@ consider_reading(struct bufferevent_openssl *bev_ssl) if (bev_ssl->write_blocked_on_read) return; - while ((n_to_read = bytes_to_read(bev_ssl)) != 0) { - r = do_read(bev_ssl, n_to_read); - if (r <= 0) + n_to_read = bytes_to_read(bev_ssl); + + while (n_to_read) { + if (do_read(bev_ssl, n_to_read) <= 0) break; + + n_to_read = SSL_pending(bev_ssl->ssl); } if (!bev_ssl->underlying) { From ebf82199dfc91c437b304649829fb0a259726d5c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 17 Nov 2011 17:42:45 -0500 Subject: [PATCH 6/6] add comment to new consider_reading code --- bufferevent_openssl.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/bufferevent_openssl.c b/bufferevent_openssl.c index 53a4d686..d703279c 100644 --- a/bufferevent_openssl.c +++ b/bufferevent_openssl.c @@ -772,6 +772,14 @@ consider_reading(struct bufferevent_openssl *bev_ssl) if (do_read(bev_ssl, n_to_read) <= 0) break; + /* Read all pending data. This won't hit the network + * again, and will (most importantly) put us in a state + * where we don't need to read anything else until the + * socket is readable again. It'll potentially make us + * overrun our read high-watermark (somewhat + * regrettable). The damage to the rate-limit has + * already been done, since OpenSSL went and read a + * whole SSL record anyway. */ n_to_read = SSL_pending(bev_ssl->ssl); }