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