[libdmapsharing] Improve error handling surrounding dmap_structure_parse and write unit tests

[W. Michael Petullo <wmpetullo src gnome org>]

commit a50bd508e7ee4d3daed41e36576c2f132d68f89c
Author: W. Michael Petullo <mike flyn org>
Date:   Sat Jul 21 17:17:00 2018 -0400

    Improve error handling surrounding dmap_structure_parse and write unit tests
    
    Signed-off-by: W. Michael Petullo <mike flyn org>

 libdmapsharing/dmap-av-share.c   |  10 +--
 libdmapsharing/dmap-connection.c | 146 ++++++++++++++++++++++++++++++++++++---
 libdmapsharing/dmap-connection.h |  10 +++
 libdmapsharing/dmap-structure.c  |  55 ++++++++-------
 libdmapsharing/dmap-structure.h  |   2 +-
 5 files changed, 184 insertions(+), 39 deletions(-)
---
diff --git a/libdmapsharing/dmap-av-share.c b/libdmapsharing/dmap-av-share.c
index 06e3244..df859d7 100644
--- a/libdmapsharing/dmap-av-share.c
+++ b/libdmapsharing/dmap-av-share.c
@@ -1405,7 +1405,7 @@ START_TEST(dmap_av_share_server_info_test)
        buffer = soup_message_body_flatten(body);
        soup_buffer_get_data(buffer, &data, &length);
 
-       root = dmap_structure_parse(data, length);
+       root = dmap_structure_parse(data, length, NULL);
 
        item = dmap_structure_find_item(root, DMAP_CC_MSTT);
        ck_assert_int_eq(DMAP_STATUS_OK, item->content.data->v_int);
@@ -1510,7 +1510,7 @@ START_TEST(_databases_browse_xxx_test)
        buffer = soup_message_body_flatten(body);
        soup_buffer_get_data(buffer, &data, &length);
 
-       root = dmap_structure_parse(data, length);
+       root = dmap_structure_parse(data, length, NULL);
 
        item = dmap_structure_find_item(root, DMAP_CC_MSTT);
        ck_assert_int_eq(DMAP_STATUS_OK, item->content.data->v_int);
@@ -1566,7 +1566,7 @@ START_TEST(_databases_browse_xxx_artists_test)
        buffer = soup_message_body_flatten(body);
        soup_buffer_get_data(buffer, &data, &length);
 
-       root = dmap_structure_parse(data, length);
+       root = dmap_structure_parse(data, length, NULL);
 
        root = dmap_structure_find_node(root, DMAP_CC_MLIT);
        ck_assert(NULL != root);
@@ -1607,7 +1607,7 @@ START_TEST(_databases_browse_xxx_albums_test)
        buffer = soup_message_body_flatten(body);
        soup_buffer_get_data(buffer, &data, &length);
 
-       root = dmap_structure_parse(data, length);
+       root = dmap_structure_parse(data, length, NULL);
 
        root = dmap_structure_find_node(root, DMAP_CC_MLIT);
        ck_assert(NULL != root);
@@ -1648,7 +1648,7 @@ START_TEST(_databases_browse_xxx_bad_category_test)
        buffer = soup_message_body_flatten(body);
        soup_buffer_get_data(buffer, &data, &length);
 
-       root = dmap_structure_parse(data, length);
+       root = dmap_structure_parse(data, length, NULL);
        ck_assert(NULL == root);
 
        g_object_unref(share);
diff --git a/libdmapsharing/dmap-connection.c b/libdmapsharing/dmap-connection.c
index 5d6990c..3daf887 100644
--- a/libdmapsharing/dmap-connection.c
+++ b/libdmapsharing/dmap-connection.c
@@ -32,6 +32,7 @@
 
 #include "dmap-md5.h"
 #include "dmap-connection.h"
+#include "dmap-error.h"
 #include "dmap-record-factory.h"
 #include "dmap-marshal.h"
 #include "dmap-structure.h"
