From 40b03798338b582b9ac467a1bc4b6e98b42d5396 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Sat, 22 Aug 2015 21:38:18 +0300 Subject: [PATCH 1/5] be_openssl: get rid off hackish "fd_is_set", to fix some corner cases This patch is a cleanup and a bug fix, it drops ```fd_is_set``` flag, and replace it with some checks to event_initialized(), and now we will not call event_assign() on already added event, plus we will delete event when we really have to (this patch fixes the case when server is down, IOW before this patch we will not call event_del() because ```fd_is_set``` was reset to 0) and this will fix some issues with retries in http layer for ssl. Reported-in: #258 Fixes: regress ssl/bufferevent_socketpair_timeout Fixes: regress ssl/bufferevent_socketpair_timeout_freed_fd --- bufferevent_openssl.c | 71 +++++++++++++++++++++---------------------- 1 file changed, 35 insertions(+), 36 deletions(-) diff --git a/bufferevent_openssl.c b/bufferevent_openssl.c index b30f90ff..2fdd193b 100644 --- a/bufferevent_openssl.c +++ b/bufferevent_openssl.c @@ -320,8 +320,6 @@ struct bufferevent_openssl { unsigned write_blocked_on_read : 1; /* Treat TCP close before SSL close on SSL >= v3 as clean EOF. */ unsigned allow_dirty_shutdown : 1; - /* XXXX */ - unsigned fd_is_set : 1; /* XXX */ unsigned n_errors : 2; @@ -968,27 +966,27 @@ set_open_callbacks(struct bufferevent_openssl *bev_ssl, evutil_socket_t fd) } else { struct bufferevent *bev = &bev_ssl->bev.bev; int rpending=0, wpending=0, r1=0, r2=0; - if (fd < 0 && bev_ssl->fd_is_set) - fd = event_get_fd(&bev->ev_read); - if (bev_ssl->fd_is_set) { + int new_fd = fd < 0 ? event_get_fd(&bev->ev_read) : fd; + + if (fd >= 0 && event_initialized(&bev->ev_read)) { rpending = event_pending(&bev->ev_read, EV_READ, NULL); wpending = event_pending(&bev->ev_write, EV_WRITE, NULL); event_del(&bev->ev_read); event_del(&bev->ev_write); } - event_assign(&bev->ev_read, bev->ev_base, fd, + + event_assign(&bev->ev_read, bev->ev_base, new_fd, EV_READ|EV_PERSIST|EV_FINALIZE, be_openssl_readeventcb, bev_ssl); - event_assign(&bev->ev_write, bev->ev_base, fd, + event_assign(&bev->ev_write, bev->ev_base, new_fd, EV_WRITE|EV_PERSIST|EV_FINALIZE, be_openssl_writeeventcb, bev_ssl); + if (rpending) r1 = bufferevent_add_event_(&bev->ev_read, &bev->timeout_read); if (wpending) r2 = bufferevent_add_event_(&bev->ev_write, &bev->timeout_write); - if (fd >= 0) { - bev_ssl->fd_is_set = 1; - } + return (r1<0 || r2<0) ? -1 : 0; } } @@ -1011,9 +1009,10 @@ do_handshake(struct bufferevent_openssl *bev_ssl) decrement_buckets(bev_ssl); if (r==1) { + int fd = event_get_fd(&bev_ssl->bev.bev.ev_read); /* We're done! */ bev_ssl->state = BUFFEREVENT_SSL_OPEN; - set_open_callbacks(bev_ssl, -1); /* XXXX handle failure */ + set_open_callbacks(bev_ssl, fd); /* XXXX handle failure */ /* Call do_read and do_write as needed */ bufferevent_enable(&bev_ssl->bev.bev, bev_ssl->bev.bev.enabled); bufferevent_run_eventcb_(&bev_ssl->bev.bev, @@ -1074,12 +1073,12 @@ set_handshake_callbacks(struct bufferevent_openssl *bev_ssl, evutil_socket_t fd) } else { struct bufferevent *bev = &bev_ssl->bev.bev; int r1=0, r2=0; - if (fd < 0 && bev_ssl->fd_is_set) - fd = event_get_fd(&bev->ev_read); - if (bev_ssl->fd_is_set) { + + if (event_initialized(&bev->ev_read)) { event_del(&bev->ev_read); event_del(&bev->ev_write); } + event_assign(&bev->ev_read, bev->ev_base, fd, EV_READ|EV_PERSIST|EV_FINALIZE, be_openssl_handshakeeventcb, bev_ssl); @@ -1089,12 +1088,23 @@ set_handshake_callbacks(struct bufferevent_openssl *bev_ssl, evutil_socket_t fd) if (fd >= 0) { r1 = bufferevent_add_event_(&bev->ev_read, &bev->timeout_read); r2 = bufferevent_add_event_(&bev->ev_write, &bev->timeout_write); - bev_ssl->fd_is_set = 1; } return (r1<0 || r2<0) ? -1 : 0; } } +static int +set_handshake_callbacks_auto(struct bufferevent_openssl *bev_ssl, evutil_socket_t fd) +{ + if (!bev_ssl->underlying) { + struct bufferevent *bev = &bev_ssl->bev.bev; + if (event_initialized(&bev->ev_read) && fd < 0) { + fd = event_get_fd(&bev->ev_read); + } + } + return set_handshake_callbacks(bev_ssl, fd); +} + int bufferevent_ssl_renegotiate(struct bufferevent *bev) { @@ -1104,7 +1114,7 @@ bufferevent_ssl_renegotiate(struct bufferevent *bev) if (SSL_renegotiate(bev_ssl->ssl) < 0) return -1; bev_ssl->state = BUFFEREVENT_SSL_CONNECTING; - if (set_handshake_callbacks(bev_ssl, -1) < 0) + if (set_handshake_callbacks_auto(bev_ssl, -1) < 0) return -1; if (!bev_ssl->underlying) return do_handshake(bev_ssl); @@ -1162,8 +1172,6 @@ static int be_openssl_disable(struct bufferevent *bev, short events) { struct bufferevent_openssl *bev_ssl = upcast(bev); - if (bev_ssl->state != BUFFEREVENT_SSL_OPEN) - return 0; if (events & EV_READ) stop_reading(bev_ssl); @@ -1274,25 +1282,16 @@ be_openssl_ctrl(struct bufferevent *bev, BIO *bio; bio = BIO_new_socket(data->fd, 0); SSL_set_bio(bev_ssl->ssl, bio, bio); - bev_ssl->fd_is_set = 1; } - if (data->fd == -1) - bev_ssl->fd_is_set = 0; if (bev_ssl->state == BUFFEREVENT_SSL_OPEN) return set_open_callbacks(bev_ssl, data->fd); else { return set_handshake_callbacks(bev_ssl, data->fd); } case BEV_CTRL_GET_FD: - if (bev_ssl->underlying) - return -1; - if (!bev_ssl->fd_is_set) - return -1; data->fd = event_get_fd(&bev->ev_read); return 0; case BEV_CTRL_GET_UNDERLYING: - if (!bev_ssl->underlying) - return -1; data->ptr = bev_ssl->underlying; return 0; case BEV_CTRL_CANCEL_ALL: @@ -1360,12 +1359,12 @@ bufferevent_openssl_new_impl(struct event_base *base, switch (state) { case BUFFEREVENT_SSL_ACCEPTING: SSL_set_accept_state(bev_ssl->ssl); - if (set_handshake_callbacks(bev_ssl, fd) < 0) + if (set_handshake_callbacks_auto(bev_ssl, fd) < 0) goto err; break; case BUFFEREVENT_SSL_CONNECTING: SSL_set_connect_state(bev_ssl->ssl); - if (set_handshake_callbacks(bev_ssl, fd) < 0) + if (set_handshake_callbacks_auto(bev_ssl, fd) < 0) goto err; break; case BUFFEREVENT_SSL_OPEN: @@ -1383,14 +1382,14 @@ bufferevent_openssl_new_impl(struct event_base *base, bufferevent_suspend_read_(underlying, BEV_SUSPEND_FILT_READ); } else { - bev_ssl->bev.bev.enabled = EV_READ|EV_WRITE; - if (bev_ssl->fd_is_set) { - if (state != BUFFEREVENT_SSL_OPEN) - if (event_add(&bev_ssl->bev.bev.ev_read, NULL) < 0) - goto err; - if (event_add(&bev_ssl->bev.bev.ev_write, NULL) < 0) + struct bufferevent *bev = &bev_ssl->bev.bev; + bev->enabled = EV_READ|EV_WRITE; + if (state != BUFFEREVENT_SSL_OPEN) + if (event_add(&bev->ev_read, NULL) < 0) + goto err; + if (event_initialized(&bev->ev_write)) + if (event_add(&bev->ev_write, NULL) < 0) goto err; - } } return &bev_ssl->bev.bev; From 510da71fae9adda147e90eb0c24612d72d922b0f Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Wed, 2 Sep 2015 12:40:26 +0300 Subject: [PATCH 2/5] be_openssl: introduce set_open_callbacks_auto() This will split cases when we need to extract fd (cases when we have fd==-1 passed to set_open_callbacks()), and cases when we mustn't have to do this -- SET_FD via be_openssl_ctrl(). --- bufferevent_openssl.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/bufferevent_openssl.c b/bufferevent_openssl.c index 2fdd193b..7d7c426a 100644 --- a/bufferevent_openssl.c +++ b/bufferevent_openssl.c @@ -966,19 +966,20 @@ set_open_callbacks(struct bufferevent_openssl *bev_ssl, evutil_socket_t fd) } else { struct bufferevent *bev = &bev_ssl->bev.bev; int rpending=0, wpending=0, r1=0, r2=0; - int new_fd = fd < 0 ? event_get_fd(&bev->ev_read) : fd; - if (fd >= 0 && event_initialized(&bev->ev_read)) { - rpending = event_pending(&bev->ev_read, EV_READ, NULL); - wpending = event_pending(&bev->ev_write, EV_WRITE, NULL); + if (event_initialized(&bev->ev_read)) { + if (fd >= 0) { + rpending = event_pending(&bev->ev_read, EV_READ, NULL); + wpending = event_pending(&bev->ev_write, EV_WRITE, NULL); + } event_del(&bev->ev_read); event_del(&bev->ev_write); } - event_assign(&bev->ev_read, bev->ev_base, new_fd, + event_assign(&bev->ev_read, bev->ev_base, fd, EV_READ|EV_PERSIST|EV_FINALIZE, be_openssl_readeventcb, bev_ssl); - event_assign(&bev->ev_write, bev->ev_base, new_fd, + event_assign(&bev->ev_write, bev->ev_base, fd, EV_WRITE|EV_PERSIST|EV_FINALIZE, be_openssl_writeeventcb, bev_ssl); @@ -990,6 +991,17 @@ set_open_callbacks(struct bufferevent_openssl *bev_ssl, evutil_socket_t fd) return (r1<0 || r2<0) ? -1 : 0; } } +static int +set_open_callbacks_auto(struct bufferevent_openssl *bev_ssl, evutil_socket_t fd) +{ + if (!bev_ssl->underlying) { + struct bufferevent *bev = &bev_ssl->bev.bev; + if (event_initialized(&bev->ev_read) && fd < 0) { + fd = event_get_fd(&bev->ev_read); + } + } + return set_open_callbacks(bev_ssl, fd); +} static int do_handshake(struct bufferevent_openssl *bev_ssl) @@ -1012,7 +1024,7 @@ do_handshake(struct bufferevent_openssl *bev_ssl) int fd = event_get_fd(&bev_ssl->bev.bev.ev_read); /* We're done! */ bev_ssl->state = BUFFEREVENT_SSL_OPEN; - set_open_callbacks(bev_ssl, fd); /* XXXX handle failure */ + set_open_callbacks_auto(bev_ssl, fd); /* XXXX handle failure */ /* Call do_read and do_write as needed */ bufferevent_enable(&bev_ssl->bev.bev, bev_ssl->bev.bev.enabled); bufferevent_run_eventcb_(&bev_ssl->bev.bev, @@ -1368,7 +1380,7 @@ bufferevent_openssl_new_impl(struct event_base *base, goto err; break; case BUFFEREVENT_SSL_OPEN: - if (set_open_callbacks(bev_ssl, fd) < 0) + if (set_open_callbacks_auto(bev_ssl, fd) < 0) goto err; break; default: From 2a8a7112a5918387c015658af347b6b8f692c6b2 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Wed, 2 Sep 2015 12:42:29 +0300 Subject: [PATCH 3/5] be_openssl: introduce be_openssl_auto_fd() helper --- bufferevent_openssl.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/bufferevent_openssl.c b/bufferevent_openssl.c index 7d7c426a..3eb5a0fe 100644 --- a/bufferevent_openssl.c +++ b/bufferevent_openssl.c @@ -955,6 +955,18 @@ be_openssl_writeeventcb(evutil_socket_t fd, short what, void *ptr) bufferevent_decref_and_unlock_(&bev_ssl->bev.bev); } +static int +be_openssl_auto_fd(struct bufferevent_openssl *bev_ssl, int fd) +{ + if (!bev_ssl->underlying) { + struct bufferevent *bev = &bev_ssl->bev.bev; + if (event_initialized(&bev->ev_read) && fd < 0) { + fd = event_get_fd(&bev->ev_read); + } + } + return fd; +} + static int set_open_callbacks(struct bufferevent_openssl *bev_ssl, evutil_socket_t fd) { @@ -994,12 +1006,7 @@ set_open_callbacks(struct bufferevent_openssl *bev_ssl, evutil_socket_t fd) static int set_open_callbacks_auto(struct bufferevent_openssl *bev_ssl, evutil_socket_t fd) { - if (!bev_ssl->underlying) { - struct bufferevent *bev = &bev_ssl->bev.bev; - if (event_initialized(&bev->ev_read) && fd < 0) { - fd = event_get_fd(&bev->ev_read); - } - } + fd = be_openssl_auto_fd(bev_ssl, fd); return set_open_callbacks(bev_ssl, fd); } @@ -1108,12 +1115,7 @@ set_handshake_callbacks(struct bufferevent_openssl *bev_ssl, evutil_socket_t fd) static int set_handshake_callbacks_auto(struct bufferevent_openssl *bev_ssl, evutil_socket_t fd) { - if (!bev_ssl->underlying) { - struct bufferevent *bev = &bev_ssl->bev.bev; - if (event_initialized(&bev->ev_read) && fd < 0) { - fd = event_get_fd(&bev->ev_read); - } - } + fd = be_openssl_auto_fd(bev_ssl, fd); return set_handshake_callbacks(bev_ssl, fd); } From e8a2da96e3fedc3ebf7d322df0fac155dfb41717 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Wed, 2 Sep 2015 14:19:16 +0300 Subject: [PATCH 4/5] be_openssl: don't call set_open_callbacks() if fd == -1 This must be illegal, firstly we must do set_do handshake and only after this we could read/write. --- bufferevent_openssl.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/bufferevent_openssl.c b/bufferevent_openssl.c index 3eb5a0fe..fbd2971f 100644 --- a/bufferevent_openssl.c +++ b/bufferevent_openssl.c @@ -980,10 +980,9 @@ set_open_callbacks(struct bufferevent_openssl *bev_ssl, evutil_socket_t fd) int rpending=0, wpending=0, r1=0, r2=0; if (event_initialized(&bev->ev_read)) { - if (fd >= 0) { - rpending = event_pending(&bev->ev_read, EV_READ, NULL); - wpending = event_pending(&bev->ev_write, EV_WRITE, NULL); - } + rpending = event_pending(&bev->ev_read, EV_READ, NULL); + wpending = event_pending(&bev->ev_write, EV_WRITE, NULL); + event_del(&bev->ev_read); event_del(&bev->ev_write); } @@ -1297,7 +1296,7 @@ be_openssl_ctrl(struct bufferevent *bev, bio = BIO_new_socket(data->fd, 0); SSL_set_bio(bev_ssl->ssl, bio, bio); } - if (bev_ssl->state == BUFFEREVENT_SSL_OPEN) + if (bev_ssl->state == BUFFEREVENT_SSL_OPEN && data->fd >= 0) return set_open_callbacks(bev_ssl, data->fd); else { return set_handshake_callbacks(bev_ssl, data->fd); From 877280db09e8e136e4aecee76149fe126069239c Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Wed, 2 Sep 2015 15:09:24 +0300 Subject: [PATCH 5/5] be_openssl: don't use *_auto() in do_handshake() we can't have fd == -1 there --- bufferevent_openssl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bufferevent_openssl.c b/bufferevent_openssl.c index fbd2971f..4afdde27 100644 --- a/bufferevent_openssl.c +++ b/bufferevent_openssl.c @@ -1030,7 +1030,7 @@ do_handshake(struct bufferevent_openssl *bev_ssl) int fd = event_get_fd(&bev_ssl->bev.bev.ev_read); /* We're done! */ bev_ssl->state = BUFFEREVENT_SSL_OPEN; - set_open_callbacks_auto(bev_ssl, fd); /* XXXX handle failure */ + set_open_callbacks(bev_ssl, fd); /* XXXX handle failure */ /* Call do_read and do_write as needed */ bufferevent_enable(&bev_ssl->bev.bev, bev_ssl->bev.bev.enabled); bufferevent_run_eventcb_(&bev_ssl->bev.bev,