soup-message-io: fix wrong-Content-Length logic

Previously we just ignored Content-Length if the server specified
"Connection: close", which does the right thing if the specified
Content-Length is too large, but fails if the Content-Length is
correct but the server "forgets" to close the connection. Fix this so
that we always stop reading once we get to the expected
Content-Length, but we also cope with the connection closing before
that point.

https://bugzilla.gnome.org/show_bug.cgi?id=611481

diff --git a/libsoup/soup-message-client-io.c b/libsoup/soup-message-client-io.c
index ef97c99..745d921 100644 (file)
--- a/libsoup/soup-message-client-io.c
+++ b/libsoup/soup-message-client-io.c
@@ -59,18 +59,6 @@ parse_response_headers (SoupMessage *req,
        if (*encoding == SOUP_ENCODING_UNRECOGNIZED)
                return SOUP_STATUS_MALFORMED;
 
-        /* mirror Mozilla here:
-         * (see netwerk/protocol/http/src/nsHttpTransaction.cpp for details)
-         *
-         * HTTP servers have been known to send erroneous Content-Length headers.
-         * So, unless the connection is persistent, we must make allowances for a
-         * possibly invalid Content-Length header. Thus, if NOT persistent, we 
-         * simply accept the whole message.
-         */
-        if (*encoding == SOUP_ENCODING_CONTENT_LENGTH &&
-            !soup_message_is_keepalive (req))
-                *encoding = SOUP_ENCODING_EOF;
-
        return SOUP_STATUS_OK;
 }
 

