From 4e62cd167b66f33e20344bff776253f19e2bc3e6 Mon Sep 17 00:00:00 2001 From: Mark Ellzey Date: Fri, 30 Mar 2012 15:08:40 -0400 Subject: [PATCH 1/2] Fixed potential double-readcb execution with openssl bufferevents. the function do_read() will call SSL_read(), and if successful, will call _bufferevent_run_readcb() before returning to consider_reading(). consider_reading() will then check SSL_pending() to make sure all pending data has also been read. If it does not, do_read() is called again. The issue with this is the possibility that the function that is executed by _bufferevent_run_readcb() called bufferevent_disable(ssl_bev, EV_READ) before the second do_read(); In this case, the users read callback is executed a second time. This is potentially bad for applications that expect the bufferevent to stay disabled until further notice. (this is why running openssl bufferevents without DEFER_CALLBACKS has always been troublesome). --- bufferevent_openssl.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/bufferevent_openssl.c b/bufferevent_openssl.c index 412c08e5..bdc363e5 100644 --- a/bufferevent_openssl.c +++ b/bufferevent_openssl.c @@ -615,9 +615,6 @@ do_read(struct bufferevent_openssl *bev_ssl, int n_to_read) evbuffer_commit_space(input, space, n_used); if (bev_ssl->underlying) BEV_RESET_GENERIC_READ_TIMEOUT(bev); - - if (evbuffer_get_length(input) >= bev->wm_read.low) - _bufferevent_run_readcb(bev); } return blocked ? 0 : 1; @@ -757,6 +754,7 @@ consider_reading(struct bufferevent_openssl *bev_ssl) { int r; int n_to_read; + int read_data = 0; while (bev_ssl->write_blocked_on_read) { r = do_write(bev_ssl, WRITE_FRAME); @@ -772,6 +770,8 @@ consider_reading(struct bufferevent_openssl *bev_ssl) if (do_read(bev_ssl, n_to_read) <= 0) break; + read_data = 1; + /* 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 @@ -800,6 +800,15 @@ consider_reading(struct bufferevent_openssl *bev_ssl) n_to_read = bytes_to_read(bev_ssl); } + if (read_data == 1) { + struct bufferevent *bev = &bev_ssl->bev.bev; + struct evbuffer *input = bev->input; + + if (evbuffer_get_length(input) >= bev->wm_read.low) { + _bufferevent_run_readcb(bev); + } + } + if (!bev_ssl->underlying) { /* Should be redundant, but let's avoid busy-looping */ if (bev_ssl->bev.read_suspended || @@ -821,6 +830,14 @@ consider_writing(struct bufferevent_openssl *bev_ssl) r = do_read(bev_ssl, 1024); /* XXXX 1024 is a hack */ if (r <= 0) break; + else { + struct bufferevent *bev = &bev_ssl->bev.bev; + struct evbuffer *input = bev->input; + + if (evbuffer_get_length(input) >= bev->wm_read.low) { + _bufferevent_run_readcb(bev); + } + } } if (bev_ssl->read_blocked_on_write) return; From b3887cdf3b2ca715a1e496b7dcf2ee583e236338 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 11 Apr 2012 21:33:27 -0400 Subject: [PATCH 2/2] Work-around a stupid gcov-breaking bug in OSX 10.6 This only affects the unit tests. Fix found at http://rachelbythebay.com/w/2011/07/12/forkcrash/ (Backport from 2.1) --- test/regress.c | 2 +- test/regress.h | 2 ++ test/regress_main.c | 20 ++++++++++++++++++++ test/regress_util.c | 2 +- test/tinytest.c | 12 ++++++++++++ 5 files changed, 36 insertions(+), 2 deletions(-) diff --git a/test/regress.c b/test/regress.c index 3d97d0b2..90f4549c 100644 --- a/test/regress.c +++ b/test/regress.c @@ -851,7 +851,7 @@ test_fork(void) event_base_assert_ok(current_base); TT_BLATHER(("Before fork")); - if ((pid = fork()) == 0) { + if ((pid = regress_fork()) == 0) { /* in the child */ TT_BLATHER(("In child, before reinit")); event_base_assert_ok(current_base); diff --git a/test/regress.h b/test/regress.h index 32adccdf..afcc35fc 100644 --- a/test/regress.h +++ b/test/regress.h @@ -118,6 +118,8 @@ int _test_ai_eq(const struct evutil_addrinfo *ai, const char *sockaddr_port, long timeval_msec_diff(const struct timeval *start, const struct timeval *end); +pid_t regress_fork(void); + #ifdef __cplusplus } #endif diff --git a/test/regress_main.c b/test/regress_main.c index 42a0366a..32557c1e 100644 --- a/test/regress_main.c +++ b/test/regress_main.c @@ -32,6 +32,14 @@ #include #endif +#if defined(__APPLE__) && defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__) +#if (__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ >= 1060 && \ + __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ < 1070) +#define FORK_BREAKS_GCOV +#include +#endif +#endif + #include "event2/event-config.h" #ifdef _EVENT___func__ @@ -154,6 +162,18 @@ regress_make_tmpfile(const void *data, size_t datalen) #endif } +#ifndef _WIN32 +pid_t +regress_fork(void) +{ + pid_t pid = fork(); +#ifdef FORK_BREAKS_GCOV + vproc_transaction_begin(0); +#endif + return pid; +} +#endif + static void ignore_log_cb(int s, const char *msg) { diff --git a/test/regress_util.c b/test/regress_util.c index 88d80577..71830a6f 100644 --- a/test/regress_util.c +++ b/test/regress_util.c @@ -469,7 +469,7 @@ check_error_logging(void (*fn)(void), int wantexitcode, int status = 0, exitcode; fatal_want_severity = wantseverity; fatal_want_message = wantmsg; - if ((pid = fork()) == 0) { + if ((pid = regress_fork()) == 0) { /* child process */ fn(); exit(0); /* should be unreachable. */ diff --git a/test/tinytest.c b/test/tinytest.c index 91cd8cb1..065bd623 100644 --- a/test/tinytest.c +++ b/test/tinytest.c @@ -40,6 +40,15 @@ #include #endif +#if defined(__APPLE__) && defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__) +#if (__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ >= 1060 && \ + __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ < 1070) +/* Workaround for a stupid bug in OSX 10.6 */ +#define FORK_BREAKS_GCOV +#include +#endif +#endif + #ifndef __GNUC__ #define __attribute__(x) #endif @@ -161,6 +170,9 @@ _testcase_run_forked(const struct testgroup_t *group, if (opt_verbosity>0) printf("[forking] "); pid = fork(); +#ifdef FORK_BREAKS_GCOV + vproc_transaction_begin(0); +#endif if (!pid) { /* child. */ int test_r, write_r;