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.
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.
+ }
+ 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 cd9f7ed48017516d01c79d59af6af63d87a4a31f 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
---
src/tests/sysdb-tests.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 97 insertions(+)
diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
index ac78f368f04b5138eecb6b121b43ac45bc3c32a3..650219d266ca052d941343321608fadf3c76872d
100644
--- a/src/tests/sysdb-tests.c
+++ b/src/tests/sysdb-tests.c
@@ -3979,6 +3979,87 @@ START_TEST(test_odd_characters)
}
END_TEST
+START_TEST(test_sss_ldb_search)
+{
+ errno_t ret;
+ struct sysdb_test_ctx *test_ctx;
+ struct ldb_dn *group_dn, *nonexist_dn;
+ struct ldb_result *res;
+ const char groupname[] = "test_group";
+ const char *received_group;
+
+ /* Setup */
+ ret = setup_sysdb_tests(&test_ctx);
+ if (ret != EOK) {
+ fail("Could not set up the test");
+ return;
+ }
+
+ check_leaks_push(test_ctx);
+
+ group_dn = sysdb_group_dn(test_ctx, test_ctx->domain, groupname);
+ fail_if(group_dn == NULL, "sysdb_group_dn failed");
+
+ nonexist_dn = sysdb_group_dn(test_ctx, test_ctx->domain,
+ "non-existing-group");
+ fail_if(nonexist_dn == NULL, "sysdb_group_dn failed");
+
+ /* Add */
+ ret = sysdb_add_incomplete_group(test_ctx->domain,
+ groupname, 20000, NULL, NULL, true, 0);
+ fail_unless(ret == EOK, "sysdb_add_incomplete_group error [%d][%s]",
+ ret, strerror(ret));
+
+ /* Retrieve */
+
+ /* Empty filter */
+ ret = sss_ldb_search(test_ctx->sysdb->ldb, test_ctx, &res, group_dn,
+ LDB_SCOPE_BASE, NULL, NULL);
+
+ fail_unless(ret == EOK, "sss_ldb_search error [%d][%s]",
+ ret, strerror(ret));
+
+ fail_unless(res->count == 1, "Received [%d] responses",
+ res->count);
+
+ received_group = ldb_msg_find_attr_as_string(res->msgs[0], SYSDB_NAME,
+ NULL);
+ fail_unless(strcmp(received_group, groupname) == 0,
+ "Expected [%s], got [%s]", groupname, received_group);
+
+ talloc_zfree(res);
+
+ /* Non-empty filter */
+ ret = sss_ldb_search(test_ctx->sysdb->ldb, test_ctx, &res, group_dn,
+ LDB_SCOPE_BASE, NULL, "objectClass=group");
+
+ fail_unless(ret == EOK, "sss_ldb_search error [%d][%s]",
+ ret, strerror(ret));
+ talloc_zfree(res);
+
+ /* Filter yeilding no results */
+ ret = sss_ldb_search(test_ctx->sysdb->ldb, test_ctx, &res, group_dn,
+ LDB_SCOPE_BASE, NULL,
+ "objectClass=nonExistingObjectClass");
+
+ fail_unless(ret == ENOENT, "sss_ldb_search error [%d][%s]",
+ ret, strerror(ret));
+ talloc_zfree(res);
+
+ /* Non-existing dn */
+ ret = sss_ldb_search(test_ctx->sysdb->ldb, test_ctx, &res, nonexist_dn,
+ LDB_SCOPE_BASE, NULL, NULL);
+
+ fail_unless(ret == ENOENT, "sss_ldb_search error [%d][%s]",
+ ret, strerror(ret));
+ talloc_zfree(res);
+
+ talloc_zfree(nonexist_dn);
+ talloc_zfree(group_dn);
+ fail_unless(check_leaks_pop(test_ctx) == true, "Memory leak");
+}
+END_TEST
+
/* == SERVICE TESTS == */
void services_check_match(struct sysdb_test_ctx *test_ctx,
bool by_name,
@@ -4539,6 +4620,18 @@ START_TEST (test_sysdb_search_return_ENOENT)
talloc_zfree(msgs);
talloc_zfree(user_dn);
+ /* sss_ldb_search */
+ user_dn = sysdb_user_dn(test_ctx, test_ctx->domain,
"nonexisting_user");
+ fail_if(user_dn == NULL, "sysdb_user_dn failed");
+ ret = sss_ldb_search(test_ctx->sysdb->ldb, test_ctx, &res, user_dn,
+ LDB_SCOPE_BASE, NULL, "objectClass=user");
+
+ fail_unless(ret == ENOENT, "sss_ldb_search failed: %d, %s",
+ ret, strerror(ret));
+
+ talloc_zfree(res);
+ talloc_zfree(user_dn);
+
/* TODO: test sysdb_search_selinux_config */
fail_unless(check_leaks_pop(test_ctx) == true, "Memory leak");
@@ -5786,6 +5879,10 @@ Suite *create_sysdb_suite(void)
MBO_GROUP_BASE , MBO_GROUP_BASE + 10);
tcase_add_loop_test(tc_memberof, test_sysdb_memberof_check_nested_double_ghosts,
MBO_GROUP_BASE , MBO_GROUP_BASE + 10);
+
+ /* sss_ldb_search */
+ tcase_add_test(tc_sysdb, test_sss_ldb_search);
+
/* This loop counts backwards so the indexing is a little odd */
tcase_add_loop_test(tc_memberof, test_sysdb_memberof_mod_replace_keep,
1 , 11);
--
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.
+ }
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 :-)
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 {
...
}
Or switch/case/default.
LS