From: Niels Provos Date: Wed, 3 Feb 2010 22:34:56 +0000 (-0800) Subject: do not fail while sending on http connections the client closed. X-Git-Tag: release-2.0.4-alpha~41^2 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=93d736910616ad32e6d8c2f7f377b12f5848f5d5;p=libevent do not fail while sending on http connections the client closed. when sending chunked requests via multiple calls to evhttp_send_reply_chunk, the client may close the connection before the server is done sending. this used to cause a crash. we introduce a new function evhttp_request_get_connection() that allows the server to determine if the request is still associated with a connection. If it's not, evhttp_request_free() needs to be called explicitly or the user can call evhttp_send_reply_end() which just frees the request, too. --- diff --git a/http.c b/http.c index 64281623..a9a14465 100644 --- a/http.c +++ b/http.c @@ -550,8 +550,18 @@ evhttp_connection_incoming_fail(struct evhttp_request *req, * these are cases in which we probably should just * close the connection and not send a reply. this * case may happen when a browser keeps a persistent - * connection open and we timeout on the read. + * connection open and we timeout on the read. when + * the request is still being used for sending, we + * need to disassociated it from the connection here. */ + if (!req->userdone) { + /* remove it so that it will not be freed */ + TAILQ_REMOVE(&req->evcon->requests, req, next); + /* indicate that this request no longer has a + * connection object + */ + req->evcon = NULL; + } return (-1); case EVCON_HTTP_INVALID_HEADER: case EVCON_HTTP_BUFFER_ERROR: @@ -608,13 +618,12 @@ evhttp_connection_fail(struct evhttp_connection *evcon, cb_arg = NULL; } - TAILQ_REMOVE(&evcon->requests, req, next); - evhttp_request_free(req); - /* do not fail all requests; the next request is going to get * send over a new connection. when a user cancels a request, * all other pending requests should be processed as normal */ + TAILQ_REMOVE(&evcon->requests, req, next); + evhttp_request_free(req); /* reset the connection */ evhttp_connection_reset(evcon); @@ -942,7 +951,11 @@ evhttp_connection_free(struct evhttp_connection *evcon) (*evcon->closecb)(evcon, evcon->closecb_arg); } - /* remove all requests that might be queued on this connection */ + /* remove all requests that might be queued on this + * connection. for server connections, this should be empty. + * because it gets dequeued either in evhttp_connection_done or + * evhttp_connection_fail. + */ while ((req = TAILQ_FIRST(&evcon->requests)) != NULL) { TAILQ_REMOVE(&evcon->requests, req, next); evhttp_request_free(req); @@ -2054,6 +2067,9 @@ evhttp_send(struct evhttp_request *req, struct evbuffer *databuf) EVUTIL_ASSERT(TAILQ_FIRST(&evcon->requests) == req); + /* we expect no more calls form the user on this request */ + req->userdone = 1; + /* xxx: not sure if we really should expose the data buffer this way */ if (databuf != NULL) evbuffer_add_buffer(req->output_buffer, databuf); @@ -2097,7 +2113,14 @@ evhttp_send_reply_start(struct evhttp_request *req, int code, void evhttp_send_reply_chunk(struct evhttp_request *req, struct evbuffer *databuf) { - struct evbuffer *output = bufferevent_get_output(req->evcon->bufev); + struct evhttp_connection *evcon = req->evcon; + struct evbuffer *output; + + if (evcon == NULL) + return; + + output = bufferevent_get_output(evcon->bufev); + if (evbuffer_get_length(databuf) == 0) return; if (!evhttp_response_needs_body(req)) @@ -2110,7 +2133,7 @@ evhttp_send_reply_chunk(struct evhttp_request *req, struct evbuffer *databuf) if (req->chunked) { evbuffer_add(output, "\r\n", 2); } - evhttp_write_buffer(req->evcon, NULL, NULL); + evhttp_write_buffer(evcon, NULL, NULL); } void @@ -2119,6 +2142,14 @@ evhttp_send_reply_end(struct evhttp_request *req) struct evhttp_connection *evcon = req->evcon; struct evbuffer *output = bufferevent_get_output(evcon->bufev); + if (evcon == NULL) { + evhttp_request_free(req); + return; + } + + /* we expect no more calls form the user on this request */ + req->userdone = 1; + if (req->chunked) { evbuffer_add(output, "0\r\n\r\n", 5); evhttp_write_buffer(req->evcon, evhttp_send_done, NULL); @@ -2857,6 +2888,13 @@ evhttp_request_is_owned(struct evhttp_request *req) return (req->flags & EVHTTP_USER_OWNED) != 0; } +struct evhttp_connection * +evhttp_request_get_connection(struct evhttp_request *req) +{ + return req->evcon; +} + + void evhttp_request_set_chunked_cb(struct evhttp_request *req, void (*cb)(struct evhttp_request *, void *)) diff --git a/include/event2/http.h b/include/event2/http.h index d5dff821..a56526e8 100644 --- a/include/event2/http.h +++ b/include/event2/http.h @@ -374,6 +374,9 @@ void evhttp_request_own(struct evhttp_request *req); /** Returns 1 if the request is owned by the user */ int evhttp_request_is_owned(struct evhttp_request *req); +/** Returns the connection object associated with the request or NULL */ +struct evhttp_connection *evhttp_request_get_connection(struct evhttp_request *req); + void evhttp_connection_set_max_headers_size(struct evhttp_connection *evcon, ev_ssize_t new_max_headers_size); diff --git a/include/event2/http_struct.h b/include/event2/http_struct.h index 902eab74..c0a0f608 100644 --- a/include/event2/http_struct.h +++ b/include/event2/http_struct.h @@ -101,7 +101,8 @@ struct { struct evbuffer *input_buffer; /* read data */ ev_int64_t ntoread; - int chunked; + int chunked:1, /* a chunked request */ + userdone:1; /* the user has sent all data */ struct evbuffer *output_buffer; /* outgoing post or data */ diff --git a/test/regress_http.c b/test/regress_http.c index c5fc50dd..2b251004 100644 --- a/test/regress_http.c +++ b/test/regress_http.c @@ -2515,6 +2515,117 @@ http_data_length_constraints_test(void) evhttp_free(http); } +/* + * Testing client reset of server chunked connections + */ + +struct terminate_state { + struct evhttp_request *req; + struct bufferevent *bev; + int fd; +} terminate_state; + +static void +terminate_chunked_trickle_cb(evutil_socket_t fd, short events, void *arg) +{ + struct terminate_state *state = arg; + struct evbuffer *evb = evbuffer_new(); + struct timeval tv; + + if (evhttp_request_get_connection(state->req) == NULL) { + test_ok = 1; + evhttp_request_free(state->req); + event_loopexit(NULL); + } + + evbuffer_add_printf(evb, "%p", evb); + evhttp_send_reply_chunk(state->req, evb); + evbuffer_free(evb); + + tv.tv_sec = 0; + tv.tv_usec = 3000; + event_once(-1, EV_TIMEOUT, terminate_chunked_trickle_cb, arg, &tv); +} + +static void +terminate_chunked_cb(struct evhttp_request *req, void *arg) +{ + struct terminate_state *state = arg; + struct timeval tv; + + /* we need to hold on to this */ + evhttp_request_own(req); + + state->req = req; + + evhttp_send_reply_start(req, HTTP_OK, "OK"); + + tv.tv_sec = 0; + tv.tv_usec = 3000; + event_once(-1, EV_TIMEOUT, terminate_chunked_trickle_cb, arg, &tv); +} + +static void +terminate_chunked_client(int fd, short event, void *arg) +{ + struct terminate_state *state = arg; + bufferevent_free(state->bev); + EVUTIL_CLOSESOCKET(state->fd); +} + +static void +terminate_readcb(struct bufferevent *bev, void *arg) +{ + /* just drop the data */ + evbuffer_drain(bufferevent_get_input(bev), -1); +} + + +static void +http_terminate_chunked_test(void) +{ + struct bufferevent *bev = NULL; + struct timeval tv; + const char *http_request; + short port = -1; + int fd = -1; + + test_ok = 0; + + http = http_setup(&port, NULL); + evhttp_del_cb(http, "/test"); + tt_assert(evhttp_set_cb(http, "/test", + terminate_chunked_cb, &terminate_state) == 0); + + fd = http_connect("127.0.0.1", port); + + /* Stupid thing to send a request */ + bev = bufferevent_new(fd, terminate_readcb, http_writecb, + http_errorcb, NULL); + + terminate_state.fd = fd; + terminate_state.bev = bev; + + /* first half of the http request */ + http_request = + "GET /test HTTP/1.1\r\n" + "Host: some\r\n\r\n"; + + bufferevent_write(bev, http_request, strlen(http_request)); + evutil_timerclear(&tv); + tv.tv_usec = 10000; + event_once(-1, EV_TIMEOUT, terminate_chunked_client, &terminate_state, + &tv); + + event_dispatch(); + + end: + if (fd >= 0) + EVUTIL_CLOSESOCKET(fd); + if (http) + evhttp_free(http); +} + #define HTTP_LEGACY(name) \ { #name, run_legacy_test_fn, TT_ISOLATED|TT_LEGACY, &legacy_setup, \ http_##name##_test } @@ -2539,6 +2650,9 @@ struct testcase_t http_testcases[] = { HTTP_LEGACY(bad_request), HTTP_LEGACY(incomplete), HTTP_LEGACY(incomplete_timeout), + { "terminate_chunked", run_legacy_test_fn, + TT_ISOLATED|TT_LEGACY, &legacy_setup, + http_terminate_chunked_test }, HTTP_LEGACY(highport), HTTP_LEGACY(dispatcher),