From Scott Lamb:

* Allow the user to set the Content-Length: then stream a reply.
  This is useful for large requests of a known size. Added unit test.

* Don't send a response body on HEAD requests, 1xx status codes, 204
  status codes, or 304 status codes, as described in RFC 2616 section
  4.3. (Doing otherwise causes problems - in particular, if a 304 has a
  chunked body (even an empty one), Safari 3.1.1 issues and then fails
  the next request on the connection with the non-sequitur error message
  "Too many HTTP redirects"!)

* Specify a default Content-Type: when a response body is required, not
  when we have data in the response buffer by the time we make the
  header. (I.e., do this on evhttp_send_reply_start() for consistency.)

* Don't expect a body in response to HEAD requests.

svn:r898

diff --git a/ChangeLog b/ChangeLog
index f19cbd2201aa8cdef64ce950d249a83e61dd7547..4fa55b3cc772b829943b8e9da63eb9538a1372f9 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -115,6 +115,7 @@ Changes in current version:
  o Support multi-line HTTP headers; based on a patch from Moshe Litvin
  o Reject negative Content-Length headers; anonymous bug report
  o Detect CLOCK_MONOTONIC at runtime for evdns; anonymous bug report   
+ o Various HTTP correctness fixes from Scott Lamb
        
 Changes in 1.4.0:
  o allow \r or \n individually to separate HTTP headers instead of the standard "\r\n"; from Charles Kerr.

diff --git a/http.c b/http.c
index b5eac236586cddf1b3bdcbbe0107d5a5b1d3f737..b283685d712491626a03edfb276a155e54776581 100644 (file)
--- a/http.c
+++ b/http.c
@@ -326,6 +326,21 @@ evhttp_method(enum evhttp_cmd_type type)
        return (method);
 }
 
