[SSSD] [sssd][patch] SYSDB: sysdb_getnetgr returns ENOENT

Jakub Hrozek jhrozek at redhat.com
Wed Jun 25 14:07:22 UTC 2014


On Wed, Jun 25, 2014 at 03:45:01PM +0200, Lukas Slebodnik wrote:
> On (24/06/14 17:28), Pavel Reichl wrote:
> >On Tue, 2014-06-24 at 16:18 +0200, Lukas Slebodnik wrote:
> >> On (24/06/14 13:33), Pavel Reichl wrote:
> >> >On Mon, 2014-06-23 at 13:50 +0200, Jakub Hrozek wrote:
> >> >> On Thu, Jun 19, 2014 at 05:53:37PM +0200, Pavel Reichl wrote:
> >> >> > From 6e2af6d696fd0e731467c50dbb715756a4bcb0a3 Mon Sep 17 00:00:00 2001
> >> >> > From: Pavel Reichl <preichl at redhat.com>
> >> >> > Date: Tue, 27 May 2014 19:11:20 +0100
> >> >> > Subject: [PATCH 1/4] SYSDB: sss_ldb_search - wrapper around ldb_search
> >> >> > 
> >> >> > Make sure that if no results were found ENOENT is returned rather than just
> >> >> > empty list of results.
> >> >> > 
> >> >> > Resolves:
> >> >> > https://fedorahosted.org/sssd/ticket/1991
> >> >> > ---
> >> >> >  src/db/sysdb.h     |  5 +++++
> >> >> >  src/db/sysdb_ops.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> >> >> >  2 files changed, 52 insertions(+)
> >> >> > 
> >> >> > diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> >> >> > index dd14190bfa156c6b2842e2af81d7b58efa3ea06f..634df049421586959531d8d1a365a2d50d15d1a6 100644
> >> >> > --- a/src/db/sysdb.h
> >> >> > +++ b/src/db/sysdb.h
> >> >> > @@ -480,6 +480,11 @@ int sysdb_get_netgroup_attr(TALLOC_CTX *mem_ctx,
> >> >> >                              const char **attributes,
> >> >> >                              struct ldb_result **res);
> >> >> >  
> >> >> > +errno_t sss_ldb_search(struct ldb_context *ldb, TALLOC_CTX *mem_ctx,
> >> >> > +                       struct ldb_result **_result, struct ldb_dn *base,
> >> >> > +                       enum ldb_scope scope, const char * const *attrs,
> >> >> > +                       const char *exp_fmt, ...);
> >> >> 
> >> >> You need to use SSS_ATTRIBUTE_PRINTF here.
> >> >
> >> >OK, but it didn't like "" being passed as fmt causing:
> >> >-Werror=format-zero-length. 
> >> >
> >> >To avoid this warning, NULL is passed instead of "". sss_ldb_search now
> >> >handles explicitly case when fmt is NULL because no filter processing is
> >> >required then.
> >> >
> >> And null caused warning in static analysers :-)
> >> 
> >> Error: PW.NON_CONST_PRINTF_FORMAT_STRING:
> >> sssd-1.11.92/src/db/sysdb_ops.c:109: non_const_printf_format_string: format string is not a string literal, potential security vulnerability if user controlled
> >> sssd-1.11.92/src/db/sysdb_ops.c:109: caretline:
> >> 
> >> I prefer to avoid compiler warnings. It will be better to use NULL.
> >
> >(lret = ldb_search(ldb, mem_ctx, _result, base, scope, attrs, NULL);)
> >
> >It's a common practice in SSSD anyway.
> >
> >> 
> >> 
> >> 
> >> >From: Pavel Reichl <preichl at redhat.com>
> >> >Date: Tue, 27 May 2014 19:11:20 +0100
> >> >Subject: [PATCH 1/5] SYSDB: sss_ldb_search - wrapper around ldb_search
> >> >
> >> >Make sure that if no results were found ENOENT is returned rather than just
> >> >empty list of results.
> >> >
> >> >Resolves:
> >> >https://fedorahosted.org/sssd/ticket/1991
> >> >---
> >> > src/db/sysdb.h     |  5 +++++
> >> > src/db/sysdb_ops.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >> > 2 files changed, 55 insertions(+)
> >> >
> >> >diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> >> >index 911430eefcbf520ca04f123a3b6f1882dc25876b..17cd5110c9bdafd8d7b18e621188523ed41e5c8a 100644
> >> >--- a/src/db/sysdb.h
> >> >+++ b/src/db/sysdb.h
> >> >@@ -481,6 +481,11 @@ int sysdb_get_netgroup_attr(TALLOC_CTX *mem_ctx,
> >> >                             const char **attributes,
> >> >                             struct ldb_result **res);
> >> > 
> >> >+errno_t sss_ldb_search(struct ldb_context *ldb, TALLOC_CTX *mem_ctx,
> >> >+                       struct ldb_result **_result, struct ldb_dn *base,
> >> >+                       enum ldb_scope scope, const char * const *attrs,
> >> >+                       const char *exp_fmt, ...) SSS_ATTRIBUTE_PRINTF(7, 8);
> >> >+
> >> > /* functions that modify the databse
> >> >  * they have to be called within a transaction
> >> >  * See sysdb_transaction_send()/_recv() */
> >> >diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
> >> >index 4d31cb6a0e83370a2c49a666f9474d93baf430b0..25fccc5fdacd85416268e02632826fd87174e7f7 100644
> >> >--- a/src/db/sysdb_ops.c
> >> >+++ b/src/db/sysdb_ops.c
> >> >@@ -74,6 +74,56 @@ static uint32_t get_attr_as_uint32(struct ldb_message *msg, const char *attr)
> >> >     return l;
> >> > }
> >> > 
> >> >+
> >> >+/* Wrapper around ldb_search to ensure that if zero results are found then
> >> >+ * ENOENT is returned
> >> >+ */
> >> >+errno_t sss_ldb_search(struct ldb_context *ldb, TALLOC_CTX *mem_ctx,
> >> >+                       struct ldb_result **_result, struct ldb_dn *base,
> >> >+                       enum ldb_scope scope, const char * const *attrs,
> >> >+                       const char *exp_fmt, ...)
> >> >+{
> >> >+    char *s;
> >> >+    int lret;
> >> >+    va_list ap;
> >> >+    errno_t ret;
> >> >+    TALLOC_CTX *tmp_ctx = NULL;
> >> >+
> >> >+    if (exp_fmt != NULL) {
> >> >+        tmp_ctx = talloc_new(NULL);
> >> >+        if (tmp_ctx == NULL) {
> >> >+            ret = ENOMEM;
> >> >+            goto done;
> >> >+        }
> >> >+
> >> >+        va_start(ap, exp_fmt);
> >> >+        s = talloc_vasprintf(tmp_ctx, exp_fmt, ap);
> >> >+        if (s == NULL) {
> >> >+            DEBUG(SSSDBG_MINOR_FAILURE, "Failed to process filter.\n");
> >> >+            ret = ENOMEM;
> >> >+            goto done;
> >> va_end is not called in this case.
> >> It will be better to move va_end before if condition.
> >Yep, I missed that sorry. Fixed in attached patch set.
> >
> Thank you.
> 
> >> 
> >> >+        }
> >> >+        va_end(ap);
> >> >+        lret = ldb_search(ldb, mem_ctx, _result, base, scope, attrs, "%s", s);
> >> >+    } else {
> >> >+        lret = ldb_search(ldb, mem_ctx, _result, base, scope, attrs, NULL);
> >> >+    }
> >> >+
> >> >+    ret = sysdb_error_to_errno(lret);
> >> >+    if (ret != EOK) {
> >> >+        goto done;
> >> >+    }
> >> >+
> >> >+    if ((*_result)->count == 0) {
> >> >+        ret = ENOENT;
> >> >+        goto done;
> >> >+    }
> >> >+
> >> >+done:
> >> >+    talloc_free(tmp_ctx);
> >> >+    return ret;
> >> >+}
> >> >+
> >> > /*
> >> >  * The wrapper around ldb_modify that uses LDB_CONTROL_PERMISSIVE_MODIFY_OID
> >> >  * so that on adds entries that already exist are skipped and similarly
> >> >-- 
> >> >1.8.4.2
> >> >
> >> 
> >> >From 7954ad8ee3310d7e7bc9ac12c3b764786c052a4d Mon Sep 17 00:00:00 2001
> >> >From: Pavel Reichl <preichl at redhat.com>
> >> >Date: Wed, 26 Feb 2014 16:52:26 +0000
> >> >Subject: [PATCH 3/5] SYSDB: sysdb_getnetgr returns ENOENT
> >> >
> >> >Replace call of ldb_search by sss_ldb_search to make sure that ENOENT is
> >> >returned if no results were found.
> >> >
> >> >Resolves:
> >> >https://fedorahosted.org/sssd/ticket/1991
> >> >---
> >> > src/db/sysdb_search.c | 20 ++++++++------------
> >> > 1 file changed, 8 insertions(+), 12 deletions(-)
> >> >
> >> >diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c
> >> >index 60ad61368d92d12bca3833bc08b0673c581ab882..608277ab34c3280659cb2cf04ea57721bd9790f5 100644
> >> >--- a/src/db/sysdb_search.c
> >> >+++ b/src/db/sysdb_search.c
> >> >@@ -786,7 +786,6 @@ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx,
> >> >     char *sanitized_netgroup;
> >> >     char *lc_sanitized_netgroup;
> >> >     char *netgroup_dn;
> >> >-    int lret;
> >> >     errno_t ret;
> >> > 
> >> >     tmp_ctx = talloc_new(NULL);
> >> >@@ -816,18 +815,15 @@ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx,
> >> >         goto done;
> >> >     }
> >> > 
> >> >-    lret = ldb_search(domain->sysdb->ldb, tmp_ctx, &result, base_dn,
> >> >-                     LDB_SCOPE_SUBTREE, attrs,
> >> >-                     SYSDB_NETGR_TRIPLES_FILTER, lc_sanitized_netgroup,
> >> >-                     sanitized_netgroup, sanitized_netgroup,
> >> >-                     netgroup_dn);
> >> >-    ret = sysdb_error_to_errno(lret);
> >> >-    if (ret != EOK) {
> >> >-        goto done;
> >> >-    }
> >> >+    ret = sss_ldb_search(domain->sysdb->ldb, tmp_ctx, &result, base_dn,
> >> >+                         LDB_SCOPE_SUBTREE, attrs,
> >> >+                         SYSDB_NETGR_TRIPLES_FILTER, lc_sanitized_netgroup,
> >> >+                         sanitized_netgroup, sanitized_netgroup,
> >> >+                         netgroup_dn);
> >> > 
> >> >-    *res = talloc_steal(mem_ctx, result);
> >> >-    ret = EOK;
> >> >+    if (ret == EOK || ret == ENOENT) {
> >> >+        *res = talloc_steal(mem_ctx, result);
> >> There is a warning from coverity
> >> Error: UNINIT (CWE-457):
> >> sssd-1.11.92/src/db/sysdb_search.c:785: var_decl: Declaring variable "result" without initializer.
> >> sssd-1.11.92/src/db/sysdb_search.c:825: uninit_use_in_call: Using uninitialized value "result" when calling "_talloc_steal_loc".
> >> 
> >> I think it is false positive.
> >So do I. But I wonder if initiating result to NULL would make coverity
> >to be quiet. (done in attached patch).
> >
> Yes. it is.
> I hope we did not hide any real problem and it was false positive.
> 
> >>
> >> >+    }
> >> > 
> >> > done:
> >> >     talloc_zfree(tmp_ctx);
> >> >-- 
> >> >1.8.4.2
> >> >
> >> 
> >> >From 94a3501fa6d0f238c889e2e05858c0a42579815d Mon Sep 17 00:00:00 2001
> >> >From: Pavel Reichl <preichl at redhat.com>
> >> >Date: Thu, 19 Jun 2014 16:18:12 +0100
> >> >Subject: [PATCH 4/5] TESTS: sysdb_getnetgr - return ENOENT
> >> >
> >> >Resolves:
> >> >https://fedorahosted.org/sssd/ticket/1991
> >> >---
> >> BTW 3rd and 4th patch are small.
> >> 
> >> I am used to sending a fix and an unit test in single patch. Small benefit is
> >> that we don't forgot to backport a test. I don't have strong preferences.
> >> I just want to start discussion what should be prefered way
> >>     a) to send a fix and an unit test in two patches
> >>     b) to send a fix and an unit test in single patch
> >> (of course there can be exceptions :-)
> >I don't have strong preferences either as I'm not paid per commit so far.
> >
> >I agree with your statement about 'forgetting to back-port'. On the
> >other hand I believe that patch should do just one thing and one thing
> >only. Either to fix the bug or introduce a test.
> >
> >Direct advantage of separate patches is possibility to run test before
> >applying patch with bug fix and then to see if it really fixes the
> >issue.
> >
> We can leave these patches as are continue with discussion in different part of
> this thread.
> 
> >
> >
> >> >From 44c4d576c840c01bade8f7f7aa6c980793d79505 Mon Sep 17 00:00:00 2001
> >> >From: Pavel Reichl <preichl at redhat.com>
> >> >Date: Tue, 24 Jun 2014 09:47:09 +0100
> >> >Subject: [PATCH 5/5] NSS: lookup_netgr_step don't access result on ENOENT
> >> >
> >> >Don't access result if return value is not EOK.
> >> >---
> >> > src/responder/nss/nsssrv_netgroup.c | 45 +++++++++++++++++++------------------
> >> > 1 file changed, 23 insertions(+), 22 deletions(-)
> >> >
> >> >diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c
> >> >index 1c9b72ae30c6ef1115d1ed972a12dc554db04bbe..d60f72a743157066dffca86493dfebf40bb451f6 100644
> >> >--- a/src/responder/nss/nsssrv_netgroup.c
> >> >+++ b/src/responder/nss/nsssrv_netgroup.c
> >> >@@ -514,30 +514,31 @@ static errno_t lookup_netgr_step(struct setent_step_ctx *step_ctx)
> >> > 
> >> >         /* Look up the netgroup in the cache */
> >> >         ret = sysdb_getnetgr(step_ctx->dctx, dom, name, &step_ctx->dctx->res);
> >> >-        if (ret == ENOENT) {
> >> >-            /* This netgroup was not found in this domain */
> >> >-            if (!step_ctx->dctx->check_provider) {
> >> >-                if (step_ctx->check_next) {
> >> >-                    dom = get_next_domain(dom, false);
> >> >-                    continue;
> >> >+        if (ret == EOK) {
> >> >+            if (step_ctx->dctx->res->count > 1) {
> >> >+                DEBUG(SSSDBG_FATAL_FAILURE,
> >> >+                      "getnetgr call returned more than one result !?!\n");
> >> >+                ret = EMSGSIZE;
> >> >+                goto done;
> >> >+            }
> >> >+        } else {
> >> >+            if (ret == ENOENT) {
> >> >+                /* This netgroup was not found in this domain */
> >> >+                if (!step_ctx->dctx->check_provider) {
> >> >+                    if (step_ctx->check_next) {
> >> >+                        dom = get_next_domain(dom, false);
> >> >+                        continue;
> >> >+                    } else {
> >> >+                        break;
> >> >+                    }
> >> >                 }
> >> >-                else break;
> >> >+                ret = EOK;
> >>                   warning: Value stored to 'ret' is never read
> >> I think conditionc could be more flatten
> >> if (ret == EOK) {
> >> ...
> >> } else if (ret == ENOENT) {
> >> ...
> >> } else {
> >> ...
> >> }
> >OK.
> >
> It loks better now.
> 
> >From 78eb31fd7be6ef8b2485f7058a31557a0a1758b6 Mon Sep 17 00:00:00 2001
> >From: Pavel Reichl <preichl at redhat.com>
> >Date: Tue, 27 May 2014 19:11:20 +0100
> >Subject: [PATCH 1/5] SYSDB: sss_ldb_search - wrapper around ldb_search
> >
> >Make sure that if no results were found ENOENT is returned rather than just
> >empty list of results.
> >
> >Resolves:
> >https://fedorahosted.org/sssd/ticket/1991
> >---
> ACK
> 
> >From e1b6545759bef0c6e97831c969a331c3fff54c1d Mon Sep 17 00:00:00 2001
> >From: Pavel Reichl <preichl at redhat.com>
> >Date: Thu, 19 Jun 2014 15:38:59 +0100
> >Subject: [PATCH 2/5] TESTS: add tests for sss_ldb_search
> >
> >Resolves:
> >https://fedorahosted.org/sssd/ticket/1991
> >---
> ACK
> 
> >From 11fcecadf767a3ecb24492d314274860d8fefba2 Mon Sep 17 00:00:00 2001
> >From: Pavel Reichl <preichl at redhat.com>
> >Date: Wed, 26 Feb 2014 16:52:26 +0000
> >Subject: [PATCH 3/5] SYSDB: sysdb_getnetgr returns ENOENT
> >
> >Replace call of ldb_search by sss_ldb_search to make sure that ENOENT is
> >returned if no results were found.
> >
> >Resolves:
> >https://fedorahosted.org/sssd/ticket/1991
> >---
> ACK
> 
> >-    lret = ldb_search(domain->sysdb->ldb, tmp_ctx, &result, base_dn,
> >-                     LDB_SCOPE_SUBTREE, attrs,
> >-                     SYSDB_NETGR_TRIPLES_FILTER, lc_sanitized_netgroup,
> >-                     sanitized_netgroup, sanitized_netgroup,
> >-                     netgroup_dn);
> >-    ret = sysdb_error_to_errno(lret);
> >-    if (ret != EOK) {
> >-        goto done;
> >-    }
> >+    ret = sss_ldb_search(domain->sysdb->ldb, tmp_ctx, &result, base_dn,
> >+                         LDB_SCOPE_SUBTREE, attrs,
> >+                         SYSDB_NETGR_TRIPLES_FILTER, lc_sanitized_netgroup,
> >+                         sanitized_netgroup, sanitized_netgroup,
> >+                         netgroup_dn);
> > 
> >-    *res = talloc_steal(mem_ctx, result);
> >-    ret = EOK;
> >+    if (ret == EOK || ret == ENOENT) {
> >+        *res = talloc_steal(mem_ctx, result);
> Please file a ticket. It should be blocked by #1991.
> We should not use result in case of ret != 0. ENOENT should not be exception.
> 
> >From 79bf514e4d528d4c41d82c4e0398db4244350262 Mon Sep 17 00:00:00 2001
> >From: Pavel Reichl <preichl at redhat.com>
> >Date: Thu, 19 Jun 2014 16:18:12 +0100
> >Subject: [PATCH 4/5] TESTS: sysdb_getnetgr - return ENOENT
> >
> >Resolves:
> >https://fedorahosted.org/sssd/ticket/1991
> >---
> ACK
> 
> >From eb08103aae3f81e11e60bd9843627043c6427a60 Mon Sep 17 00:00:00 2001
> >From: Pavel Reichl <preichl at redhat.com>
> >Date: Tue, 24 Jun 2014 09:47:09 +0100
> >Subject: [PATCH 5/5] NSS: lookup_netgr_step don't access result on ENOENT
> >
> >Don't access result if return value is not EOK.
> >---
> ACK
> 
> LS

All patches were acked, pushed to master:
    8ca1875209325ddeb28fccb421457b76b4b0c7db
    50c009a3d51521880ddf568e7173d1ed9d5c2685
    9436c86caf9b2f7ec33c3022dfed2a653d3ec965
    940dd08102ac8527ef9e367dc5d6fb88cd53a8a2
    5153e8b9793dea1e212ca08af0f77ea1d023cbb7



More information about the sssd-devel mailing list