From f5d7586a12f5313d6301ba96aadaa06d84f2fc21 Mon Sep 17 00:00:00 2001 From: petervo Date: Fri, 24 Apr 2015 02:26:24 -0700 Subject: [PATCH] bridge: Fix bug with streaming chunked data Fixes #2170 Closes #2204 Signed-off-by: Stef Walter * Tweak the comments a bit, change order of checks --- src/bridge/cockpithttpstream.c | 6 ++ src/bridge/mock-transport.c | 25 ++++++++ src/bridge/mock-transport.h | 4 ++ src/bridge/test-httpstream.c | 132 +++++++++++++++++++++++++++++++++++++++++ src/bridge/test-packages.c | 30 +--------- 5 files changed, 170 insertions(+), 27 deletions(-) diff --git a/src/bridge/cockpithttpstream.c b/src/bridge/cockpithttpstream.c index d9ee3df..0e573a9 100644 --- a/src/bridge/cockpithttpstream.c +++ b/src/bridge/cockpithttpstream.c @@ -426,6 +426,12 @@ relay_chunked (CockpitHttpStream *self, return FALSE; /* want more data */ beg = (pos + 2) - data; + if (length < beg) + { + /* have to have a least the ending chars */ + return FALSE; /* want more data */ + } + size = g_ascii_strtoull (data, &end, 16); if (pos[1] != '\n' || end != pos) { diff --git a/src/bridge/mock-transport.c b/src/bridge/mock-transport.c index 4ad2b41..8ff0892 100644 --- a/src/bridge/mock-transport.c +++ b/src/bridge/mock-transport.c @@ -194,3 +194,28 @@ mock_transport_count_sent (MockTransport *mock) { return mock->count; } + +GBytes * +mock_transport_combine_output (MockTransport *transport, + const gchar *channel_id, + guint *count) +{ + GByteArray *combined; + GBytes *block; + + if (count) + *count = 0; + + combined = g_byte_array_new (); + for (;;) + { + block = mock_transport_pop_channel (transport, channel_id); + if (!block) + break; + + g_byte_array_append (combined, g_bytes_get_data (block, NULL), g_bytes_get_size (block)); + if (count) + (*count)++; + } + return g_byte_array_free_to_bytes (combined); +} diff --git a/src/bridge/mock-transport.h b/src/bridge/mock-transport.h index 6722870..e824051 100644 --- a/src/bridge/mock-transport.h +++ b/src/bridge/mock-transport.h @@ -49,4 +49,8 @@ JsonObject * mock_transport_pop_control (MockTransport *mock); GBytes * mock_transport_pop_channel (MockTransport *mock, const gchar *channel); +GBytes * mock_transport_combine_output (MockTransport *transport, + const gchar *channel_id, + guint *count); + #endif /* MOCK_TRANSPORT_H */ diff --git a/src/bridge/test-httpstream.c b/src/bridge/test-httpstream.c index 68b27ae..a4ef436 100644 --- a/src/bridge/test-httpstream.c +++ b/src/bridge/test-httpstream.c @@ -23,6 +23,8 @@ #include "cockpithttpstream.h" #include "cockpithttpstream.c" #include "common/cockpittest.h" +#include "common/cockpitwebresponse.h" +#include "common/cockpitwebserver.h" #include "mock-transport.h" #include @@ -31,6 +33,135 @@ * Test */ +typedef struct { + gchar *problem; + gboolean done; +} TestResult; + +/* + * Yes this is a magic number. It's the lowest number that would + * trigger a bug where chunked data would be rejected due to an incomplete read. + */ +const gint MAGIC_NUMBER = 3068; + +static gboolean +handle_chunked (CockpitWebServer *server, + const gchar *path, + GHashTable *headers, + CockpitWebResponse *response, + gpointer user_data) +{ + GBytes *bytes; + GHashTable *h = g_hash_table_new (g_str_hash, g_str_equal); + + cockpit_web_response_headers_full (response, 200, + "OK", -1, h); + bytes = g_bytes_new_take (g_strdup_printf ("%0*d", + MAGIC_NUMBER, 0), + MAGIC_NUMBER); + cockpit_web_response_queue (response, bytes); + cockpit_web_response_complete (response); + + g_bytes_unref (bytes); + g_hash_table_unref (h); + return TRUE; +} + +static void +on_channel_close (CockpitChannel *channel, + const gchar *problem, + gpointer user_data) +{ + TestResult *tr = user_data; + g_assert (tr->done == FALSE); + tr->done = TRUE; + tr->problem = g_strdup (problem); +} + +static void +on_transport_closed (CockpitTransport *transport, + const gchar *problem, + gpointer user_data) +{ + g_assert_not_reached (); +} + +static void +test_http_chunked (void) +{ + MockTransport *transport = NULL; + CockpitChannel *channel = NULL; + CockpitWebServer *web_server = NULL; + JsonObject *options = NULL; + JsonObject *headers = NULL; + TestResult *tr = g_slice_new (TestResult); + + GBytes *bytes = NULL; + GBytes *data = NULL; + + const gchar *control; + gchar *expected = g_strdup_printf ("{\"status\":200,\"reason\":\"OK\",\"headers\":{}}%0*d", MAGIC_NUMBER, 0); + guint count; + guint port; + + web_server = cockpit_web_server_new (0, NULL, + NULL, NULL, NULL); + g_assert (web_server); + port = cockpit_web_server_get_port (web_server); + g_signal_connect (web_server, "handle-resource::/", + G_CALLBACK (handle_chunked), NULL); + + transport = mock_transport_new (); + g_signal_connect (transport, "closed", G_CALLBACK (on_transport_closed), NULL); + + options = json_object_new (); + json_object_set_int_member (options, "port", port); + json_object_set_string_member (options, "payload", "http-stream1"); + json_object_set_string_member (options, "method", "GET"); + json_object_set_string_member (options, "path", "/"); + + headers = json_object_new (); + json_object_set_string_member (headers, "Pragma", "no-cache"); + json_object_set_object_member (options, "headers", headers); + + channel = g_object_new (COCKPIT_TYPE_HTTP_STREAM, + "transport", transport, + "id", "444", + "options", options, + NULL); + + json_object_unref (options); + + /* Tell HTTP we have no more data to send */ + control = "{\"command\": \"done\", \"channel\": \"444\"}"; + bytes = g_bytes_new_static (control, strlen (control)); + cockpit_transport_emit_recv (COCKPIT_TRANSPORT (transport), NULL, bytes); + g_bytes_unref (bytes); + + tr->done = FALSE; + g_signal_connect (channel, "closed", G_CALLBACK (on_channel_close), tr); + + while (tr->done == FALSE) + g_main_context_iteration (NULL, TRUE); + g_assert_cmpstr (tr->problem, ==, NULL); + + data = mock_transport_combine_output (transport, "444", &count); + cockpit_assert_bytes_eq (data, expected, -1); + g_assert_cmpuint (count, ==, 2); + + g_bytes_unref (data); + g_free (expected); + + g_object_unref (transport); + g_object_add_weak_pointer (G_OBJECT (channel), (gpointer *)&channel); + g_object_unref (channel); + g_assert (channel == NULL); + g_clear_object (&web_server); + + g_free (tr->problem); + g_slice_free (TestResult, tr); +} + static void test_parse_keep_alive (void) { @@ -82,6 +213,7 @@ main (int argc, { cockpit_test_init (&argc, &argv); g_test_add_func ("/http-stream/parse_keepalive", test_parse_keep_alive); + g_test_add_func ("/http-stream/http_chunked", test_http_chunked); return g_test_run (); } diff --git a/src/bridge/test-packages.c b/src/bridge/test-packages.c index 09596b9..dbfb6d6 100644 --- a/src/bridge/test-packages.c +++ b/src/bridge/test-packages.c @@ -146,30 +146,6 @@ teardown (TestCase *tc, cockpit_bridge_data_dirs = NULL; } -static GBytes * -combine_output (TestCase *tc, - guint *count) -{ - GByteArray *combined; - GBytes *block; - - if (count) - *count = 0; - - combined = g_byte_array_new (); - for (;;) - { - block = mock_transport_pop_channel (tc->transport, "444"); - if (!block) - break; - - g_byte_array_append (combined, g_bytes_get_data (block, NULL), g_bytes_get_size (block)); - if (count) - (*count)++; - } - return g_byte_array_free_to_bytes (combined); -} - static const Fixture fixture_simple = { .path = "/test/sub/file.ext", }; @@ -187,7 +163,7 @@ test_simple (TestCase *tc, g_main_context_iteration (NULL, TRUE); g_assert_cmpstr (tc->problem, ==, NULL); - data = combine_output (tc, &count); + data = mock_transport_combine_output (tc->transport, "444", &count); cockpit_assert_bytes_eq (data, "{\"status\":200,\"reason\":\"OK\",\"headers\":{}}" "These are the contents of file.ext\nOh marmalaaade\n", -1); g_assert_cmpuint (count, ==, 2); @@ -220,7 +196,7 @@ test_large (TestCase *tc, &contents, &length, &error); g_assert_no_error (error); - data = combine_output (tc, &count); + data = mock_transport_combine_output (tc->transport, "444", &count); /* Should not have been sent as one block */ g_assert_cmpuint (count, ==, 8); @@ -442,7 +418,7 @@ test_list_bad_name (TestCase *tc, g_main_context_iteration (NULL, TRUE); g_assert_cmpstr (tc->problem, ==, NULL); - data = combine_output (tc, &count); + data = mock_transport_combine_output (tc->transport, "444", &count); cockpit_assert_bytes_eq (data, "{\"status\":200,\"reason\":\"OK\",\"headers\":" "{\"Content-Type\":\"application/json\"}}" "{\"ok\":{}}", -1); -- 2.3.5