[SSSD] [PATCH] SYSDB: sysdb_search_object_by_sid returns ENOENT

Lukas Slebodnik lslebodn at redhat.com
Wed Dec 10 08:28:25 UTC 2014


On (09/12/14 12:13), Pavel Reichl wrote:
>
>On 12/02/2014 12:50 PM, Lukas Slebodnik wrote:
>>On (29/10/14 17:17), Pavel Reichl wrote:
>>>Hello,
>>>
>>>please see attached patch.
>>>
>>>This patch is part of solution for
>>>https://fedorahosted.org/sssd/ticket/1991
>>>which aims to unify return values of sysdb calls in case no results are
>>>found.
>>>
>>a) this patch cannot be applied on current master.
>rebased
>>b) I wrote similar patch becuse sssd_be crashed in function
>>     get_object_from_cache. sysdb_search_object_by_sid didn' returned ENOENT and
>>     res->msgs was NULL (result: dereference of NULL pointer)
>>     -- it is yet another result of broken IPA <-> AD trust
>>c) Could you compare your version and mine?
>>     Feel free to include my patch to yours if it is not good enogh.
>I don't say it was not good enough but I updated my patch.
>>
>>d) test test_sysdb_delete_by_sid seems to be unrelated to this patch.
>>    I would prefer to have it in separete patch.
>I have moved it to separate patch. This patch is just to verify that
>sysdb_delete_by_sid() was not changed by the second patch.
>>
>>LS
>>
>Thanks, please see updated patches.

>From 53c3be375eff41dc70e17bb9e079ec98b8c2c5e7 Mon Sep 17 00:00:00 2001
>From: Pavel Reichl <preichl at redhat.com>
>Date: Tue, 9 Dec 2014 09:47:11 +0000
>Subject: [PATCH 1/2] TESTS: sysdb_delete_by_sid() test return value
>
>Check that return value of sysdb_delete_by_sid() is not changed as
>called SYSDB functions have changed the return value.
>
>Part of patches for:
>https://fedorahosted.org/sssd/ticket/1991
>---
LGTM

