>From 966205cdcd3286128f2693399c0d31ad0ea078d2 Mon Sep 17 00:00:00 2001 From: Stef Walter Date: Fri, 21 Feb 2014 16:59:48 +0100 Subject: [PATCH 5/5] sbus: Add the sbus_request_parse_or_finish() method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Reviewed-by: Jakub Hrozek Reviewed-by: Pavel Březina Reviewed-by: Lukáš Slebodník (cherry picked from commit 06b7bc8ca2e005ed510210d3b8dee16afbabbcc9) --- 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 3c2dda230dee02f2bfa44e7d96eb63d8c5d308b4..ac703b821d1f1bb84542743e5ac03ee0a7058f70 100644 --- a/src/providers/data_provider_be.c +++ b/src/providers/data_provider_be.c @@ -558,7 +558,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; @@ -569,17 +568,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) { @@ -1036,7 +1029,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; @@ -1051,19 +1043,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); @@ -1607,7 +1593,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; @@ -1627,17 +1612,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 @@ -1818,7 +1797,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; @@ -1831,17 +1809,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 0206ec15b670e1763102ee2d99e18918a56c56e7..dd1b75826fbfc384dd37ba659a8653547cc35bcf 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 5e62377229de49034c5d80e87495a5d6dedf3229..c6e7cb402d5bade600b36442d096d14d505b30a9 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 ef728bd30e6b52cb18bd5d68f0d4f4e759358eca..3bf70c8cceaeed45d57dad802d196c931d7f030d 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 973089c4bbc5d766edfe5792a366d0548c3c3855..c45f3a9fdb7204a2f296f1a0870c1275aa42e0d8 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 7290fe7dbf9d6b0811afbc3b27d3500cff63761a..b8cd4abb0d306d0b9dbf7896f0e1709c1dc7fd40 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.9.0