Correctly invoke callbacks when a SSL bufferevent reads some and then blocks.

Based on a patch by Andrew Hochhaus, who correctly diagnosed this bug.

diff --git a/bufferevent_openssl.c b/bufferevent_openssl.c
index bdc363e5de43cd4ac2cc6e6bdeb8108cd8ce9374..fdbec28015aafab252c397766925e89ca064962d 100644 (file)
--- a/bufferevent_openssl.c
+++ b/bufferevent_openssl.c
@@ -556,15 +556,20 @@ decrement_buckets(struct bufferevent_openssl *bev_ssl)
        bev_ssl->counts.n_read = num_r;
 }
 
-/* returns -1 on internal error, 0 on stall, 1 on progress */
-static int
-do_read(struct bufferevent_openssl *bev_ssl, int n_to_read)
-{
+#define OP_MADE_PROGRESS 1
+#define OP_BLOCKED 2
+#define OP_ERR 4
+
+/* Return a bitmask of OP_MADE_PROGRESS (if we read anything); OP_BLOCKED (if
+   we're now blocked); and OP_ERR (if an error occurred). */
+ static int
+do_read(struct bufferevent_openssl *bev_ssl, int n_to_read) {
        /* Requires lock */
        struct bufferevent *bev = &bev_ssl->bev.bev;
        struct evbuffer *input = bev->input;
-       int r, n, i, n_used = 0, blocked = 0, atmost;
+       int r, n, i, n_used = 0, atmost;
        struct evbuffer_iovec space[2];
+       int result = 0;
 
        atmost = _bufferevent_get_read_max(&bev_ssl->bev);
        if (n_to_read > atmost)
@@ -572,16 +577,17 @@ do_read(struct bufferevent_openssl *bev_ssl, int n_to_read)
 
        n = evbuffer_reserve_space(input, n_to_read, space, 2);
        if (n < 0)
-               return -1;
+               return OP_ERR;
 
        for (i=0; i<n; ++i) {
                if (bev_ssl->bev.read_suspended)
                        break;
                r = SSL_read(bev_ssl->ssl, space[i].iov_base, space[i].iov_len);
                if (r>0) {
+                       result |= OP_MADE_PROGRESS;
                        if (bev_ssl->read_blocked_on_write)
                                if (clear_rbow(bev_ssl) < 0)
-                                       return -1;
+                                       return OP_ERR | result;
                        ++n_used;
                        space[i].iov_len = r;
                        decrement_buckets(bev_ssl);
@@ -593,20 +599,20 @@ do_read(struct bufferevent_openssl *bev_ssl, int n_to_read)
                                /* Can't read until underlying has more data. */
                                if (bev_ssl->read_blocked_on_write)
                                        if (clear_rbow(bev_ssl) < 0)
-                                               return -1;
+                                               return OP_ERR | result;
                                break;
                        case SSL_ERROR_WANT_WRITE:
                                /* This read operation requires a write, and the
                                 * underlying is full */
                                if (!bev_ssl->read_blocked_on_write)
                                        if (set_rbow(bev_ssl) < 0)
-                                               return -1;
+                                               return OP_ERR | result;
                                break;
                        default:
                                conn_closed(bev_ssl, err, r);
                                break;
                        }
-                       blocked = 1;
+                       result |= OP_BLOCKED;
                        break; /* out of the loop */
                }
        }
@@ -617,16 +623,19 @@ do_read(struct bufferevent_openssl *bev_ssl, int n_to_read)
                        BEV_RESET_GENERIC_READ_TIMEOUT(bev);
        }
 
-       return blocked ? 0 : 1;
+       return result;
 }
 
