>From 8e94d85009e9684c94b5f7a79f83916fb118b8ef Mon Sep 17 00:00:00 2001
From: Stef Walter <stefw@redhat.com>
Date: Fri, 21 Feb 2014 16:59:48 +0100
Subject: [PATCH 6/6] sbus: Add the sbus_request_parse_or_finish() method

Some DBus types returned from dbus_message_get_args() require memory
to be released when done. We automatically attach these to the talloc
struct sbus_request memory context in this function.

This accepts varargs similar to dbus_message_get_args(), which are
rather awkward. However instead of reworking them completely, future
generated marshalling code will replace most uses of these varargs.

If parsing the dbus message fails, then it responds to the DBus caller
with an appropriate error such as o.f.D.Error.InvalidArgs. In these
cases (ie: when it returns FALSE) the sbus_request is finished.

Migrated some, but not all, uses of dbus_message_get_args() to the
new function. Some instances have uncommon semantics such as terminating
the connection upon failure to parse a message.
---
 src/providers/data_provider_be.c |  72 ++++++++------------------
 src/providers/proxy/proxy_init.c |  19 ++-----
 src/responder/nss/nsssrv.c       |  23 +++------
 src/sbus/sssd_dbus.h             |  49 ++++++++++++++++++
 src/sbus/sssd_dbus_request.c     | 106 +++++++++++++++++++++++++++++++++++++++
 src/tests/sbus_tests.c           |  99 ++++++++++++++++++++++++++++++++++++
 6 files changed, 287 insertions(+), 81 deletions(-)

diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index 6c0a823..0957bed 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -564,7 +564,6 @@ static int be_get_subdomains(struct sbus_request *dbus_req, void *user_data)
     struct be_subdom_req *req;
     struct be_req *be_req = NULL;
     struct be_client *becli;
-    DBusError dbus_error;
     dbus_bool_t force;
     char *domain_hint;
     dbus_uint16_t err_maj;
@@ -575,17 +574,11 @@ static int be_get_subdomains(struct sbus_request *dbus_req, void *user_data)
     becli = talloc_get_type(user_data, struct be_client);
     if (!becli) return EINVAL;
 
