From f5d7586a12f5313d6301ba96aadaa06d84f2fc21 Mon Sep 17 00:00:00 2001
From: petervo <petervo@redhat.com>
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 <stefw@redhat.com>
* 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 <json-glib/json-glib.h>
@@ -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