From 5c8b446eba46cc5ee383f24cadb25898686575b3 Mon Sep 17 00:00:00 2001 From: Niels Provos Date: Wed, 3 Feb 2010 14:34:56 -0800 Subject: [PATCH] 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. --- evhttp.h | 6 ++- http.c | 55 ++++++++++++++++++--- test/regress_http.c | 116 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 168 insertions(+), 9 deletions(-) diff --git a/evhttp.h b/evhttp.h index 99d16a2f..7ddf7201 100644 --- a/evhttp.h +++ b/evhttp.h @@ -221,7 +221,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 */ @@ -252,6 +253,9 @@ void evhttp_request_set_chunked_cb(struct evhttp_request *, /** Frees the request object and removes associated events. */ void evhttp_request_free(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); + /** * A connection object that can be used to for making HTTP requests. The * connection object tries to establish the connection when it is given an diff --git a/http.c b/http.c index c7fc51ef..3f5f8209 100644 --- a/http.c +++ b/http.c @@ -606,8 +606,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: default: /* xxx: probably should just error on default */ @@ -654,11 +664,13 @@ evhttp_connection_fail(struct evhttp_connection *evcon, cb = req->cb; cb_arg = req->cb_arg; + /* 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); - /* xxx: maybe we should fail all requests??? */ - /* reset the connection */ evhttp_connection_reset(evcon); @@ -991,7 +1003,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); @@ -1921,6 +1937,9 @@ evhttp_send(struct evhttp_request *req, struct evbuffer *databuf) 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); @@ -1958,15 +1977,20 @@ evhttp_send_reply_start(struct evhttp_request *req, int code, void evhttp_send_reply_chunk(struct evhttp_request *req, struct evbuffer *databuf) { + struct evhttp_connection *evcon = req->evcon; + + if (evcon == NULL) + return; + if (req->chunked) { - evbuffer_add_printf(req->evcon->output_buffer, "%x\r\n", + evbuffer_add_printf(evcon->output_buffer, "%x\r\n", (unsigned)EVBUFFER_LENGTH(databuf)); } - evbuffer_add_buffer(req->evcon->output_buffer, databuf); + evbuffer_add_buffer(evcon->output_buffer, databuf); if (req->chunked) { - evbuffer_add(req->evcon->output_buffer, "\r\n", 2); + evbuffer_add(evcon->output_buffer, "\r\n", 2); } - evhttp_write_buffer(req->evcon, NULL, NULL); + evhttp_write_buffer(evcon, NULL, NULL); } void @@ -1974,6 +1998,14 @@ evhttp_send_reply_end(struct evhttp_request *req) { struct evhttp_connection *evcon = req->evcon; + 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(req->evcon->output_buffer, "0\r\n\r\n", 5); evhttp_write_buffer(req->evcon, evhttp_send_done, NULL); @@ -2513,6 +2545,13 @@ evhttp_request_free(struct evhttp_request *req) free(req); } +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/test/regress_http.c b/test/regress_http.c index 276f12e0..943b29d4 100644 --- a/test/regress_http.c +++ b/test/regress_http.c @@ -1604,6 +1604,121 @@ http_negative_content_length_test(void) fprintf(stdout, "OK\n"); } +/* + * 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(int 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); + return; + } + + 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; + + 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(bev->output, -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; + fprintf(stdout, "Testing Terminated Chunked Connection: "); + + http = http_setup(&port, NULL); + evhttp_del_cb(http, "/test"); + evhttp_set_cb(http, "/test", terminate_chunked_cb, &terminate_state); + + 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(); + + if (test_ok != 1) { + fprintf(stdout, "FAILED\n"); + exit(1); + } + + fprintf(stdout, "OK\n"); + + if (fd >= 0) + EVUTIL_CLOSESOCKET(fd); + if (http) + evhttp_free(http); +} + void http_suite(void) { @@ -1625,4 +1740,5 @@ http_suite(void) http_negative_content_length_test(); http_chunked_test(); + http_terminate_chunked_test(); }