-    dbus_error_init(&dbus_error);
-
-    ret = dbus_message_get_args(dbus_req->message, &dbus_error,
-                                DBUS_TYPE_BOOLEAN, &force,
-                                DBUS_TYPE_STRING, &domain_hint,
-                                DBUS_TYPE_INVALID);
-    if (!ret) {
-        DEBUG(SSSDBG_CRIT_FAILURE,"Failed, to parse message!\n");
-        if (dbus_error_is_set(&dbus_error)) dbus_error_free(&dbus_error);
-        return EIO;
-    }
+    if (!sbus_request_parse_or_finish(dbus_req,
+                                      DBUS_TYPE_BOOLEAN, &force,
+                                      DBUS_TYPE_STRING, &domain_hint,
+                                      DBUS_TYPE_INVALID))
+        return EOK; /* handled */
 
     /* return an error if corresponding backend target is not configured */
     if (becli->bectx->bet_info[BET_SUBDOMAINS].bet_ops == NULL) {
@@ -1042,7 +1035,6 @@ static int be_get_account_info(struct sbus_request *dbus_req, void *user_data)
     struct be_acct_req *req;
     struct be_req *be_req;
     struct be_client *becli;
-    DBusError dbus_error;
     uint32_t type;
     char *filter;
     char *domain;
@@ -1057,19 +1049,13 @@ static int be_get_account_info(struct sbus_request *dbus_req, void *user_data)
     becli = talloc_get_type(user_data, struct be_client);
     if (!becli) return EINVAL;
 
-    dbus_error_init(&dbus_error);
-
-    ret = dbus_message_get_args(dbus_req->message, &dbus_error,
-                                DBUS_TYPE_UINT32, &type,
-                                DBUS_TYPE_UINT32, &attr_type,
-                                DBUS_TYPE_STRING, &filter,
-                                DBUS_TYPE_STRING, &domain,
-                                DBUS_TYPE_INVALID);
-    if (!ret) {
-        DEBUG(SSSDBG_CRIT_FAILURE,"Failed, to parse message!\n");
-        if (dbus_error_is_set(&dbus_error)) dbus_error_free(&dbus_error);
-        return EIO;
-    }
+    if (!sbus_request_parse_or_finish(dbus_req,
+                                      DBUS_TYPE_UINT32, &type,
+                                      DBUS_TYPE_UINT32, &attr_type,
+                                      DBUS_TYPE_STRING, &filter,
+                                      DBUS_TYPE_STRING, &domain,
+                                      DBUS_TYPE_INVALID))
+        return EOK; /* handled */
 
     DEBUG(SSSDBG_CONF_SETTINGS,
           "Got request for [%u][%d][%s]\n", type, attr_type, filter);
@@ -1613,7 +1599,6 @@ static void be_autofs_handler_callback(struct be_req *req,
 
 static int be_autofs_handler(struct sbus_request *dbus_req, void *user_data)
 {
-    DBusError dbus_error;
     struct be_client *be_cli = NULL;
     struct be_req *be_req = NULL;
     struct be_autofs_req *be_autofs_req = NULL;
@@ -1633,17 +1618,11 @@ static int be_autofs_handler(struct sbus_request *dbus_req, void *user_data)
         return EINVAL;
     }
 
-    dbus_error_init(&dbus_error);
-
-    ret = dbus_message_get_args(dbus_req->message, &dbus_error,
-                                DBUS_TYPE_UINT32, &type,
-                                DBUS_TYPE_STRING, &filter,
-                                DBUS_TYPE_INVALID);
-    if (!ret) {
-        DEBUG(SSSDBG_CRIT_FAILURE, "Failed, to parse message!\n");
-        if (dbus_error_is_set(&dbus_error)) dbus_error_free(&dbus_error);
-        return EIO;
-    }
+    if (!sbus_request_parse_or_finish(dbus_req,
+                                      DBUS_TYPE_UINT32, &type,
+                                      DBUS_TYPE_STRING, &filter,
+                                      DBUS_TYPE_INVALID))
+        return EOK; /* handled */
 
     /* If we are offline and fast reply was requested
      * return offline immediately
@@ -1817,7 +1796,6 @@ static int be_host_handler(struct sbus_request *dbus_req, void *user_data)
     struct be_host_req *req;
     struct be_req *be_req;
     struct be_client *becli;
-    DBusError dbus_error;
     uint32_t flags;
     char *filter;
     int ret;
@@ -1830,17 +1808,11 @@ static int be_host_handler(struct sbus_request *dbus_req, void *user_data)
     becli = talloc_get_type(user_data, struct be_client);
     if (!becli) return EINVAL;
 
-    dbus_error_init(&dbus_error);
-
-    ret = dbus_message_get_args(dbus_req->message, &dbus_error,
-                                DBUS_TYPE_UINT32, &flags,
-                                DBUS_TYPE_STRING, &filter,
-                                DBUS_TYPE_INVALID);
-    if (!ret) {
-        DEBUG(SSSDBG_CRIT_FAILURE, "Failed, to parse message!\n");
-        if (dbus_error_is_set(&dbus_error)) dbus_error_free(&dbus_error);
-        return EIO;
-    }
+    if (!sbus_request_parse_or_finish(dbus_req,
+                                      DBUS_TYPE_UINT32, &flags,
+                                      DBUS_TYPE_STRING, &filter,
+                                      DBUS_TYPE_INVALID))
+        return EOK; /* request finished */
 
     DEBUG(SSSDBG_TRACE_LIBS,
           "Got request for [%u][%s]\n", flags, filter);
diff --git a/src/providers/proxy/proxy_init.c b/src/providers/proxy/proxy_init.c
index 0206ec1..dd1b758 100644
--- a/src/providers/proxy/proxy_init.c
+++ b/src/providers/proxy/proxy_init.c
@@ -397,10 +397,8 @@ static int client_registration(struct sbus_request *dbus_req, void *data)
     dbus_uint16_t version = DATA_PROVIDER_VERSION;
     struct sbus_connection *conn;
     struct proxy_client *proxy_cli;
-    DBusError dbus_error;
     dbus_uint16_t cli_ver;
     uint32_t cli_id;
-    dbus_bool_t dbret;
     int hret;
     hash_key_t key;
     hash_value_t value;
@@ -421,19 +419,12 @@ static int client_registration(struct sbus_request *dbus_req, void *data)
           "Cancel proxy client ID timeout [%p]\n", proxy_cli->timeout);
     talloc_zfree(proxy_cli->timeout);
 
-    dbus_error_init(&dbus_error);
-
-    dbret = dbus_message_get_args(dbus_req->message, &dbus_error,
-                                  DBUS_TYPE_UINT16, &cli_ver,
-                                  DBUS_TYPE_UINT32, &cli_id,
-                                  DBUS_TYPE_INVALID);
-    if (!dbret) {
-        DEBUG(SSSDBG_CRIT_FAILURE,
-              "Failed to parse message, killing connection\n");
-        if (dbus_error_is_set(&dbus_error)) dbus_error_free(&dbus_error);
+    if (!sbus_request_parse_or_finish(dbus_req,
+                                      DBUS_TYPE_UINT16, &cli_ver,
+                                      DBUS_TYPE_UINT32, &cli_id,
+                                      DBUS_TYPE_INVALID)) {
         sbus_disconnect(conn);
-        /* FIXME: should we just talloc_zfree(conn) ? */
-        return EIO;
+        return EOK; /* handled */
     }
 
     DEBUG(SSSDBG_FUNC_DATA, "Proxy client [%"PRIu32"] connected\n", cli_id);
diff --git a/src/responder/nss/nsssrv.c b/src/responder/nss/nsssrv.c
index ca4fbfd..e4896a7 100644
--- a/src/responder/nss/nsssrv.c
+++ b/src/responder/nss/nsssrv.c
@@ -311,28 +311,17 @@ static int nss_memcache_initgr_check(struct sbus_request *dbus_req, void *data)
 {
     struct resp_ctx *rctx = talloc_get_type(data, struct resp_ctx);
     struct nss_ctx *nctx = talloc_get_type(rctx->pvt_ctx, struct nss_ctx);
-    DBusError dbus_error;
-    dbus_bool_t dbret;
     char *user;
     char *domain;
     uint32_t *groups;
     int gnum;
 
-    dbus_error_init(&dbus_error);
-
-    dbret = dbus_message_get_args(dbus_req->message, &dbus_error,
-                                  DBUS_TYPE_STRING, &user,
-                                  DBUS_TYPE_STRING, &domain,
-                                  DBUS_TYPE_ARRAY, DBUS_TYPE_UINT32,
-                                  &groups, &gnum,
-                                  DBUS_TYPE_INVALID);
-
-    if (!dbret) {
-        DEBUG(SSSDBG_CRIT_FAILURE, "Failed, to parse message!\n");
-        if (dbus_error_is_set(&dbus_error)) {
-            dbus_error_free(&dbus_error);
-        }
-        return EIO;
+    if (!sbus_request_parse_or_finish(dbus_req,
+                                      DBUS_TYPE_STRING, &user,
+                                      DBUS_TYPE_STRING, &domain,
+                                      DBUS_TYPE_ARRAY, DBUS_TYPE_UINT32, &groups, &gnum,
+                                      DBUS_TYPE_INVALID)) {
+        return EOK; /* handled */
     }
 
     DEBUG(SSSDBG_TRACE_LIBS,
diff --git a/src/sbus/sssd_dbus.h b/src/sbus/sssd_dbus.h
index ef728bd..3bf70c8 100644
--- a/src/sbus/sssd_dbus.h
+++ b/src/sbus/sssd_dbus.h
@@ -252,4 +252,53 @@ int sbus_request_return_and_finish(struct sbus_request *dbus_req,
 int sbus_request_fail_and_finish(struct sbus_request *dbus_req,
                                  const DBusError *error);
 
+/*
+ * Parse a DBus method call request.
+ *
+ * If parsing the method call message does not succeed, then an error is
+ * sent to the DBus caller and the request is finished. If this function
+ * returns false then @request is no longer valid.
+ *
+ * This also means if this method returns false within a handler, you should
+ * return EOK from the handler. The message has been handled, appropriate
+ * logs have been written, and everything should just move on.
+ *
+ * If the method call does not match the expected arguments, then a
+ * org.freedesktop.DBus.Error.InvalidArgs is returned to the caller as
+ * expected.
+ *
+ * The variable arguments are (unfortunately) formatted exactly the same
+ * as those of the dbus_message_get_args() function. Documented here:
+ *
+ * http://dbus.freedesktop.org/doc/api/html/group__DBusMessage.html
+ *
+ * Exception: You don't need to free string arrays returned by this
+ * function. They are automatically talloc parented to the request memory
+ * context and can be used until the request has been finished.
+ *
+ * Important: don't pass int or bool or such types as values to this
+ * function. That's not portable. Use actual dbus types. You must also pass
+ * pointers as the values:
+ *
+ *    dbus_bool_t val1;
+ *    dbus_int32_t val2;
+ *    ret = sbus_request_parse_or_finish(request,
+ *                                       DBUS_TYPE_BOOLEAN, &val1,
+ *                                       DBUS_TYPE_INT32, &val2,
+ *                                       DBUS_TYPE_INVALID);
+ *
+ * To pass arrays to this function, use the following syntax. Never
+ * pass actual C arrays with [] syntax to this function. The C standard is
+ * rather vague with C arrays and varargs, and it just plain doesn't work.
+ *
+ *    int count; // yes, a plain int
+ *    const char **array;
+ *    ret = sbus_request_parse_or_finish(request,
+ *                                       DBUS_TYPE_ARRAY, DBUS_TYPE_STRING, &array, &count,
+ *                                       DBUS_TYPE_INVALID);
+ */
+bool sbus_request_parse_or_finish(struct sbus_request *request,
+                                  int first_arg_type,
+                                  ...);
+
 #endif /* _SSSD_DBUS_H_*/
diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c
index 973089c..c45f3a9 100644
--- a/src/sbus/sssd_dbus_request.c
+++ b/src/sbus/sssd_dbus_request.c
@@ -110,3 +110,109 @@ int sbus_request_fail_and_finish(struct sbus_request *dbus_req,
     dbus_message_unref(reply);
     return ret;
 }
+
+struct array_arg {
+    char **dbus_array;
+};
+
+static int array_arg_destructor(struct array_arg *arg)
+{
+    dbus_free_string_array(arg->dbus_array);
+    return 0;
+}
+
+static bool
+parent_dbus_string_arrays(struct sbus_request *request, int first_arg_type,
+                          va_list va)
+{
+    struct array_arg *array_arg;
+    int arg_type;
+    void **arg_ptr;
+
+    /*
+     * Here we iterate through the entire thing again and look for
+     * things we need to fix allocation for. Normally certain types
+     * returned from dbus_message_get_args() and friends require
+     * later freeing. We tie those to the talloc context here.
+     *
+     * The list of argument has already been validated by the previous
+     * dbus_message_get_args() call, so we can be cheap.
+     */
+
+    arg_type = first_arg_type;
+    while (arg_type != DBUS_TYPE_INVALID) {
+
+        if (arg_type == DBUS_TYPE_ARRAY) {
+            arg_type = va_arg(va, int);     /* the array element type */
+            arg_ptr = va_arg(va, void **);  /* the array elements */
+            va_arg(va, int *);              /* the array length */
+
+            /* Arrays of these things need to be freed */
+            if (arg_type == DBUS_TYPE_STRING ||
+                arg_type == DBUS_TYPE_OBJECT_PATH ||
+                arg_type == DBUS_TYPE_SIGNATURE) {
+
+                array_arg = talloc_zero(request, struct array_arg);
+                if(array_arg == NULL) {
+                    /* no kidding ... */
+                    DEBUG(SSSDBG_CRIT_FAILURE, "Out of memory while trying not to leak memory\n");
+                    return false;
+                }
+
+                array_arg->dbus_array = *arg_ptr;
+                talloc_set_destructor(array_arg, array_arg_destructor);
+            }
+
+        /* A non array argument */
+        } else {
+            arg_ptr = va_arg(va, void**);
+        }
+
+        /* The next type */
+        arg_type = va_arg(va, int);
+    }
+
+    return true;
+}
+
+bool
+sbus_request_parse_or_finish(struct sbus_request *request,
+                             int first_arg_type,
+                             ...)
+{
+    DBusError error = DBUS_ERROR_INIT;
+    bool ret = true;
+    va_list va2;
+    va_list va;
+
+    va_start(va, first_arg_type);
+    va_copy(va2, va);
+
+    if (dbus_message_get_args_valist(request->message, &error,
+                                     first_arg_type, va)) {
+        ret = parent_dbus_string_arrays (request, first_arg_type, va2);
+
+    } else {
+        /* Trying to send the error back to the caller in this case is a joke */
+        if (!dbus_error_is_set(&error) || dbus_error_has_name(&error, DBUS_ERROR_NO_MEMORY)) {
+            DEBUG(SSSDBG_CRIT_FAILURE, "Out of memory parsing DBus message\n");
+            sbus_request_finish(request, NULL);
+
+        /* Log other errors and send them back, this include o.f.d.InvalidArgs */
+        } else {
+            DEBUG(SSSDBG_OP_FAILURE, "Couldn't parse DBus message %s.%s: %s\n",
+                  dbus_message_get_interface(request->message),
+                  dbus_message_get_member(request->message),
+                  error.message);
+            sbus_request_fail_and_finish(request, &error);
+        }
+
+        dbus_error_free(&error);
+        ret = false;
+    }
+
+    va_end(va2);
+    va_end(va);
+
+    return ret;
+}
diff --git a/src/tests/sbus_tests.c b/src/tests/sbus_tests.c
index 7290fe7..b8cd4ab 100644
--- a/src/tests/sbus_tests.c
+++ b/src/tests/sbus_tests.c
@@ -42,11 +42,13 @@
 
 #define PILOT_IFACE "test.Pilot"
 #define PILOT_BLINK "Blink"
+#define PILOT_EAT "Eat"
 
 /* our vtable */
 struct pilot_vtable {
     struct sbus_vtable vtable;
     sbus_msg_handler_fn Blink;
+    sbus_msg_handler_fn Eat;
 };
 
 const struct sbus_method_meta pilot_methods[] = {
@@ -56,6 +58,12 @@ const struct sbus_method_meta pilot_methods[] = {
         NULL, /* out args: manually parsed */
         offsetof(struct pilot_vtable, Blink),
     },
+    {
+        PILOT_EAT, /* method name */
+        NULL, /* in args: manually parsed */
+        NULL, /* out args: manually parsed */
+        offsetof(struct pilot_vtable, Eat),
+    },
     { NULL, }
 };
 
@@ -104,9 +112,35 @@ static int blink_handler(struct sbus_request *req, void *data)
                                           DBUS_TYPE_INVALID);
 }
 
+static int eat_handler(struct sbus_request *req, void *data)
+{
+    dbus_int32_t integer;
+    dbus_bool_t boolean;
+    const char **array;
+    int count;
+
+    if (!sbus_request_parse_or_finish (req,
+                                       DBUS_TYPE_INT32, &integer,
+                                       DBUS_TYPE_ARRAY, DBUS_TYPE_STRING, &array, &count,
+                                       DBUS_TYPE_BOOLEAN, &boolean,
+                                       DBUS_TYPE_INVALID)) {
+        return EOK; /* handled */
+    }
+
+    ck_assert_int_eq(integer, 5);
+    ck_assert(boolean == TRUE);
+    ck_assert_int_eq(count, 3);
+    ck_assert_str_eq(array[0], "one");
+    ck_assert_str_eq(array[1], "two");
+    ck_assert_str_eq(array[2], "three");
+
+    return sbus_request_return_and_finish(req, DBUS_TYPE_INVALID);
+}
+
 struct pilot_vtable pilot_impl = {
     { &pilot_meta, 0 },
     .Blink = blink_handler,
+    .Eat = eat_handler,
 };
 
 static int pilot_test_server_init(struct sbus_connection *server, void *unused)
@@ -179,11 +213,76 @@ START_TEST(test_raw_handler)
 }
 END_TEST
 
+START_TEST(test_request_parse_ok)
+{
+    const char *args[] = { "one", "two", "three" };
+    const char **array;
+    TALLOC_CTX *ctx;
+    DBusConnection *client;
+    DBusError error = DBUS_ERROR_INIT;
+    DBusMessage *reply;
+    dbus_bool_t boolean;
+    dbus_int32_t integer;
+    int count;
+
+    ctx = talloc_new(NULL);
+    client = test_dbus_setup_mock(ctx, NULL, pilot_test_server_init, NULL);
+
+    boolean = TRUE;
+    integer = 5;
+    count = 3;
+    array = args;
+    reply = test_dbus_call_sync(client,
+                                "/test/leela",
+                                PILOT_IFACE,
+                                PILOT_EAT,
+                                &error,
+                                DBUS_TYPE_INT32, &integer,
+                                DBUS_TYPE_ARRAY, DBUS_TYPE_STRING, &array, count,
+                                DBUS_TYPE_BOOLEAN, &boolean,
+                                DBUS_TYPE_INVALID);
+    ck_assert(reply != NULL);
+    ck_assert(!dbus_error_is_set(&error));
+    ck_assert(dbus_message_get_args(reply, NULL,
+                                    DBUS_TYPE_INVALID));
+    dbus_message_unref (reply);
+
+    talloc_free(ctx);
+}
+END_TEST
+
+START_TEST(test_request_parse_bad_args)
+{
+    TALLOC_CTX *ctx;
+    DBusConnection *client;
+    DBusError error = DBUS_ERROR_INIT;
+    DBusMessage *reply;
+
+    ctx = talloc_new(NULL);
+    client = test_dbus_setup_mock(ctx, NULL, pilot_test_server_init, NULL);
+
+    reply = test_dbus_call_sync(client,
+                                "/test/leela",
+                                PILOT_IFACE,
+                                PILOT_EAT,
+                                &error,
+                                DBUS_TYPE_INVALID); /* bad agruments */
+    ck_assert(reply == NULL);
+    ck_assert(dbus_error_is_set(&error));
+    ck_assert(dbus_error_has_name(&error, DBUS_ERROR_INVALID_ARGS));
+    dbus_error_free(&error);
+
+    talloc_free(ctx);
+}
+END_TEST
+
 TCase *create_sbus_tests(void)
 {
     TCase *tc = tcase_create("tests");
 
     tcase_add_test(tc, test_raw_handler);
+    tcase_add_test(tc, test_request_parse_ok);
+    tcase_add_test(tc, test_request_parse_bad_args);
 
     return tc;
 }
-- 
1.8.5.3