+/* Return a bitmask of OP_MADE_PROGRESS (if we wrote anything); OP_BLOCKED (if
+   we're now blocked); and OP_ERR (if an error occurred). */
 static int
 do_write(struct bufferevent_openssl *bev_ssl, int atmost)
 {
-       int i, r, n, n_written = 0, blocked=0;
+       int i, r, n, n_written = 0;
        struct bufferevent *bev = &bev_ssl->bev.bev;
        struct evbuffer *output = bev->output;
        struct evbuffer_iovec space[8];
+       int result = 0;
 
        if (bev_ssl->last_write > 0)
                atmost = bev_ssl->last_write;
@@ -635,7 +644,7 @@ do_write(struct bufferevent_openssl *bev_ssl, int atmost)
 
        n = evbuffer_peek(output, atmost, NULL, space, 8);
        if (n < 0)
-               return -1;
+               return OP_ERR | result;
 
        if (n > 8)
                n = 8;
@@ -652,9 +661,10 @@ do_write(struct bufferevent_openssl *bev_ssl, int atmost)
                r = SSL_write(bev_ssl->ssl, space[i].iov_base,
                    space[i].iov_len);
                if (r > 0) {
+                       result |= OP_MADE_PROGRESS;
                        if (bev_ssl->write_blocked_on_read)
                                if (clear_wbor(bev_ssl) < 0)
-                                       return -1;
+                                       return OP_ERR | result;
                        n_written += r;
                        bev_ssl->last_write = -1;
                        decrement_buckets(bev_ssl);
@@ -666,7 +676,7 @@ do_write(struct bufferevent_openssl *bev_ssl, int atmost)
                                /* Can't read until underlying has more data. */
                                if (bev_ssl->write_blocked_on_read)
                                        if (clear_wbor(bev_ssl) < 0)
-                                               return -1;
+                                               return OP_ERR | result;
                                bev_ssl->last_write = space[i].iov_len;
                                break;
                        case SSL_ERROR_WANT_READ:
@@ -674,7 +684,7 @@ do_write(struct bufferevent_openssl *bev_ssl, int atmost)
                                 * underlying is full */
                                if (!bev_ssl->write_blocked_on_read)
                                        if (set_wbor(bev_ssl) < 0)
-                                               return -1;
+                                               return OP_ERR | result;
                                bev_ssl->last_write = space[i].iov_len;
                                break;
                        default:
@@ -682,7 +692,7 @@ do_write(struct bufferevent_openssl *bev_ssl, int atmost)
                                bev_ssl->last_write = -1;
                                break;
                        }
-                       blocked = 1;
+                       result |= OP_BLOCKED;
                        break;
                }
        }
@@ -694,7 +704,7 @@ do_write(struct bufferevent_openssl *bev_ssl, int atmost)
                if (evbuffer_get_length(output) <= bev->wm_write.low)
                        _bufferevent_run_writecb(bev);
        }
-       return blocked ? 0 : 1;
+       return result;
 }
 
 #define WRITE_FRAME 15000
@@ -754,11 +764,11 @@ consider_reading(struct bufferevent_openssl *bev_ssl)
 {
        int r;
        int n_to_read;
-       int read_data = 0;
+       int all_result_flags = 0;
 
        while (bev_ssl->write_blocked_on_read) {
                r = do_write(bev_ssl, WRITE_FRAME);
-               if (r <= 0)
+               if (r & (OP_BLOCKED|OP_ERR))
                        break;
        }
        if (bev_ssl->write_blocked_on_read)
@@ -767,10 +777,11 @@ consider_reading(struct bufferevent_openssl *bev_ssl)
        n_to_read = bytes_to_read(bev_ssl);
 
        while (n_to_read) {
-               if (do_read(bev_ssl, n_to_read) <= 0)
-                       break;
+               r = do_read(bev_ssl, n_to_read);
+               all_result_flags |= r;
 
-               read_data = 1;
+               if (r & (OP_BLOCKED|OP_ERR))
+                       break;
 
                /* Read all pending data.  This won't hit the network
                 * again, and will (most importantly) put us in a state
@@ -800,7 +811,7 @@ consider_reading(struct bufferevent_openssl *bev_ssl)
                        n_to_read = bytes_to_read(bev_ssl);
        }
 
-       if (read_data == 1) {
+       if (all_result_flags & OP_MADE_PROGRESS) {
                struct bufferevent *bev = &bev_ssl->bev.bev;
                struct evbuffer *input = bev->input;
 
@@ -828,9 +839,7 @@ consider_writing(struct bufferevent_openssl *bev_ssl)
 
        while (bev_ssl->read_blocked_on_write) {
                r = do_read(bev_ssl, 1024); /* XXXX 1024 is a hack */
-               if (r <= 0)
-                       break;
-               else {
+               if (r & OP_MADE_PROGRESS) {
                        struct bufferevent *bev = &bev_ssl->bev.bev;
                        struct evbuffer *input = bev->input;
 
@@ -838,6 +847,8 @@ consider_writing(struct bufferevent_openssl *bev_ssl)
                                _bufferevent_run_readcb(bev);
                        }
                }
+               if (r & (OP_ERR|OP_BLOCKED))
+                       break;
        }
        if (bev_ssl->read_blocked_on_write)
                return;
@@ -855,7 +866,7 @@ consider_writing(struct bufferevent_openssl *bev_ssl)
                else
                        n_to_write = WRITE_FRAME;
                r = do_write(bev_ssl, n_to_write);
-               if (r <= 0)
+               if (r & (OP_BLOCKED|OP_ERR))
                        break;
        }