From: Azat Khuzhin Date: Sun, 22 Oct 2017 21:13:37 +0000 (+0300) Subject: Fix crashing http server when callback do not reply in place X-Git-Tag: release-2.1.9-beta^2~154 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=5b40744d1581447f5b4496ee8d4807383e468e7a;p=libevent Fix crashing http server when callback do not reply in place General http callback looks like: static void http_cb(struct evhttp_request *req, void *arg) { evhttp_send_reply(req, HTTP_OK, "Everything is fine", NULL); } And they will work fine becuase in this case http will write request first, and during write preparation it will disable *read callback* (in evhttp_write_buffer()), but if we don't reply immediately, for example: static void http_cb(struct evhttp_request *req, void *arg) { return; } This will leave connection in incorrect state, and if another request will be written to the same connection libevent will abort with: [err] ../http.c: illegal connection state 7 Because it thinks that read for now is not possible, since there were no write. Fix this by disabling EV_READ entirely. We couldn't just reset callbacks because this will leave EOF detection, which we don't need, since user hasn't replied to callback yet. Reported-by: Cory Fields (cherry picked from commit 5ff8eb26371c4dc56f384b2de35bea2d87814779) --- diff --git a/http.c b/http.c index 3983b3dd..8aa2b120 100644 --- a/http.c +++ b/http.c @@ -368,15 +368,15 @@ evhttp_write_buffer(struct evhttp_connection *evcon, evcon->cb_arg = arg; /* Disable the read callback: we don't actually care about data; - * we only care about close detection. (We don't disable reading, - * since we *do* want to learn about any close events.) */ + * we only care about close detection. (We don't disable reading -- + * EV_READ, since we *do* want to learn about any close events.) */ bufferevent_setcb(evcon->bufev, NULL, /*read*/ evhttp_write_cb, evhttp_error_cb, evcon); - bufferevent_enable(evcon->bufev, EV_WRITE); + bufferevent_enable(evcon->bufev, EV_READ|EV_WRITE); } static void @@ -3437,6 +3437,8 @@ evhttp_handle_request(struct evhttp_request *req, void *arg) } if ((cb = evhttp_dispatch_callback(&http->callbacks, req)) != NULL) { + bufferevent_disable(req->evcon->bufev, EV_READ); + (*cb->cb)(req, cb->cbarg); return; } diff --git a/test/regress_http.c b/test/regress_http.c index 58ccf1f7..86fdf433 100644 --- a/test/regress_http.c +++ b/test/regress_http.c @@ -72,6 +72,7 @@ static struct event_base *exit_base; static char const BASIC_REQUEST_BODY[] = "This is funny"; static void http_basic_cb(struct evhttp_request *req, void *arg); +static void http_timeout_cb(struct evhttp_request *req, void *arg); static void http_large_cb(struct evhttp_request *req, void *arg); static void http_chunked_cb(struct evhttp_request *req, void *arg); static void http_post_cb(struct evhttp_request *req, void *arg); @@ -146,6 +147,7 @@ http_setup(ev_uint16_t *pport, struct event_base *base, int mask) /* Register a callback for certain types of requests */ evhttp_set_cb(myhttp, "/test", http_basic_cb, myhttp); + evhttp_set_cb(myhttp, "/timeout", http_timeout_cb, myhttp); evhttp_set_cb(myhttp, "/large", http_large_cb, base); evhttp_set_cb(myhttp, "/chunked", http_chunked_cb, base); evhttp_set_cb(myhttp, "/streamed", http_chunked_cb, base); @@ -346,6 +348,20 @@ end: evbuffer_free(evb); } +static void http_timeout_reply_cb(int fd, short events, void *arg) +{ + struct evhttp_request *req = arg; + evhttp_send_reply(req, HTTP_OK, "Everything is fine", NULL); + test_ok++; +} +static void +http_timeout_cb(struct evhttp_request *req, void *arg) +{ + struct timeval when = { 0, 100 }; + event_base_once(exit_base, -1, EV_TIMEOUT, + http_timeout_reply_cb, req, &when); +} + static void http_large_cb(struct evhttp_request *req, void *arg) { @@ -4510,6 +4526,54 @@ http_request_own_test(void *arg) test_ok = 1; } +static void +http_request_extra_body_test(void *arg) +{ + struct basic_test_data *data = arg; + struct bufferevent *bev = NULL; + evutil_socket_t fd; + ev_uint16_t port = 0; + int i; + struct evhttp *http = http_setup(&port, data->base, 0); + struct evbuffer *out, *body = NULL; + + exit_base = data->base; + test_ok = 0; + + fd = http_connect("127.0.0.1", port); + + /* Stupid thing to send a request */ + bev = create_bev(data->base, fd, 0); + bufferevent_setcb(bev, http_readcb, http_writecb, + http_errorcb, data->base); + out = bufferevent_get_output(bev); + body = evbuffer_new(); + + for (i = 0; i < 10000; ++i) + evbuffer_add_printf(body, "this is the body that HEAD should not have"); + + evbuffer_add_printf(out, + "HEAD /timeout HTTP/1.1\r\n" + "Host: somehost\r\n" + "Connection: close\r\n" + "Content-Length: %i\r\n" + "\r\n", + (int)evbuffer_get_length(body) + ); + evbuffer_add_buffer(out, body); + + event_base_dispatch(data->base); + + tt_assert(test_ok == -2); + + end: + evhttp_free(http); + if (bev) + bufferevent_free(bev); + if (body) + evbuffer_free(body); +} + #define HTTP_LEGACY(name) \ { #name, run_legacy_test_fn, TT_ISOLATED|TT_LEGACY, &legacy_setup, \ http_##name##_test } @@ -4629,6 +4693,8 @@ struct testcase_t http_testcases[] = { HTTP(write_during_read), HTTP(request_own), + HTTP(request_extra_body), + #ifdef EVENT__HAVE_OPENSSL HTTPS(basic), HTTPS(filter_basic),