From 06308fbd29c2516b259ac65d326c4591284d59d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20=C4=8Cech?= Date: Thu, 23 Mar 2017 09:17:55 +0100 Subject: [PATCH 1/4] IFP: Filter with * in infopipe group methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch fixes asterisk in filter of the ListByName Groups' method, which ends up calling ifp_groups_list_copy() with a NULL pointer. Resolves: https://pagure.io/SSSD/sssd/issue/3305 Reviewed-by: Fabiano Fidêncio --- src/responder/ifp/ifp_groups.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/responder/ifp/ifp_groups.c b/src/responder/ifp/ifp_groups.c index c568c6200..5c126fc88 100644 --- a/src/responder/ifp/ifp_groups.c +++ b/src/responder/ifp/ifp_groups.c @@ -307,12 +307,14 @@ static void ifp_groups_list_by_name_done(struct tevent_req *req) return; } - ret = ifp_groups_list_copy(list_ctx, result->ldb_result); - if (ret != EOK) { - error = sbus_error_new(sbus_req, SBUS_ERROR_INTERNAL, - "Failed to copy domain result"); - sbus_request_fail_and_finish(sbus_req, error); - return; + if (ret == EOK) { + ret = ifp_groups_list_copy(list_ctx, result->ldb_result); + if (ret != EOK) { + error = sbus_error_new(sbus_req, SBUS_ERROR_INTERNAL, + "Failed to copy domain result"); + sbus_request_fail_and_finish(sbus_req, error); + return; + } } list_ctx->dom = get_next_domain(list_ctx->dom, SSS_GND_DESCEND); @@ -394,11 +396,13 @@ static void ifp_groups_list_by_domain_and_name_done(struct tevent_req *req) goto done; } - ret = ifp_groups_list_copy(list_ctx, result->ldb_result); - if (ret != EOK) { - error = sbus_error_new(sbus_req, SBUS_ERROR_INTERNAL, - "Failed to copy domain result"); - goto done; + if (ret == EOK) { + ret = ifp_groups_list_copy(list_ctx, result->ldb_result); + if (ret != EOK) { + error = sbus_error_new(sbus_req, SBUS_ERROR_INTERNAL, + "Failed to copy domain result"); + goto done; + } } done: From b3d565cdfa905d0030e876ec0b91fcc5f4b2d36d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20=C4=8Cech?= Date: Tue, 28 Mar 2017 12:07:55 +0200 Subject: [PATCH 2/4] IFP: Fix of limit = 0 (unlimited result) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we set limit to 0 it means that result is unlimited. Internally we restrict number of result by allocation of result array. In unlimited case there was a bug and zero array was allocated. This fix allocates neccessary array when we know real result size. Resolves: https://pagure.io/SSSD/sssd/issue/3306 Reviewed-by: Fabiano Fidêncio --- src/responder/ifp/ifpsrv_util.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/responder/ifp/ifpsrv_util.c b/src/responder/ifp/ifpsrv_util.c index 5866d30d8..c948d5ab5 100644 --- a/src/responder/ifp/ifpsrv_util.c +++ b/src/responder/ifp/ifpsrv_util.c @@ -314,6 +314,15 @@ size_t ifp_list_ctx_remaining_capacity(struct ifp_list_ctx *list_ctx, { size_t capacity = list_ctx->limit - list_ctx->path_count; + if (list_ctx->limit == 0) { + list_ctx->paths = talloc_zero_array(list_ctx, const char *, entries); + if (list_ctx->paths == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero_array() failed\n"); + return 0; + } + return entries; + } + if (capacity < entries) { DEBUG(SSSDBG_MINOR_FAILURE, "IFP list request has limit of %"PRIu32" entries but back end " From d07629e32162f3461244ef74f9925647d12f973d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= Date: Fri, 5 May 2017 10:38:41 +0200 Subject: [PATCH 3/4] IFP: Change ifp_list_ctx_remaining_capacity() return type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now ifp_list_ctx_remaining_capacity() returns an errno_t and receives the count as an output parameter. It allows better handling and error reporting in case something goes wrong internally in this function. Related: https://pagure.io/SSSD/sssd/issue/3306 Signed-off-by: Fabiano Fidêncio --- src/responder/ifp/ifp_groups.c | 14 +++++++++++--- src/responder/ifp/ifp_private.h | 5 +++-- src/responder/ifp/ifp_users.c | 21 +++++++++++++++++---- src/responder/ifp/ifpsrv_util.c | 22 ++++++++++++++++------ 4 files changed, 47 insertions(+), 15 deletions(-) diff --git a/src/responder/ifp/ifp_groups.c b/src/responder/ifp/ifp_groups.c index 5c126fc88..e9dcbe563 100644 --- a/src/responder/ifp/ifp_groups.c +++ b/src/responder/ifp/ifp_groups.c @@ -87,8 +87,12 @@ static int ifp_groups_list_copy(struct ifp_list_ctx *list_ctx, struct ldb_result *result) { size_t copy_count, i; + errno_t ret; - copy_count = ifp_list_ctx_remaining_capacity(list_ctx, result->count); + ret = ifp_list_ctx_remaining_capacity(list_ctx, result->count, ©_count); + if (ret != EOK) { + goto done; + } for (i = 0; i < copy_count; i++) { list_ctx->paths[list_ctx->path_count + i] = \ @@ -96,12 +100,16 @@ static int ifp_groups_list_copy(struct ifp_list_ctx *list_ctx, list_ctx->dom, result->msgs[i]); if (list_ctx->paths[list_ctx->path_count + i] == NULL) { - return ENOMEM; + ret = ENOMEM; + goto done; } } list_ctx->path_count += copy_count; - return EOK; + ret = EOK; + +done: + return ret; } static void ifp_groups_find_by_name_done(struct tevent_req *req); diff --git a/src/responder/ifp/ifp_private.h b/src/responder/ifp/ifp_private.h index e800070a5..d7e8b7516 100644 --- a/src/responder/ifp/ifp_private.h +++ b/src/responder/ifp/ifp_private.h @@ -99,8 +99,9 @@ struct ifp_list_ctx *ifp_list_ctx_new(struct sbus_request *sbus_req, const char *filter, uint32_t limit); -size_t ifp_list_ctx_remaining_capacity(struct ifp_list_ctx *list_ctx, - size_t entries); +errno_t ifp_list_ctx_remaining_capacity(struct ifp_list_ctx *list_ctx, + size_t entries, + size_t *_capacity); errno_t ifp_ldb_el_output_name(struct resp_ctx *rctx, struct ldb_message *msg, diff --git a/src/responder/ifp/ifp_users.c b/src/responder/ifp/ifp_users.c index 188194f2a..01590628f 100644 --- a/src/responder/ifp/ifp_users.c +++ b/src/responder/ifp/ifp_users.c @@ -436,8 +436,12 @@ static int ifp_users_list_copy(struct ifp_list_ctx *list_ctx, struct ldb_result *result) { size_t copy_count, i; + errno_t ret; - copy_count = ifp_list_ctx_remaining_capacity(list_ctx, result->count); + ret = ifp_list_ctx_remaining_capacity(list_ctx, result->count, ©_count); + if (ret != EOK) { + goto done; + } for (i = 0; i < copy_count; i++) { list_ctx->paths[list_ctx->path_count + i] = \ @@ -445,12 +449,16 @@ static int ifp_users_list_copy(struct ifp_list_ctx *list_ctx, list_ctx->dom, result->msgs[i]); if (list_ctx->paths[list_ctx->path_count + i] == NULL) { - return ENOMEM; + ret = ENOMEM; + goto done; } } list_ctx->path_count += copy_count; - return EOK; + ret = EOK; + +done: + return ret; } struct name_and_cert_ctx { @@ -906,7 +914,12 @@ static void ifp_users_list_by_domain_and_name_done(struct tevent_req *req) goto done; } - copy_count = ifp_list_ctx_remaining_capacity(list_ctx, result->count); + ret = ifp_list_ctx_remaining_capacity(list_ctx, result->count, ©_count); + if (ret != EOK) { + error = sbus_error_new(sbus_req, SBUS_ERROR_INTERNAL, + "Failed to get the list remaining capacity\n"); + goto done; + } for (i = 0; i < copy_count; i++) { list_ctx->paths[i] = ifp_users_build_path_from_msg(list_ctx->paths, diff --git a/src/responder/ifp/ifpsrv_util.c b/src/responder/ifp/ifpsrv_util.c index c948d5ab5..0a3bdc9bf 100644 --- a/src/responder/ifp/ifpsrv_util.c +++ b/src/responder/ifp/ifpsrv_util.c @@ -309,28 +309,38 @@ struct ifp_list_ctx *ifp_list_ctx_new(struct sbus_request *sbus_req, return list_ctx; } -size_t ifp_list_ctx_remaining_capacity(struct ifp_list_ctx *list_ctx, - size_t entries) +errno_t ifp_list_ctx_remaining_capacity(struct ifp_list_ctx *list_ctx, + size_t entries, + size_t *_capacity) { size_t capacity = list_ctx->limit - list_ctx->path_count; + errno_t ret; if (list_ctx->limit == 0) { list_ctx->paths = talloc_zero_array(list_ctx, const char *, entries); if (list_ctx->paths == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero_array() failed\n"); - return 0; + ret = ENOMEM; + goto done; } - return entries; + capacity = entries; + goto immediately; } if (capacity < entries) { DEBUG(SSSDBG_MINOR_FAILURE, "IFP list request has limit of %"PRIu32" entries but back end " "returned %zu entries\n", list_ctx->limit, entries); - return capacity; } else { - return entries; + capacity = entries; } + +immediately: + *_capacity = capacity; + ret = EOK; + +done: + return ret; } errno_t ifp_ldb_el_output_name(struct resp_ctx *rctx, From abf25b8406b60aedbb16211049b2ef47624f9989 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= Date: Tue, 9 May 2017 13:08:55 +0200 Subject: [PATCH 4/4] IFP: Don't pre-allocate the amount of entries requested MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit By allocating the number of entries when actually copying the list we can avoid situations where users request an enourmous amount of results but the number of results got from the backend are just a few. With this new approach we end up allocating the whole list more frequently but we avoid not returning valid results because the requested number of enties is too big (note that if the amount of results is too big as well, there's nothing much we can do). A simple reproducer for this issue can be the really extreme call: $ dbus-send --system --print-reply --dest=org.freedesktop.sssd.infopipe \ /org/freedesktop/sssd/infopipe/Users \ org.freedesktop.sssd.infopipe.Users.ListByName string:"*" uint32:"-1" The example pasted above would try to allocate an array of MAX_UINT32 size, which would fail directly. Signed-off-by: Fabiano Fidêncio --- src/responder/ifp/ifpsrv_util.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/responder/ifp/ifpsrv_util.c b/src/responder/ifp/ifpsrv_util.c index 0a3bdc9bf..d958de95d 100644 --- a/src/responder/ifp/ifpsrv_util.c +++ b/src/responder/ifp/ifpsrv_util.c @@ -300,7 +300,7 @@ struct ifp_list_ctx *ifp_list_ctx_new(struct sbus_request *sbus_req, list_ctx->ctx = ctx; list_ctx->dom = ctx->rctx->domains; list_ctx->filter = filter; - list_ctx->paths = talloc_zero_array(list_ctx, const char *, limit); + list_ctx->paths = talloc_zero_array(list_ctx, const char *, 1); if (list_ctx->paths == NULL) { talloc_free(list_ctx); return NULL; @@ -317,12 +317,6 @@ errno_t ifp_list_ctx_remaining_capacity(struct ifp_list_ctx *list_ctx, errno_t ret; if (list_ctx->limit == 0) { - list_ctx->paths = talloc_zero_array(list_ctx, const char *, entries); - if (list_ctx->paths == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero_array() failed\n"); - ret = ENOMEM; - goto done; - } capacity = entries; goto immediately; } @@ -336,6 +330,14 @@ errno_t ifp_list_ctx_remaining_capacity(struct ifp_list_ctx *list_ctx, } immediately: + talloc_zfree(list_ctx->paths); + list_ctx->paths = talloc_zero_array(list_ctx, const char *, capacity); + if (list_ctx->paths == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero_array() failed\n"); + ret = ENOMEM; + goto done; + } + *_capacity = capacity; ret = EOK;