From d047c2412ecb67187fe5edbf703f26f5d160c5f5 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Fri, 2 Dec 2016 18:32:03 +0300 Subject: [PATCH 1/5] test/ssl: cover case when we writing to be_openssl after connecting Right now it fails because of regression for filtered openssl bufferevent, and by it I mean ssl/bufferevent_filter_write_after_connect test, and by fails - hang. Regression-for: da52933550fd4736aa1c213b6de497e2ffc31e34 ("be_openssl: don't call do_write() directly from outbuf_cb") --- test/regress_ssl.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/test/regress_ssl.c b/test/regress_ssl.c index 8a5524a8..6d31cfce 100644 --- a/test/regress_ssl.c +++ b/test/regress_ssl.c @@ -227,6 +227,8 @@ enum regress_openssl_type REGRESS_OPENSSL_FREED = 256, REGRESS_OPENSSL_TIMEOUT = 512, REGRESS_OPENSSL_SLEEP = 1024, + + REGRESS_OPENSSL_CLIENT_WRITE = 2048, }; static void @@ -322,6 +324,9 @@ eventcb(struct bufferevent *bev, short what, void *ctx) if (--pending_connect_events == 0) event_base_loopexit(exit_base, NULL); } + + if ((type & REGRESS_OPENSSL_CLIENT_WRITE) && (type & REGRESS_OPENSSL_CLIENT)) + evbuffer_add_printf(bufferevent_get_output(bev), "1\n"); } else if (what & BEV_EVENT_EOF) { TT_BLATHER(("Got a good EOF")); ++got_close; @@ -465,7 +470,8 @@ regress_bufferevent_openssl(void *arg) bufferevent_enable(bev1, EV_READ|EV_WRITE); bufferevent_enable(bev2, EV_READ|EV_WRITE); - evbuffer_add_printf(bufferevent_get_output(bev1), "1\n"); + if (!(type & REGRESS_OPENSSL_CLIENT_WRITE)) + evbuffer_add_printf(bufferevent_get_output(bev1), "1\n"); event_base_dispatch(data->base); @@ -488,7 +494,8 @@ regress_bufferevent_openssl(void *arg) bufferevent_set_timeouts(bev1, &t, &t); - evbuffer_add_printf(bufferevent_get_output(bev1), "1\n"); + if (!(type & REGRESS_OPENSSL_CLIENT_WRITE)) + evbuffer_add_printf(bufferevent_get_output(bev1), "1\n"); event_base_dispatch(data->base); @@ -731,8 +738,14 @@ struct testcase_t ssl_testcases[] = { #define T(a) ((void *)(a)) { "bufferevent_socketpair", regress_bufferevent_openssl, TT_ISOLATED, &basic_setup, T(REGRESS_OPENSSL_SOCKETPAIR) }, + { "bufferevent_socketpair_write_after_connect", regress_bufferevent_openssl, + TT_ISOLATED, &basic_setup, + T(REGRESS_OPENSSL_SOCKETPAIR|REGRESS_OPENSSL_CLIENT_WRITE) }, { "bufferevent_filter", regress_bufferevent_openssl, TT_ISOLATED, &basic_setup, T(REGRESS_OPENSSL_FILTER) }, + { "bufferevent_filter_write_after_connect", regress_bufferevent_openssl, + TT_ISOLATED, &basic_setup, + T(REGRESS_OPENSSL_FILTER|REGRESS_OPENSSL_CLIENT_WRITE) }, { "bufferevent_renegotiate_socketpair", regress_bufferevent_openssl, TT_ISOLATED, &basic_setup, T(REGRESS_OPENSSL_SOCKETPAIR | REGRESS_OPENSSL_RENEGOTIATE) }, From d77fcea15fe8ebb166853597cfff934fd5a47b0f Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Wed, 7 Dec 2016 03:07:59 +0300 Subject: [PATCH 2/5] test/https: separate cases for https client with filtered openssl bufferevent - http/https_filter_chunk_out # now hang - http/https_filter_basic # works, since writes only before connect() --- test/regress_http.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/test/regress_http.c b/test/regress_http.c index b88ec1bc..80100500 100644 --- a/test/regress_http.c +++ b/test/regress_http.c @@ -85,6 +85,7 @@ static void http_on_complete_cb(struct evhttp_request *req, void *arg); #define HTTP_BIND_IPV6 1 #define HTTP_BIND_SSL 2 +#define HTTP_SSL_FILTER 4 static int http_bind(struct evhttp *myhttp, ev_uint16_t *pport, int mask) { @@ -424,18 +425,25 @@ http_complete_write(evutil_socket_t fd, short what, void *arg) } static struct bufferevent * -create_bev(struct event_base *base, int fd, int ssl) +create_bev(struct event_base *base, int fd, int ssl_mask) { int flags = BEV_OPT_DEFER_CALLBACKS; struct bufferevent *bev = NULL; - if (!ssl) { + if (!ssl_mask) { bev = bufferevent_socket_new(base, fd, flags); } else { #ifdef EVENT__HAVE_OPENSSL SSL *ssl = SSL_new(get_ssl_ctx()); - bev = bufferevent_openssl_socket_new( - base, fd, ssl, BUFFEREVENT_SSL_CONNECTING, flags); + if (ssl_mask & HTTP_SSL_FILTER) { + struct bufferevent *underlying = + bufferevent_socket_new(base, fd, flags); + bev = bufferevent_openssl_filter_new( + base, underlying, ssl, BUFFEREVENT_SSL_CONNECTING, flags); + } else { + bev = bufferevent_openssl_socket_new( + base, fd, ssl, BUFFEREVENT_SSL_CONNECTING, flags); + } bufferevent_openssl_set_allow_dirty_shutdown(bev, 1); #endif } @@ -4519,6 +4527,8 @@ http_request_own_test(void *arg) #ifdef EVENT__HAVE_OPENSSL static void https_basic_test(void *arg) { return http_basic_test_impl(arg, 1); } +static void https_filter_basic_test(void *arg) +{ return http_basic_test_impl(arg, 1 | HTTP_SSL_FILTER); } static void https_incomplete_test(void *arg) { http_incomplete_test_(arg, 0, 1); } static void https_incomplete_timeout_test(void *arg) @@ -4533,6 +4543,8 @@ static void https_connection_retry_test(void *arg) { return http_connection_retry_test_impl(arg, 1); } static void https_chunk_out_test(void *arg) { return http_chunk_out_test_impl(arg, 1); } +static void https_filter_chunk_out_test(void *arg) +{ return http_chunk_out_test_impl(arg, 1 | HTTP_SSL_FILTER); } static void https_stream_out_test(void *arg) { return http_stream_out_test_impl(arg, 1); } static void https_connection_fail_test(void *arg) @@ -4620,6 +4632,7 @@ struct testcase_t http_testcases[] = { #ifdef EVENT__HAVE_OPENSSL HTTPS(basic), + HTTPS(filter_basic), HTTPS(simple), HTTPS(simple_dirty), HTTPS(incomplete), @@ -4628,6 +4641,7 @@ struct testcase_t http_testcases[] = { { "https_connection_retry_conn_address", https_connection_retry_conn_address_test, TT_ISOLATED|TT_OFF_BY_DEFAULT, &basic_setup, NULL }, HTTPS(chunk_out), + HTTPS(filter_chunk_out), HTTPS(stream_out), HTTPS(connection_fail), HTTPS(write_during_read), From 8939676706b8e2a125c5e3344f8672ddfadeb4e1 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Wed, 7 Dec 2016 02:53:07 +0300 Subject: [PATCH 3/5] be_openssl: Fix writing into filted openssl bufferevent after connected The main problems was due to when bufferevent_openssl has underlying (i.e. created with bufferevent_openssl_filter_new()) some events was disabled/suspended, while with openssl, READ can require WRITE and vice-versa hence this issues. The BEV_CTRL_GET_FD hunk to fix http subsystem, since it depends from what bufferevent_getfd() returns. Fixes: #428 Fixes: ssl/bufferevent_filter_write_after_connect Fixes: http/https_filter_chunk_out Fixes: da52933550fd4736aa1c213b6de497e2ffc31e34 ("be_openssl: don't call do_write() directly from outbuf_cb") --- bufferevent_openssl.c | 57 +++++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/bufferevent_openssl.c b/bufferevent_openssl.c index cbb7f8a5..da3963af 100644 --- a/bufferevent_openssl.c +++ b/bufferevent_openssl.c @@ -404,7 +404,10 @@ start_writing(struct bufferevent_openssl *bev_ssl) { int r = 0; if (bev_ssl->underlying) { - ; + if (bev_ssl->write_blocked_on_read) { + bufferevent_unsuspend_read_(bev_ssl->underlying, + BEV_SUSPEND_FILT_READ); + } } else { struct bufferevent *bev = &bev_ssl->bev.bev; r = bufferevent_add_event_(&bev->ev_write, &bev->timeout_write); @@ -435,7 +438,8 @@ stop_writing(struct bufferevent_openssl *bev_ssl) if (bev_ssl->read_blocked_on_write) return; if (bev_ssl->underlying) { - ; + bufferevent_unsuspend_read_(bev_ssl->underlying, + BEV_SUSPEND_FILT_READ); } else { struct bufferevent *bev = &bev_ssl->bev.bev; event_del(&bev->ev_write); @@ -716,7 +720,7 @@ do_write(struct bufferevent_openssl *bev_ssl, int atmost) if (bev_ssl->underlying) BEV_RESET_GENERIC_WRITE_TIMEOUT(bev); - bufferevent_trigger_nolock_(bev, EV_WRITE, 0); + bufferevent_trigger_nolock_(bev, EV_WRITE, BEV_OPT_DEFER_CALLBACKS); } return result; } @@ -1040,17 +1044,11 @@ do_handshake(struct bufferevent_openssl *bev_ssl) print_err(err); switch (err) { case SSL_ERROR_WANT_WRITE: - if (!bev_ssl->underlying) { - stop_reading(bev_ssl); - return start_writing(bev_ssl); - } - return 0; + stop_reading(bev_ssl); + return start_writing(bev_ssl); case SSL_ERROR_WANT_READ: - if (!bev_ssl->underlying) { - stop_writing(bev_ssl); - return start_reading(bev_ssl); - } - return 0; + stop_writing(bev_ssl); + return start_reading(bev_ssl); default: conn_closed(bev_ssl, BEV_EVENT_READING, err, r); return -1; @@ -1086,6 +1084,13 @@ set_handshake_callbacks(struct bufferevent_openssl *bev_ssl, evutil_socket_t fd) be_openssl_handshakecb, be_openssl_handshakecb, be_openssl_eventcb, bev_ssl); + + if (fd < 0) + return 0; + + if (bufferevent_setfd(bev_ssl->underlying, fd)) + return 1; + return do_handshake(bev_ssl); } else { struct bufferevent *bev = &bev_ssl->bev.bev; @@ -1131,10 +1136,13 @@ be_openssl_outbuf_cb(struct evbuffer *buf, int r = 0; /* XXX need to hold a reference here. */ - if (cbinfo->n_added && bev_ssl->state == BUFFEREVENT_SSL_OPEN && - cbinfo->orig_size == 0) { - r = bufferevent_add_event_(&bev_ssl->bev.bev.ev_write, - &bev_ssl->bev.bev.timeout_write); + if (cbinfo->n_added && bev_ssl->state == BUFFEREVENT_SSL_OPEN) { + if (cbinfo->orig_size == 0) + r = bufferevent_add_event_(&bev_ssl->bev.bev.ev_write, + &bev_ssl->bev.bev.timeout_write); + + if (bev_ssl->underlying) + consider_writing(bev_ssl); } /* XXX Handle r < 0 */ (void)r; @@ -1286,17 +1294,24 @@ be_openssl_ctrl(struct bufferevent *bev, struct bufferevent_openssl *bev_ssl = upcast(bev); switch (op) { case BEV_CTRL_SET_FD: - if (bev_ssl->underlying) - return -1; - { + if (!bev_ssl->underlying) { BIO *bio; bio = BIO_new_socket(data->fd, 0); SSL_set_bio(bev_ssl->ssl, bio, bio); + } else { + BIO *bio; + if (!(bio = BIO_new_bufferevent(bev_ssl->underlying, 0))) + return -1; + SSL_set_bio(bev_ssl->ssl, bio, bio); } return be_openssl_set_fd(bev_ssl, bev_ssl->old_state, data->fd); case BEV_CTRL_GET_FD: - data->fd = event_get_fd(&bev->ev_read); + if (bev_ssl->underlying) { + data->fd = event_get_fd(&bev_ssl->underlying->ev_read); + } else { + data->fd = event_get_fd(&bev->ev_read); + } return 0; case BEV_CTRL_GET_UNDERLYING: data->ptr = bev_ssl->underlying; From 09b620130408b7456485089e3919b8cba3768dae Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Thu, 8 Dec 2016 02:11:22 +0300 Subject: [PATCH 4/5] test/ssl: fix bufferevent_getfd() for bufferevent_openssl_filter_new() --- test/regress_ssl.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/test/regress_ssl.c b/test/regress_ssl.c index 6d31cfce..681705fc 100644 --- a/test/regress_ssl.c +++ b/test/regress_ssl.c @@ -234,14 +234,13 @@ enum regress_openssl_type static void bufferevent_openssl_check_fd(struct bufferevent *bev, int filter) { + tt_int_op(bufferevent_getfd(bev), !=, -1); + tt_int_op(bufferevent_setfd(bev, -1), ==, 0); if (filter) { - tt_int_op(bufferevent_getfd(bev), ==, -1); - tt_int_op(bufferevent_setfd(bev, -1), ==, -1); - } else { tt_int_op(bufferevent_getfd(bev), !=, -1); - tt_int_op(bufferevent_setfd(bev, -1), ==, 0); + } else { + tt_int_op(bufferevent_getfd(bev), ==, -1); } - tt_int_op(bufferevent_getfd(bev), ==, -1); end: ; From 9a0a3a3e6510b67c378aade2e8e6097b70ff6daa Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 16 Jan 2017 02:31:54 +0300 Subject: [PATCH 5/5] be: fix with filtered bufferevents and connect() without EAGAIN With filtered bufferevents (i.e. not real one, that have socket), we can trigger incorrect callback in this case. Let's look at example with http and bufferevent_openssl_filter_new(): - bev = bufferevent_openssl_filter_new() - http layer trying to connect() to localhost with bev # at this time, bev have writecb/readcb NULL but ev_write/ev_read has # timeout with 45 secs, default HTTP connect timeout - and when connect() retruns without EAGAIN (BSD'ism) we called event_active() before (with EV_WRITE), and this will call ev_write timeout only, while it is more correct to act on bufferevent instead of plain event, so let's trigger EV_WRITE for bufferevent which will do the job (and let's do this deferred). Fixes: http/https_simple # under solaris --- bufferevent_sock.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/bufferevent_sock.c b/bufferevent_sock.c index 58095f37..93aedb33 100644 --- a/bufferevent_sock.c +++ b/bufferevent_sock.c @@ -249,8 +249,8 @@ bufferevent_writecb(evutil_socket_t fd, short event, void *arg) /* we need to fake the error if the connection was refused * immediately - usually connection to localhost on BSD */ if (bufev_p->connection_refused) { - bufev_p->connection_refused = 0; - c = -1; + bufev_p->connection_refused = 0; + c = -1; } if (c == 0) @@ -438,13 +438,12 @@ bufferevent_socket_connect(struct bufferevent *bev, /* The connect succeeded already. How very BSD of it. */ result = 0; bufev_p->connecting = 1; - event_active(&bev->ev_write, EV_WRITE, 1); + bufferevent_trigger_nolock_(bev, EV_WRITE, BEV_OPT_DEFER_CALLBACKS); } else { /* The connect failed already. How very BSD of it. */ - bufev_p->connection_refused = 1; - bufev_p->connecting = 1; result = 0; - event_active(&bev->ev_write, EV_WRITE, 1); + bufferevent_run_eventcb_(bev, BEV_EVENT_ERROR, BEV_OPT_DEFER_CALLBACKS); + bufferevent_disable(bev, EV_WRITE|EV_READ); } goto done;