diff --git a/libsoup/soup-message-io.c b/libsoup/soup-message-io.c
index ff1fec2..f4f4c96 100644 (file)
--- a/libsoup/soup-message-io.c
+++ b/libsoup/soup-message-io.c
@@ -54,6 +54,7 @@ typedef struct {
        GByteArray           *read_meta_buf;
        SoupMessageBody      *read_body;
        goffset               read_length;
+       gboolean              read_eof_ok;
 
        gboolean              need_content_sniffed, need_got_chunk;
        SoupMessageBody      *sniff_data;
@@ -212,8 +213,7 @@ io_disconnected (SoupSocket *sock, SoupMessage *msg)
        SoupMessageIOData *io = priv->io_data;
 
        /* Closing the connection to signify EOF is sometimes ok */
-       if (io->read_state == SOUP_MESSAGE_IO_STATE_BODY &&
-           io->read_encoding == SOUP_ENCODING_EOF) {
+       if (io->read_state == SOUP_MESSAGE_IO_STATE_BODY && io->read_eof_ok) {
                io->read_state = SOUP_MESSAGE_IO_STATE_FINISHING;
                io_read (sock, msg);
                return;
@@ -462,8 +462,10 @@ read_body_chunk (SoupMessage *msg)
                        break;
 
                case SOUP_SOCKET_EOF:
-                       if (read_to_eof)
+                       if (io->read_eof_ok) {
+                               io->read_length = 0;
                                return TRUE;
+                       }
                        /* else fall through */
 
                case SOUP_SOCKET_ERROR:
@@ -842,11 +844,24 @@ io_read (SoupSocket *sock, SoupMessage *msg)
                        break;
                }
 
+               if (io->read_encoding == SOUP_ENCODING_EOF)
+                       io->read_eof_ok = TRUE;
+
                if (io->read_encoding == SOUP_ENCODING_CONTENT_LENGTH) {
                        SoupMessageHeaders *hdrs =
                                (io->mode == SOUP_MESSAGE_IO_CLIENT) ?
                                msg->response_headers : msg->request_headers;
                        io->read_length = soup_message_headers_get_content_length (hdrs);
+
+                       if (io->mode == SOUP_MESSAGE_IO_CLIENT &&
+                           !soup_message_is_keepalive (msg)) {
+                               /* Some servers suck and send
+                                * incorrect Content-Length values, so
+                                * allow EOF termination in this case
+                                * (iff the message is too short) too.
+                                */
+                               io->read_eof_ok = TRUE;
+                       }
                }
 
                if (io->mode == SOUP_MESSAGE_IO_CLIENT &&

diff --git a/tests/misc-test.c b/tests/misc-test.c
index dabbb06..8b462c5 100644 (file)
--- a/tests/misc-test.c
+++ b/tests/misc-test.c
@@ -29,6 +29,20 @@ auth_callback (SoupAuthDomain *auth_domain, SoupMessage *msg,
 }
 
 static void
+forget_close (SoupMessage *msg, gpointer user_data)
+{
+       soup_message_headers_remove (msg->response_headers, "Connection");
+}
+
+static void
+close_socket (SoupMessage *msg, gpointer user_data)
+{
+       SoupSocket *sock = user_data;
+
+       soup_socket_disconnect (sock);
+}
+
+static void
 server_callback (SoupServer *server, SoupMessage *msg,
                 const char *path, GHashTable *query,
                 SoupClientContext *context, gpointer data)
@@ -61,6 +75,41 @@ server_callback (SoupServer *server, SoupMessage *msg,
                return;
        }
 
+       if (g_str_has_prefix (path, "/content-length/")) {
+               gboolean too_long = strcmp (path, "/content-length/long") == 0;
+               gboolean no_close = strcmp (path, "/content-length/noclose") == 0;
+
+               soup_message_set_status (msg, SOUP_STATUS_OK);
+               soup_message_set_response (msg, "text/plain",
+                                          SOUP_MEMORY_STATIC, "foobar", 6);
+               if (too_long)
+                       soup_message_headers_set_content_length (msg->response_headers, 9);
+               soup_message_headers_append (msg->response_headers,
+                                            "Connection", "close");
+
+               if (too_long) {
+                       SoupSocket *sock;
+
+                       /* soup-message-io will wait for us to add
+                        * another chunk after the first, to fill out
+                        * the declared Content-Length. Instead, we
+                        * forcibly close the socket at that point.
+                        */
+                       sock = soup_client_context_get_socket (context);
+                       g_signal_connect (msg, "wrote-chunk",
+                                         G_CALLBACK (close_socket), sock);
+               } else if (no_close) {
+                       /* Remove the 'Connection: close' after writing
+                        * the headers, so that when we check it after
+                        * writing the body, we'll think we aren't
+                        * supposed to close it.
+                        */
+                       g_signal_connect (msg, "wrote-headers",
+                                         G_CALLBACK (forget_close), NULL);
+               }
+               return;
+       }
+
        soup_message_set_status (msg, SOUP_STATUS_OK);
        if (!strcmp (uri->host, "foo")) {
                soup_message_set_response (msg, "text/plain",
@@ -473,6 +522,65 @@ do_early_abort_test (void)
        soup_test_session_abort_unref (session);
 }
 
+static void
+do_content_length_framing_test (void)
+{
+       SoupSession *session;
+       SoupMessage *msg;
+       SoupURI *request_uri;
+       goffset declared_length;
+
+       debug_printf (1, "\nInvalid Content-Length framing tests\n");
+
+       session = soup_test_session_new (SOUP_TYPE_SESSION_ASYNC, NULL);
+
+       debug_printf (1, "  Content-Length larger than message body length\n");
+       request_uri = soup_uri_new_with_base (base_uri, "/content-length/long");
+       msg = soup_message_new_from_uri ("GET", request_uri);
+       soup_session_send_message (session, msg);
+       if (msg->status_code != SOUP_STATUS_OK) {
+               debug_printf (1, "    Unexpected response: %d %s\n",
+                             msg->status_code, msg->reason_phrase);
+               errors++;
+       } else {
+               declared_length = soup_message_headers_get_content_length (msg->response_headers);
+               debug_printf (2, "    Content-Length: %lu, body: %s\n",
+                             (gulong)declared_length, msg->response_body->data);
+               if (msg->response_body->length >= declared_length) {
+                       debug_printf (1, "    Body length %lu >= declared length %lu\n",
+                                     (gulong)msg->response_body->length,
+                                     (gulong)declared_length);
+                       errors++;
+               }
+       }
+       soup_uri_free (request_uri);
+       g_object_unref (msg);
+
+       debug_printf (1, "  Server claims 'Connection: close' but doesn't\n");
+       request_uri = soup_uri_new_with_base (base_uri, "/content-length/noclose");
+       msg = soup_message_new_from_uri ("GET", request_uri);
+       soup_session_send_message (session, msg);
+       if (msg->status_code != SOUP_STATUS_OK) {
+               debug_printf (1, "    Unexpected response: %d %s\n",
+                             msg->status_code, msg->reason_phrase);
+               errors++;
+       } else {
+               declared_length = soup_message_headers_get_content_length (msg->response_headers);
+               debug_printf (2, "    Content-Length: %lu, body: %s\n",
+                             (gulong)declared_length, msg->response_body->data);
+               if (msg->response_body->length != declared_length) {
+                       debug_printf (1, "    Body length %lu != declared length %lu\n",
+                                     (gulong)msg->response_body->length,
+                                     (gulong)declared_length);
+                       errors++;
+               }
+       }
+       soup_uri_free (request_uri);
+       g_object_unref (msg);
+
+       soup_test_session_abort_unref (session);
+}
+
 int
 main (int argc, char **argv)
 {
@@ -498,6 +606,7 @@ main (int argc, char **argv)
        do_msg_reuse_test ();
        do_star_test ();
        do_early_abort_test ();
+       do_content_length_framing_test ();
 
        soup_uri_free (base_uri);
        soup_test_server_quit_unref (server);