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
All patches were acked, pushed to master:
8ca1875209325ddeb28fccb421457b76b4b0c7db
50c009a3d51521880ddf568e7173d1ed9d5c2685
9436c86caf9b2f7ec33c3022dfed2a653d3ec965
940dd08102ac8527ef9e367dc5d6fb88cd53a8a2
5153e8b9793dea1e212ca08af0f77ea1d023cbb7