@@ -607,7 +608,7 @@ typedef struct {
 
        DmapResponseHandler response_handler;
        gpointer user_data;
-} DAAPResponseData;
+} DmapResponseData;
 
 static gboolean
 _emit_progress_idle (DmapConnection * connection)
@@ -623,7 +624,7 @@ _emit_progress_idle (DmapConnection * connection)
 }
 
 static void
-_actual_http_response_handler (DAAPResponseData * data)
+_actual_http_response_handler (DmapResponseData * data)
 {
        DmapConnectionPrivate *priv;
        GNode *structure;
@@ -749,6 +750,7 @@ _actual_http_response_handler (DAAPResponseData * data)
                _connection_set_error_message (data->connection,
                                              ("libdmapsharing is not able to connect to iTunes 7 shares"));
        } else if (SOUP_STATUS_IS_SUCCESSFUL (data->status)) {
+               GError *error = NULL;
                DmapStructureItem *item;
 
                if ( /* FIXME: !rb_is_main_thread () */ TRUE) {
@@ -760,12 +762,13 @@ _actual_http_response_handler (DAAPResponseData * data)
                                g_idle_add ((GSourceFunc) _emit_progress_idle,
                                            data->connection);
                }
-               structure = dmap_structure_parse (response, response_length);
+               structure = dmap_structure_parse (response, response_length, &error);
                if (structure == NULL) {
-                       g_debug ("No daap structure returned from %s",
-                                message_path);
-
+                       dmap_connection_emit_error(data->connection, DMAP_ERROR_FAILED,
+                                                 "Error parsing %s response: %s\n", message_path,
+                                                  error->message);
                        data->status = SOUP_STATUS_MALFORMED;
+                       g_clear_error(&error);
                } else {
                        int dmap_status = 0;
 
@@ -818,7 +821,7 @@ _actual_http_response_handler (DAAPResponseData * data)
 
 static void
 _http_response_handler (SoupSession * session,
-                        SoupMessage * message, DAAPResponseData * data)
+                        SoupMessage * message, DmapResponseData * data)
 {
        int response_length;
 
@@ -866,7 +869,7 @@ _http_get (DmapConnection * connection,
 {
        gboolean ok = FALSE;
        DmapConnectionPrivate *priv = connection->priv;
-       DAAPResponseData *data;
+       DmapResponseData *data;
        SoupMessage *message;
 
        message = _build_message (connection, path, need_hash,
@@ -879,7 +882,7 @@ _http_get (DmapConnection * connection,
 
        priv->use_response_handler_thread = use_thread;
 
-       data = g_new0 (DAAPResponseData, 1);
+       data = g_new0 (DmapResponseData, 1);
        data->response_handler = handler;
        data->user_data = user_data;
 
@@ -1828,3 +1831,128 @@ dmap_connection_get_playlists (DmapConnection * connection)
 {
        return connection->priv->playlists;
 }
+
+void
+dmap_connection_emit_error(DmapConnection *connection, gint code,
+                           const gchar *format, ...)
+{
+       va_list ap;
+       GError *error;
+
+       va_start(ap, format);
+       error = g_error_new_valist(DMAP_ERROR, code, format, ap);
+       g_signal_emit_by_name(connection, "error", error);
+
+       va_end(ap);
+}
+
+#ifdef HAVE_CHECK
+
+#include <check.h>
+#include <libdmapsharing/dmap-av-connection.h>
+
+static gboolean _error_triggered = FALSE;
+
+static void
+_error_cb(DmapConnection *share, GError *error, gpointer user_data)
+{
+       _error_triggered = TRUE;
+}
+
+START_TEST(_actual_http_response_handler_test)
+{
+       SoupMessage      *message;
+       DmapConnection   *connection;
+       const char        body[] = "minm\x00\x00\x00\x0dHello, world!";
+       DmapResponseData *data;
+
+       _error_triggered = FALSE;
+
+       message = soup_message_new(SOUP_METHOD_GET, "http://test/";);
+       soup_message_set_response(message,
+                                "application/x-dmap-tagged",
+                                 SOUP_MEMORY_STATIC,
+                                 body,
+                                 sizeof body);
+       soup_message_set_status(message, SOUP_STATUS_OK);
+
+       connection = g_object_new(DMAP_TYPE_AV_CONNECTION, NULL);
+       g_signal_connect(connection, "error", G_CALLBACK(_error_cb), NULL);
+
+       data = g_new0(DmapResponseData, 1);
+       data->message    = message;
+       data->status     = SOUP_STATUS_OK;
+       data->connection = connection;
+
+       _actual_http_response_handler(data);
+
+       ck_assert(!_error_triggered);
+}
+END_TEST
+
+START_TEST(_actual_http_response_handler_bad_cc_test)
+{
+       SoupMessage      *message;
+       DmapConnection   *connection;
+       const char       *body = "xxx";
+       DmapResponseData *data;
+
+       _error_triggered = FALSE;
+
+       message = soup_message_new(SOUP_METHOD_GET, "http://test/";);
+       soup_message_set_response(message,
+                                "application/x-dmap-tagged",
+                                 SOUP_MEMORY_STATIC,
+                                 body,
+                                 sizeof body);
+       soup_message_set_status(message, SOUP_STATUS_OK);
+
+       connection = g_object_new(DMAP_TYPE_AV_CONNECTION, NULL);
+       g_signal_connect(connection, "error", G_CALLBACK(_error_cb), NULL);
+
+       data = g_new0(DmapResponseData, 1);
+       data->message    = message;
+       data->status     = SOUP_STATUS_OK;
+       data->connection = connection;
+
+       _actual_http_response_handler(data);
+
+       ck_assert(_error_triggered);
+}
+END_TEST
+
+START_TEST(_actual_http_response_handler_bad_length_test)
+{
+       SoupMessage      *message;
+       DmapConnection   *connection;
+       /* Length of 99 is larger than sizeof containing array. */
+       const char        body[] = "minm\x00\x00\x00\x99Hello, world!";
+       DmapResponseData *data;
+
+       _error_triggered = FALSE;
+
+       message = soup_message_new(SOUP_METHOD_GET, "http://test/";);
+       soup_message_set_response(message,
+                                "application/x-dmap-tagged",
+                                 SOUP_MEMORY_STATIC,
+                                 body,
+                                 sizeof body);
+       soup_message_set_status(message, SOUP_STATUS_OK);
+
+       connection = g_object_new(DMAP_TYPE_AV_CONNECTION, NULL);
+       g_signal_connect(connection, "error", G_CALLBACK(_error_cb), NULL);
+
+       data = g_new0(DmapResponseData, 1);
+       data->message    = message;
+       data->status     = SOUP_STATUS_OK;
+       data->connection = connection;
+
+       _actual_http_response_handler(data);
+
+       ck_assert(_error_triggered);
+}
+END_TEST
+
+#include "dmap-connection-suite.c"
+
+#endif
diff --git a/libdmapsharing/dmap-connection.h b/libdmapsharing/dmap-connection.h
index 3f8c323..dd712b6 100644
--- a/libdmapsharing/dmap-connection.h
+++ b/libdmapsharing/dmap-connection.h
@@ -223,5 +223,15 @@ void dmap_connection_authenticate_message (DmapConnection *connection,
                                           SoupAuth *auth,
                                           const char *password);
 
+/**
+ * dmap_connection_emit_error:
+ * @connection: a #DmapConnection instance.
+ * @code: error code.
+ * @format: printf()-style format for error message
+ * @...: parameters for message format
+ */
+void dmap_connection_emit_error(DmapConnection *connection, gint code,
+                                const gchar *format, ...);
+
 G_END_DECLS
 #endif /* _DMAP_CONNECTION_H */
diff --git a/libdmapsharing/dmap-structure.c b/libdmapsharing/dmap-structure.c
index b922204..3b64ecf 100644
--- a/libdmapsharing/dmap-structure.c
+++ b/libdmapsharing/dmap-structure.c
@@ -18,6 +18,7 @@
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA*
  */
 
+#include "dmap-error.h"
 #include "dmap-structure.h"
 #include "dmap-private-utils.h"
 
@@ -355,14 +356,15 @@ _cc_name (DmapContentCode code)
 }
 
 static DmapType
-_cc_dmap_type (DmapContentCode code)
+_cc_dmap_type (DmapContentCode code, GError **error)
 {
        DmapType type = DMAP_TYPE_INVALID;
 
        if (code < sizeof _cc_defs / sizeof(DmapContentCodeDefinition)) {
                type = _cc_defs[code - 1].type;
        } else {
-               g_warning("Invalid content code: %d\n", code);
+               g_set_error(error, DMAP_ERROR, DMAP_ERROR_FAILED,
+                          "Invalid content code: %d", code);
        }
 
        return type;
@@ -375,11 +377,11 @@ _cc_string (DmapContentCode code)
 }
 
 static GType
-_cc_gtype (DmapContentCode code)
+_cc_gtype (DmapContentCode code, GError **error)
 {
        GType type = G_TYPE_NONE;
 
-       switch (_cc_dmap_type (code)) {
+       switch (_cc_dmap_type (code, error)) {
        case DMAP_TYPE_BYTE:
        case DMAP_TYPE_SIGNED_INT:
                type = G_TYPE_CHAR;
@@ -427,7 +429,7 @@ _node_serialize (GNode * node, GByteArray * array)
                g_byte_array_append (array, (const guint8 *) &size, 4);
        }
 
-       dmap_type = _cc_dmap_type (item->content_code);
+       dmap_type = _cc_dmap_type (item->content_code, NULL);
 
        switch (dmap_type) {
        case DMAP_TYPE_BYTE:
@@ -531,8 +533,8 @@ dmap_structure_serialize (GNode * structure, guint * length)
        return data;
 }
 
-DmapContentCode
-dmap_structure_cc_read_from_buffer (const gchar * buf)
+static DmapContentCode
+_cc_read_from_buffer (const gchar * buf, GError **error)
 {
        DmapContentCode cc = DMAP_CC_INVALID;
 
@@ -546,7 +548,8 @@ dmap_structure_cc_read_from_buffer (const gchar * buf)
                }
        }
 
-       g_warning ("Content code %4s is invalid.", buf);
+       g_set_error(error, DMAP_ERROR, DMAP_ERROR_FAILED,
+                  "Invalid content code: %4s", buf);
 
 done:
        return cc;
@@ -567,7 +570,8 @@ _read_string (const guint8 * buf, gsize size)
 }
 
 static void
-_parse_container_buffer (GNode * parent, const guint8 * buf, gsize buf_length)
+_parse_container_buffer (GNode * parent, const guint8 * buf,
+                         gsize buf_length, GError **error)
 {
        gint l = 0;
 
@@ -602,12 +606,12 @@ _parse_container_buffer (GNode * parent, const guint8 * buf, gsize buf_length)
                 * content_code and 4 of size) is odd.
                 */
                if (buf_length - l < 8) {
-                       g_debug ("Malformed response received\n");
+                       g_set_error(error, DMAP_ERROR, DMAP_ERROR_FAILED,
+                                  "Malformed response received");
                        goto done;
                }
 
-               cc = dmap_structure_cc_read_from_buffer ((const gchar *)
-                                                       &(buf[l]));
+               cc = _cc_read_from_buffer ((const gchar *) &(buf[l]), error);
                if (cc == DMAP_CC_INVALID) {
                        goto done;
                }
@@ -621,7 +625,9 @@ _parse_container_buffer (GNode * parent, const guint8 * buf, gsize buf_length)
                 * then get out before we start processing it
                 */
                if (codesize > buf_length - l - 4 || codesize < 0) {
-                       g_debug ("Invalid codesize %d received in buf_length %zd\n", codesize, buf_length);
+                       g_set_error(error, DMAP_ERROR, DMAP_ERROR_FAILED,
+                                  "Invalid codesize %d received in buffer of length %zd",
+                                   codesize, buf_length);
                        goto done;
                }
                l += 4;
@@ -631,13 +637,13 @@ _parse_container_buffer (GNode * parent, const guint8 * buf, gsize buf_length)
                node = g_node_new (item);
                g_node_append (parent, node);
 
-               gtype = _cc_gtype (item->content_code);
+               gtype = _cc_gtype (item->content_code, error);
 
                if (gtype != G_TYPE_NONE) {
                        g_value_init (&(item->content), gtype);
                }
 // FIXME USE THE G_TYPE CONVERTOR FUNCTION dmap_type_to_gtype
-               switch (_cc_dmap_type (item->content_code)) {
+               switch (_cc_dmap_type (item->content_code, error)) {
                case DMAP_TYPE_SIGNED_INT:
                case DMAP_TYPE_BYTE:{
                                gchar c = 0;
@@ -728,14 +734,14 @@ _parse_container_buffer (GNode * parent, const guint8 * buf, gsize buf_length)
                                break;
                        }
                case DMAP_TYPE_CONTAINER:{
-                               _parse_container_buffer (node, &(buf[l]), codesize);
+                               _parse_container_buffer (node, &(buf[l]), codesize, error);
                                break;
                        }
                case DMAP_TYPE_INVALID:
                default:
                        /*
                         * Bad type should have been caught as bad content code
-                        * by dmap_structure_cc_read_from_buffer()
+                        * by _cc_read_from_buffer()
                         */
                        g_assert_not_reached();
                }
@@ -748,14 +754,14 @@ done:
 }
 
 GNode *
-dmap_structure_parse (const guint8 * buf, gsize buf_length)
+dmap_structure_parse (const guint8 * buf, gsize buf_length, GError **error)
 {
-       GNode *root = NULL;
+       GNode *root  = NULL;
        GNode *child = NULL;
 
        root = g_node_new (NULL);
 
-       _parse_container_buffer (root, (guchar *) buf, buf_length);
+       _parse_container_buffer (root, (guchar *) buf, buf_length, error);
 
        child = root->children;
        if (child) {
@@ -814,8 +820,8 @@ dmap_structure_add (GNode * parent, DmapContentCode cc, ...)
 
        va_start (list, cc);
 
-       dmap_type = _cc_dmap_type (cc);
-       gtype = _cc_gtype (cc);
+       dmap_type = _cc_dmap_type (cc, NULL);
+       gtype = _cc_gtype (cc, NULL);
 
        item = g_new0 (DmapStructureItem, 1);
        item->content_code = cc;
@@ -921,7 +927,7 @@ dmap_structure_find_node (GNode * structure, DmapContentCode code)
 static void
 _dmap_item_free (DmapStructureItem * item)
 {
-       DmapType type = _cc_dmap_type (item->content_code);
+       DmapType type = _cc_dmap_type (item->content_code, NULL);
 
        if (DMAP_TYPE_INVALID != type && DMAP_TYPE_CONTAINER != type) {
                g_value_unset (&(item->content));
@@ -965,10 +971,11 @@ dmap_structure_cc_string_as_int32 (const gchar * str)
        union
        {
                gint32 i;
-               gchar str[4];
+               gchar str[5];
        } u;
 
        strncpy (u.str, str, 4);
+       u.str[4] = 0x00;
 
        return g_htonl (u.i);
 }
diff --git a/libdmapsharing/dmap-structure.h b/libdmapsharing/dmap-structure.h
index 52ecb92..1ce64bb 100644
--- a/libdmapsharing/dmap-structure.h
+++ b/libdmapsharing/dmap-structure.h
@@ -37,7 +37,7 @@ struct _DmapStructureItem
 
 GNode *dmap_structure_add (GNode * parent, DmapContentCode cc, ...);
 gchar *dmap_structure_serialize (GNode * structure, guint * length);
-GNode *dmap_structure_parse (const guint8 * buf, gsize buf_length);
+GNode *dmap_structure_parse (const guint8 * buf, gsize buf_length, GError **error);
 DmapStructureItem *dmap_structure_find_item (GNode * structure,
                                             DmapContentCode code);
 GNode *dmap_structure_find_node (GNode * structure, DmapContentCode code);