+/**
+ * Determines if a response should have a body.
+ * Follows the rules in RFC 2616 section 4.3.
+ * @return 1 if the response MUST have a body;
+ *         0 if the response MUST NOT have a body.
+ */
+static int
+evhttp_response_needs_body(struct evhttp_request *req)
+{
+       return (req->response_code != HTTP_NOCONTENT &&
+               req->response_code != HTTP_NOTMODIFIED &&
+               (req->response_code < 100 || req->response_code >= 200) &&
+               req->type != EVHTTP_REQ_HEAD);
+}
+
 static void
 evhttp_add_event(struct event *ev, int timeout, int default_timeout)
 {
@@ -482,7 +497,8 @@ evhttp_make_header_response(struct evhttp_connection *evcon,
                        evhttp_add_header(req->output_headers,
                            "Connection", "keep-alive");
 
-               if (req->minor == 1 || is_keepalive) {
+               if ((req->minor == 1 || is_keepalive) &&
+                   evhttp_response_needs_body(req)) {
                        /* 
                         * we need to add the content length if the
                         * user did not give it, this is required for
@@ -495,7 +511,7 @@ evhttp_make_header_response(struct evhttp_connection *evcon,
        }
 
        /* Potentially add headers for unidentified content. */
-       if (EVBUFFER_LENGTH(req->output_buffer)) {
+       if (evhttp_response_needs_body(req)) {
                if (evhttp_find_header(req->output_headers,
                        "Content-Type") == NULL) {
                        evhttp_add_header(req->output_headers,
@@ -1601,9 +1617,7 @@ evhttp_read_header(struct evhttp_connection *evcon,
                break;
 
        case EVHTTP_RESPONSE:
-               if (req->response_code == HTTP_NOCONTENT ||
-                   req->response_code == HTTP_NOTMODIFIED ||
-                   (req->response_code >= 100 && req->response_code < 200)) {
+               if (!evhttp_response_needs_body(req)) {
                        event_debug(("%s: skipping body for code %d\n",
                                        __func__, req->response_code));
                        evhttp_connection_done(evcon);
@@ -1957,8 +1971,14 @@ evhttp_send_reply_start(struct evhttp_request *req, int code,
        /* set up to watch for client close */
        evhttp_connection_start_detectclose(req->evcon);
        evhttp_response_code(req, code, reason);
-       if (req->major == 1 && req->minor == 1) {
-               /* use chunked encoding for HTTP/1.1 */
+       if (evhttp_find_header(req->output_headers, "Content-Length") == NULL &&
+           req->major == 1 && req->minor == 1 &&
+           evhttp_response_needs_body(req)) {
+               /*
+                * prefer HTTP/1.1 chunked encoding to closing the connection;
+                * note RFC 2616 section 4.4 forbids it with Content-Length:
+                * and it's not necessary then anyway.
+                */
                evhttp_add_header(req->output_headers, "Transfer-Encoding",
                    "chunked");
                req->chunked = 1;
@@ -1971,6 +1991,10 @@ void
 evhttp_send_reply_chunk(struct evhttp_request *req, struct evbuffer *databuf)
 {
        struct evbuffer *output = bufferevent_get_output(req->evcon->bufev);
+       if (EVBUFFER_LENGTH(databuf) == 0)
+               return;
+       if (!evhttp_response_needs_body(req))
+               return;
        if (req->chunked) {
                evbuffer_add_printf(output, "%x\r\n",
                                    (unsigned)EVBUFFER_LENGTH(databuf));

diff --git a/test/regress_http.c b/test/regress_http.c
index bd4469bfb4a2cd2e8327f4fc3cc82386836afd25..207814ad15c0af4052293f387c7f7cc296978130 100644 (file)
--- a/test/regress_http.c
+++ b/test/regress_http.c
@@ -64,6 +64,8 @@ static struct evhttp *http;
 /* set if a test needs to call loopexit on a base */
 static struct event_base *base;
 
+static char const BASIC_REQUEST_BODY[] = "This is funny";
+
 void http_suite(void);
 
 static void http_basic_cb(struct evhttp_request *req, void *arg);
@@ -96,6 +98,7 @@ http_setup(short *pport, struct event_base *base)
        /* Register a callback for certain types of requests */
        evhttp_set_cb(myhttp, "/test", http_basic_cb, NULL);
        evhttp_set_cb(myhttp, "/chunked", http_chunked_cb, NULL);
+       evhttp_set_cb(myhttp, "/streamed", http_chunked_cb, NULL);
        evhttp_set_cb(myhttp, "/postit", http_post_cb, NULL);
        evhttp_set_cb(myhttp, "/putit", http_put_cb, NULL);
        evhttp_set_cb(myhttp, "/deleteit", http_delete_cb, NULL);
@@ -174,7 +177,7 @@ http_connect(const char *address, u_short port)
 static void
 http_readcb(struct bufferevent *bev, void *arg)
 {
-       const char *what = "This is funny";
+       const char *what = BASIC_REQUEST_BODY;
 
        event_debug(("%s: %s\n", __func__, EVBUFFER_DATA(EVBUFFER_INPUT(bev))));
        
@@ -230,7 +233,7 @@ http_basic_cb(struct evhttp_request *req, void *arg)
        struct evbuffer *evb = evbuffer_new();
        int empty = evhttp_find_header(req->input_headers, "Empty") != NULL;
        event_debug(("%s: called\n", __func__));
-       evbuffer_add_printf(evb, "This is funny");
+       evbuffer_add_printf(evb, BASIC_REQUEST_BODY);
        
        /* For multi-line headers test */
        {
@@ -297,7 +300,11 @@ http_chunked_cb(struct evhttp_request *req, void *arg)
        memset(state, 0, sizeof(struct chunk_req_state));
        state->req = req;
 
-       /* generate a chunked reply */
+       if (strcmp(evhttp_request_uri(req), "/streamed") == 0) {
+               evhttp_add_header(req->output_headers, "Content-Length", "39");
+       }
+ 
+       /* generate a chunked/streamed reply */
        evhttp_send_reply_start(req, HTTP_OK, "Everything is fine");
 
        /* but trickle it across several iterations to ensure we're not
@@ -416,7 +423,7 @@ http_delete_cb(struct evhttp_request *req, void *arg)
        }
 
        event_debug(("%s: called\n", __func__));
-       evbuffer_add_printf(evb, "This is funny");
+       evbuffer_add_printf(evb, BASIC_REQUEST_BODY);
 
        /* allow sending of an empty reply */
        evhttp_send_reply(req, HTTP_OK, "Everything is fine",
@@ -494,7 +501,7 @@ http_connection_test(int persistent)
         * server using our make request method.
         */
 
-       req = evhttp_request_new(http_request_done, NULL);
+       req = evhttp_request_new(http_request_done, (void*) BASIC_REQUEST_BODY);
 
        /* Add the information that we care about */
        evhttp_add_header(req->output_headers, "Host", "somehost");
@@ -515,7 +522,7 @@ http_connection_test(int persistent)
        /* try to make another request over the same connection */
        test_ok = 0;
        
-       req = evhttp_request_new(http_request_done, NULL);
+       req = evhttp_request_new(http_request_done, (void*) BASIC_REQUEST_BODY);
 
        /* Add the information that we care about */
        evhttp_add_header(req->output_headers, "Host", "somehost");
@@ -636,7 +643,7 @@ http_cancel_test(void)
        /* try to make another request over the same connection */
        test_ok = 0;
        
-       req = evhttp_request_new(http_request_done, NULL);
+       req = evhttp_request_new(http_request_done, (void*) BASIC_REQUEST_BODY);
 
        /* Add the information that we care about */
        evhttp_add_header(req->output_headers, "Host", "somehost");
@@ -679,7 +686,7 @@ http_cancel_test(void)
 static void
 http_request_done(struct evhttp_request *req, void *arg)
 {
-       const char *what = "This is funny";
+       const char *what = arg;
 
        if (req->response_code != HTTP_OK) {
                fprintf(stderr, "FAILED\n");
@@ -776,7 +783,7 @@ http_virtual_host_test(void)
        test_ok = 0;
 
        /* make a request with the right host and expect a response */
-       req = evhttp_request_new(http_request_done, NULL);
+       req = evhttp_request_new(http_request_done, (void*) BASIC_REQUEST_BODY);
 
        /* Add the information that we care about */
        evhttp_add_header(req->output_headers, "Host", "foo.com");
@@ -798,7 +805,7 @@ http_virtual_host_test(void)
        test_ok = 0;
 
        /* make a request with the right host and expect a response */
-       req = evhttp_request_new(http_request_done, NULL);
+       req = evhttp_request_new(http_request_done, (void*) BASIC_REQUEST_BODY);
 
        /* Add the information that we care about */
        evhttp_add_header(req->output_headers, "Host", "bar.magic.foo.com");
@@ -1046,7 +1053,7 @@ http_post_cb(struct evhttp_request *req, void *arg)
        }
        
        evb = evbuffer_new();
-       evbuffer_add_printf(evb, "This is funny");
+       evbuffer_add_printf(evb, BASIC_REQUEST_BODY);
 
        evhttp_send_reply(req, HTTP_OK, "Everything is fine", evb);
 
@@ -1056,7 +1063,7 @@ http_post_cb(struct evhttp_request *req, void *arg)
 void
 http_postrequest_done(struct evhttp_request *req, void *arg)
 {
-       const char *what = "This is funny";
+       const char *what = BASIC_REQUEST_BODY;
 
        if (req == NULL) {
                fprintf(stderr, "FAILED (timeout)\n");
@@ -1719,7 +1726,7 @@ http_chunked_request_done(struct evhttp_request *req, void *arg)
 }
 
 static void
-http_chunked_test(void)
+http_chunk_out_test(void)
 {
        struct bufferevent *bev;
        int fd;
@@ -1804,6 +1811,55 @@ http_chunked_test(void)
        fprintf(stdout, "OK\n");
 }
 
+static void
+http_stream_out_test(void)
+{
+       short port = -1;
+       struct evhttp_connection *evcon = NULL;
+       struct evhttp_request *req = NULL;
+       
+       test_ok = 0;
+       printf("Tests streaming responses out: ");
+
+       http = http_setup(&port, NULL);
+
+       evcon = evhttp_connection_new("127.0.0.1", port);
+       if (evcon == NULL) {
+               fprintf(stdout, "FAILED\n");
+               exit(1);
+       }
+
+       /*
+        * At this point, we want to schedule a request to the HTTP
+        * server using our make request method.
+        */
+
+       req = evhttp_request_new(http_request_done,
+           (void *)"This is funnybut not hilarious.bwv 1052");
+
+       /* Add the information that we care about */
+       evhttp_add_header(req->output_headers, "Host", "somehost");
+
+       /* We give ownership of the request to the connection */
+       if (evhttp_make_request(evcon, req, EVHTTP_REQ_GET, "/streamed")
+           == -1) {
+               fprintf(stdout, "FAILED\n");
+               exit(1);
+       }
+
+       event_dispatch();
+
+       if (test_ok != 1) {
+               fprintf(stdout, "FAILED\n");
+               exit(1);
+       }
+
+       evhttp_connection_free(evcon);
+       evhttp_free(http);
+       
+       fprintf(stdout, "OK\n");
+}
+
 static void
 http_stream_in_chunk(struct evhttp_request *req, void *arg)
 {
@@ -1885,7 +1941,8 @@ http_stream_in_test(void)
            "This is funnybut not hilarious.bwv 1052");
 
        printf("Testing streamed reading of non-chunked response: ");
-       _http_stream_in_test("/test", 13, "This is funny");
+       _http_stream_in_test("/test", strlen(BASIC_REQUEST_BODY),
+           BASIC_REQUEST_BODY);
 }
 
 static void
@@ -2273,7 +2330,8 @@ http_suite(void)
        http_incomplete_test(0 /* use_timeout */);
        http_incomplete_test(1 /* use_timeout */);
 
-       http_chunked_test();
+       http_chunk_out_test();
+       http_stream_out_test();
 
        http_stream_in_test();
        http_stream_in_cancel_test();