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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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(a)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