>From 1cb395244d3ab3906af5b6c8b294e80055ad0a07 Mon Sep 17 00:00:00 2001
>From: Pavel Reichl <preichl at redhat.com>
>Date: Tue, 9 Dec 2014 11:01:13 +0000
>Subject: [PATCH 2/2] SYSDB: sysdb_search_object_by_sid returns ENOENT
>
>sysdb_search_object_by_sid returns ENOENT if no results are found.
>
>Part od solution for:
>https://fedorahosted.org/sssd/ticket/1991
>---
> src/db/sysdb.h                 |  2 +-
> src/db/sysdb_ops.c             | 70 ++++++------------------------------------
> src/responder/nss/nsssrv_cmd.c | 27 ++++++++--------
> src/responder/pac/pacsrv_cmd.c | 26 ++++++++++------
> src/tests/sysdb-tests.c        |  5 +--
> 5 files changed, 42 insertions(+), 88 deletions(-)
>
>diff --git a/src/db/sysdb.h b/src/db/sysdb.h
>index 5bd7f90acb685bbaff5c98f433c7dce8175c33ca..9b88b4a63619456f8f3cc1961e29bbf3946ca5b8 100644
>--- a/src/db/sysdb.h
>+++ b/src/db/sysdb.h
>@@ -1033,7 +1033,7 @@ errno_t sysdb_search_object_by_sid(TALLOC_CTX *mem_ctx,
>                                    struct sss_domain_info *domain,
>                                    const char *sid_str,
>                                    const char **attrs,
>-                                   struct ldb_result **msg);
>+                                   struct ldb_result **res);
> 
> errno_t sysdb_search_object_by_uuid(TALLOC_CTX *mem_ctx,
>                                     struct sss_domain_info *domain,
>diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
>index 998046a2ca1c746b2032f430e5f9c4a7151e1dbc..8d6b11c248fd7f895ff6a95c25d4372fc7f8445d 100644
>--- a/src/db/sysdb_ops.c
>+++ b/src/db/sysdb_ops.c
>@@ -2995,8 +2995,15 @@ int sysdb_delete_by_sid(struct sysdb_ctx *sysdb,
> 
>     ret = sysdb_search_object_by_sid(tmp_ctx, domain, sid_str, NULL, &res);
>     if (ret != EOK) {
>-        DEBUG(SSSDBG_OP_FAILURE, "search by sid failed: %d (%s)\n",
>-              ret, strerror(ret));
>+        if (ret == ENOENT) {
>+            /* No existing entry. Just quit. */
>+            DEBUG(SSSDBG_TRACE_FUNC,
>+                  "search by sid did not return any results.\n");
>+            ret = EOK;
>+        } else {
>+            DEBUG(SSSDBG_OP_FAILURE, "search by sid failed: %d (%s)\n",
>+                  ret, strerror(ret));
>+        }
>         goto done;
I'm sorry but this isn't very readable.
I prefer flat version.
If/else if/else is easier to read and if you want to do micro optimisation
then you can use switch case.


>     }
> 
>@@ -3007,12 +3014,6 @@ int sysdb_delete_by_sid(struct sysdb_ctx *sysdb,
>         goto done;
>     }
> 
>-    if (res->count == 0) {
>-        /* No existing entry. Just quit. */
>-        ret = EOK;
>-        goto done;
>-    }
>-
>     ret = sysdb_delete_entry(sysdb, res->msgs[0]->dn, false);
>     if (ret != EOK) {
>         goto done;

> errno_t sysdb_search_object_by_uuid(TALLOC_CTX *mem_ctx,
>diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
>index ea58920bc3611c84958e8d0aca0e122d90c68e5c..d81737b6182bf283f61655c59dcffa135a1244d7 100644
>--- a/src/responder/nss/nsssrv_cmd.c
>+++ b/src/responder/nss/nsssrv_cmd.c
>@@ -4492,6 +4492,20 @@ static errno_t nss_cmd_getbysid_search(struct nss_dom_ctx *dctx)
>     ret = sysdb_search_object_by_sid(cmdctx, dom, cmdctx->secid, NULL,
>                                      &dctx->res);
>     if (ret != EOK) {
>+        if (ret == ENOENT) {
>+            if (!dctx->check_provider) {
>+                DEBUG(SSSDBG_OP_FAILURE, "No results for getbysid call.\n");
>+
>+                /* set negative cache only if not result of cache check */
>+                ret = sss_ncache_set_sid(nctx->ncache, false, cmdctx->secid);
>+                if (ret != EOK) {
>+                    DEBUG(SSSDBG_MINOR_FAILURE,
>+                          "Cannot set negative cache for %s\n", cmdctx->secid);
>+                }
>+            }
BTW you override error from sss_ncache_set_sid on next line.

>+            return ENOENT;
>+        }
>+
Please use flat version of if/else instead of nested one. (or switch case)


>diff --git a/src/responder/pac/pacsrv_cmd.c b/src/responder/pac/pacsrv_cmd.c
>index cc92592893899b1fa269188facc1a8154f80991d..5cedea14747b578dd7896d4d9a80c2e81fc04c17 100644
>--- a/src/responder/pac/pacsrv_cmd.c
>+++ b/src/responder/pac/pacsrv_cmd.c
>@@ -298,16 +298,17 @@ static void pac_lookup_sids_done(struct tevent_req *req)
>             ret = sysdb_search_object_by_sid(pr_ctx, dom, entries[c].key.str,
>                                              NULL, &msg);
>             if (ret != EOK) {
>-                DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_object_by_sid " \
>-                                          "failed.\n");
>+                if (ret == ENOENT) {
>+                    DEBUG(SSSDBG_OP_FAILURE, "No entry found for SID [%s].\n",
>+                          entries[c].key.str);
>+                } else {
>+                    DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_object_by_sid " \
                                                                            ^^
                                                  wrapping new line is useless
>+                          "failed.\n");
>+                }
>                 continue;
>             }
Please use flat version of if/else instead of nested one. (or switch case)

> 
>-            if (msg->count == 0) {
>-                DEBUG(SSSDBG_OP_FAILURE, "No entry found for SID [%s].\n",
>-                                          entries[c].key.str);
>-                continue;
>-            } else if (msg->count > 1) {
>+            if (msg->count > 1) {
>                 DEBUG(SSSDBG_CRIT_FAILURE, "More then one result returned " \
>                                             "for SID [%s].\n",
>                                             entries[c].key.str);
>@@ -912,9 +913,14 @@ pac_store_membership(struct pac_req_ctx *pr_ctx,
>     ret = sysdb_search_object_by_sid(tmp_ctx, grp_dom, grp_sid_str,
>                                      group_attrs, &group);
>     if (ret != EOK) {
>-        DEBUG(SSSDBG_TRACE_INTERNAL, "sysdb_search_object_by_sid " \
>-                                      "for SID [%s] failed [%d][%s].\n",
>-                                      grp_sid_str, ret, strerror(ret));
>+        if (ret == ENOENT) {
>+            DEBUG(SSSDBG_OP_FAILURE, "Unexpected number of groups returned.\n");
>+            ret = EINVAL;
>+        } else {
>+            DEBUG(SSSDBG_TRACE_INTERNAL, "sysdb_search_object_by_sid "  \
                                                                         ^^
                                                  wrapping new line is useless
>+                  "for SID [%s] failed [%d][%s].\n",
>+                  grp_sid_str, ret, strerror(ret));
>+        }
>         goto done;
>     }
Please use flat version of if/else instead of nested one. (or switch case)

> 
>diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
>index d303982647547eefdce7c37ac5b70e1ffbe869ce..92b41e90d08c2d50775289ba8c922f39350ce625 100644
>--- a/src/tests/sysdb-tests.c
>+++ b/src/tests/sysdb-tests.c
>@@ -4861,13 +4861,10 @@ START_TEST (test_sysdb_search_return_ENOENT)
>     talloc_zfree(res);
> 
>     /* Search object */
>-    /* TODO: Should return ENOENT */
>     ret = sysdb_search_object_by_sid(test_ctx, test_ctx->domain,
>                                      "S-5-4-3-2-1", NULL, &res);
>-    fail_unless(ret == EOK, "sysdb_search_object_by_sid_str failed with "
>+    fail_unless(ret == ENOENT, "sysdb_search_object_by_sid_str failed with "
>                              "[%d][%s].", ret, strerror(ret));
                               ^^
                  You touched code around. So you can fix indentation as well.

LS



More information about the sssd-devel mailing list