Hello,
attached patch is the first of many to solve https://fedorahosted.org/sssd/ticket/1991
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think it would be best if in case that no results were found both ENOENT value and 'properly' empty list were returned.
Thank for opinions or/and review.
Pavel Reichl
On (17/04/14 18:44), Pavel Reichl wrote:
Hello,
attached patch is the first of many to solve https://fedorahosted.org/sssd/ticket/1991
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think it would be best if in case that no results were found both ENOENT value and 'properly' empty list were returned.
Thank for opinions or/and review.
Pavel Reichl
From 1e7f69536acfb50e221807b59446ed9fb584bab9 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 26 Feb 2014 16:52:26 +0000 Subject: [PATCH 1/5] SYSDB: sysdb_getnetgr returns ENOENT
sysdb_getnetgr did not return ENOENT althought it was expected.
Resolves: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 1 + src/db/sysdb_search.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 5d337ffd8c0f7fbe6808d50538192369102caf17..45d486c3fa32e393ddb0bfe38426f9c2a3611955 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -455,6 +455,7 @@ struct sysdb_netgroup_ctx { } value; };
+/* If no group is found ENOENT is returned and result count is set to 0 */ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *netgroup, diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c index 60ad61368d92d12bca3833bc08b0673c581ab882..97192115b90b985cb0ff42516cdc02a44a33acc9 100644 --- a/src/db/sysdb_search.c +++ b/src/db/sysdb_search.c @@ -827,7 +827,12 @@ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, }
*res = talloc_steal(mem_ctx, result);
- ret = EOK;
- if((*res)->count == 0) {
ret = ENOENT;- } else {
ret = EOK;- }
done: talloc_zfree(tmp_ctx); -- 1.8.4.2
http://jhrozek.fedorapeople.org/sssd-coverage/html/db/sysdb_search.c.gcov.ht... 80.0% of functions are covered.
Please do not decrease code coverage in sysdb. It is one of the most important parts of sssd.
LS
On Thu, Apr 17, 2014 at 07:25:09PM +0200, Lukas Slebodnik wrote:
On (17/04/14 18:44), Pavel Reichl wrote:
Hello,
attached patch is the first of many to solve https://fedorahosted.org/sssd/ticket/1991
Thanks for doing this work, there's been many bugs over the years as a result of inconsitent return codes from different sysdb functions.
I have one more request, though -- would you mind sending a short mail (without the [PATCH] tag) to this list explaining why you chose the convention you did? I know we talked about the reasons in person but not all developers are physically located in Brno :-)
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think it would be best if in case that no results were found both ENOENT value and 'properly' empty list were returned.
Thank for opinions or/and review.
Pavel Reichl
From 1e7f69536acfb50e221807b59446ed9fb584bab9 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 26 Feb 2014 16:52:26 +0000 Subject: [PATCH 1/5] SYSDB: sysdb_getnetgr returns ENOENT
sysdb_getnetgr did not return ENOENT althought it was expected.
Resolves: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 1 + src/db/sysdb_search.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 5d337ffd8c0f7fbe6808d50538192369102caf17..45d486c3fa32e393ddb0bfe38426f9c2a3611955 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -455,6 +455,7 @@ struct sysdb_netgroup_ctx { } value; };
+/* If no group is found ENOENT is returned and result count is set to 0 */ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *netgroup, diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c index 60ad61368d92d12bca3833bc08b0673c581ab882..97192115b90b985cb0ff42516cdc02a44a33acc9 100644 --- a/src/db/sysdb_search.c +++ b/src/db/sysdb_search.c @@ -827,7 +827,12 @@ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, }
*res = talloc_steal(mem_ctx, result);
- ret = EOK;
- if((*res)->count == 0) {
ret = ENOENT;
Style police: Seems like there are two spaces between "=" and "ENOENT".
- } else {
ret = EOK;- }
done: talloc_zfree(tmp_ctx); -- 1.8.4.2
http://jhrozek.fedorapeople.org/sssd-coverage/html/db/sysdb_search.c.gcov.ht... 80.0% of functions are covered.
Please do not decrease code coverage in sysdb. It is one of the most important parts of sssd.
I agree. Not increasing the coverage is OK-ish, but we should not decrease the coverage unless the module in question is extremely hard to test (like the AD provider might be sometimes).
On (17/04/14 18:44), Pavel Reichl wrote:
Hello,
attached patch is the first of many to solve https://fedorahosted.org/sssd/ticket/1991
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think it would be best if in case that no results were found both ENOENT value and 'properly' empty list were returned.
Thank for opinions or/and review.
Pavel Reichl
yet another comment.
From 1e7f69536acfb50e221807b59446ed9fb584bab9 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 26 Feb 2014 16:52:26 +0000 Subject: [PATCH 1/5] SYSDB: sysdb_getnetgr returns ENOENT
sysdb_getnetgr did not return ENOENT althought it was expected.
Resolves: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 1 + src/db/sysdb_search.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 5d337ffd8c0f7fbe6808d50538192369102caf17..45d486c3fa32e393ddb0bfe38426f9c2a3611955 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -455,6 +455,7 @@ struct sysdb_netgroup_ctx { } value; };
+/* If no group is found ENOENT is returned and result count is set to 0 */ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *netgroup, diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c index 60ad61368d92d12bca3833bc08b0673c581ab882..97192115b90b985cb0ff42516cdc02a44a33acc9 100644 --- a/src/db/sysdb_search.c +++ b/src/db/sysdb_search.c @@ -827,7 +827,12 @@ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, }
*res = talloc_steal(mem_ctx, result);
- ret = EOK;
- if((*res)->count == 0) {
^^^^^^ I do not see a reason of using output argument here. You can use local variable result.
ret = ENOENT;- } else {
ret = EOK;- }
done: talloc_zfree(tmp_ctx); -- 1.8.4.2
BTW I would expect more reusable solution for ticket #1991 or do you want to add the same snippet to other function?
In the function sysdb_error_to_errno ldb error LDB_ERR_NO_SUCH_OBJECT is transformed to the ENOENT. Could you explain why it did not work?
LS
On Tue, 2014-04-22 at 08:46 +0200, Lukas Slebodnik wrote:
On (17/04/14 18:44), Pavel Reichl wrote:
Hello,
attached patch is the first of many to solve https://fedorahosted.org/sssd/ticket/1991
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think it would be best if in case that no results were found both ENOENT value and 'properly' empty list were returned.
Thank for opinions or/and review.
Pavel Reichl
yet another comment.
From 1e7f69536acfb50e221807b59446ed9fb584bab9 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 26 Feb 2014 16:52:26 +0000 Subject: [PATCH 1/5] SYSDB: sysdb_getnetgr returns ENOENT
sysdb_getnetgr did not return ENOENT althought it was expected.
Resolves: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 1 + src/db/sysdb_search.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 5d337ffd8c0f7fbe6808d50538192369102caf17..45d486c3fa32e393ddb0bfe38426f9c2a3611955 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -455,6 +455,7 @@ struct sysdb_netgroup_ctx { } value; };
+/* If no group is found ENOENT is returned and result count is set to 0 */ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *netgroup, diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c index 60ad61368d92d12bca3833bc08b0673c581ab882..97192115b90b985cb0ff42516cdc02a44a33acc9 100644 --- a/src/db/sysdb_search.c +++ b/src/db/sysdb_search.c @@ -827,7 +827,12 @@ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, }
*res = talloc_steal(mem_ctx, result);
- ret = EOK;
- if((*res)->count == 0) {
^^^^^^ I do not see a reason of using output argument here. You can use local variable result.
OK, fixed.
ret = ENOENT;- } else {
ret = EOK;- }
done: talloc_zfree(tmp_ctx); -- 1.8.4.2
BTW I would expect more reusable solution for ticket #1991 or do you want to add the same snippet to other function?
Well, actually I don't know yet. I think that there are too many different functions, some of them call directly ldap_search others call it indirectly. At first I would prefer to concentrate to fix implementation to expected behavior and mainly to don't break anything.
But of course if/when I will see patterns for reusable solution I will propose it.
In the function sysdb_error_to_errno ldb error LDB_ERR_NO_SUCH_OBJECT is transformed to the ENOENT. Could you explain why it did not work?
I don't think there's any problem with function sysdb_error_to_errno, it converts LDB_SUCCESS to EOK perfectly.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (22/04/14 12:02), Pavel Reichl wrote:
On Tue, 2014-04-22 at 08:46 +0200, Lukas Slebodnik wrote:
On (17/04/14 18:44), Pavel Reichl wrote:
Hello,
attached patch is the first of many to solve https://fedorahosted.org/sssd/ticket/1991
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think it would be best if in case that no results were found both ENOENT value and 'properly' empty list were returned.
Thank for opinions or/and review.
Pavel Reichl
yet another comment.
From 1e7f69536acfb50e221807b59446ed9fb584bab9 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 26 Feb 2014 16:52:26 +0000 Subject: [PATCH 1/5] SYSDB: sysdb_getnetgr returns ENOENT
sysdb_getnetgr did not return ENOENT althought it was expected.
Resolves: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 1 + src/db/sysdb_search.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 5d337ffd8c0f7fbe6808d50538192369102caf17..45d486c3fa32e393ddb0bfe38426f9c2a3611955 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -455,6 +455,7 @@ struct sysdb_netgroup_ctx { } value; };
+/* If no group is found ENOENT is returned and result count is set to 0 */ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *netgroup, diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c index 60ad61368d92d12bca3833bc08b0673c581ab882..97192115b90b985cb0ff42516cdc02a44a33acc9 100644 --- a/src/db/sysdb_search.c +++ b/src/db/sysdb_search.c @@ -827,7 +827,12 @@ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, }
*res = talloc_steal(mem_ctx, result);
- ret = EOK;
- if((*res)->count == 0) {
^^^^^^ I do not see a reason of using output argument here. You can use local variable result.OK, fixed.
ret = ENOENT;- } else {
ret = EOK;- }
done: talloc_zfree(tmp_ctx); -- 1.8.4.2
BTW I would expect more reusable solution for ticket #1991 or do you want to add the same snippet to other function?
Well, actually I don't know yet. I think that there are too many different functions, some of them call directly ldap_search others call
^^^^^^^^^^^ Did you mean ldb_search?
it indirectly. At first I would prefer to concentrate to fix implementation to expected behavior and mainly to don't break anything.
But of course if/when I will see patterns for reusable solution I will propose it.
reusable solution can to use wrapper function for ldb_search sss_ldb_search ???
ret = ldb_search( ... ret = sysdb_error_to_errno(lret); if(ret== EOK && result->count == 0) { ret = ENOENT; }
any objections from other developers?
In the function sysdb_error_to_errno ldb error LDB_ERR_NO_SUCH_OBJECT is transformed to the ENOENT. Could you explain why it did not work?
I don't think there's any problem with function sysdb_error_to_errno, it converts LDB_SUCCESS to EOK perfectly.
It can be bug in ldb that LDB_ERR_NO_SUCH_OBJECT was not returned.
LS
On Tue, Apr 22, 2014 at 12:20:22PM +0200, Lukas Slebodnik wrote:
On (22/04/14 12:02), Pavel Reichl wrote:
On Tue, 2014-04-22 at 08:46 +0200, Lukas Slebodnik wrote:
On (17/04/14 18:44), Pavel Reichl wrote:
Hello,
attached patch is the first of many to solve https://fedorahosted.org/sssd/ticket/1991
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think it would be best if in case that no results were found both ENOENT value and 'properly' empty list were returned.
Thank for opinions or/and review.
Pavel Reichl
yet another comment.
From 1e7f69536acfb50e221807b59446ed9fb584bab9 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 26 Feb 2014 16:52:26 +0000 Subject: [PATCH 1/5] SYSDB: sysdb_getnetgr returns ENOENT
sysdb_getnetgr did not return ENOENT althought it was expected.
Resolves: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 1 + src/db/sysdb_search.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 5d337ffd8c0f7fbe6808d50538192369102caf17..45d486c3fa32e393ddb0bfe38426f9c2a3611955 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -455,6 +455,7 @@ struct sysdb_netgroup_ctx { } value; };
+/* If no group is found ENOENT is returned and result count is set to 0 */ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *netgroup, diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c index 60ad61368d92d12bca3833bc08b0673c581ab882..97192115b90b985cb0ff42516cdc02a44a33acc9 100644 --- a/src/db/sysdb_search.c +++ b/src/db/sysdb_search.c @@ -827,7 +827,12 @@ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, }
*res = talloc_steal(mem_ctx, result);
- ret = EOK;
- if((*res)->count == 0) {
^^^^^^ I do not see a reason of using output argument here. You can use local variable result.OK, fixed.
ret = ENOENT;- } else {
ret = EOK;- }
done: talloc_zfree(tmp_ctx); -- 1.8.4.2
BTW I would expect more reusable solution for ticket #1991 or do you want to add the same snippet to other function?
Well, actually I don't know yet. I think that there are too many different functions, some of them call directly ldap_search others call
^^^^^^^^^^^ Did you mean ldb_search?it indirectly. At first I would prefer to concentrate to fix implementation to expected behavior and mainly to don't break anything.
But of course if/when I will see patterns for reusable solution I will propose it.
reusable solution can to use wrapper function for ldb_search sss_ldb_search ???
ret = ldb_search( ... ret = sysdb_error_to_errno(lret); if(ret== EOK && result->count == 0) { ret = ENOENT; }any objections from other developers?
This seems like a good way to go if we have at least two places that would use this pattern.
On (22/04/14 12:02), Pavel Reichl wrote:
On Tue, 2014-04-22 at 08:46 +0200, Lukas Slebodnik wrote:
On (17/04/14 18:44), Pavel Reichl wrote:
Hello,
attached patch is the first of many to solve https://fedorahosted.org/sssd/ticket/1991
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think it would be best if in case that no results were found both ENOENT value and 'properly' empty list were returned.
Thank for opinions or/and review.
Pavel Reichl
yet another comment.
From 1e7f69536acfb50e221807b59446ed9fb584bab9 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 26 Feb 2014 16:52:26 +0000 Subject: [PATCH 1/5] SYSDB: sysdb_getnetgr returns ENOENT
sysdb_getnetgr did not return ENOENT althought it was expected.
Resolves: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 1 + src/db/sysdb_search.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 5d337ffd8c0f7fbe6808d50538192369102caf17..45d486c3fa32e393ddb0bfe38426f9c2a3611955 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -455,6 +455,7 @@ struct sysdb_netgroup_ctx { } value; };
+/* If no group is found ENOENT is returned and result count is set to 0 */ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *netgroup, diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c index 60ad61368d92d12bca3833bc08b0673c581ab882..97192115b90b985cb0ff42516cdc02a44a33acc9 100644 --- a/src/db/sysdb_search.c +++ b/src/db/sysdb_search.c @@ -827,7 +827,12 @@ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, }
*res = talloc_steal(mem_ctx, result);
- ret = EOK;
- if((*res)->count == 0) {
^^^^^^ I do not see a reason of using output argument here. You can use local variable result.OK, fixed.
ret = ENOENT;- } else {
ret = EOK;- }
done: talloc_zfree(tmp_ctx); -- 1.8.4.2
BTW I would expect more reusable solution for ticket #1991 or do you want to add the same snippet to other function?
Well, actually I don't know yet. I think that there are too many different functions, some of them call directly ldap_search others call it indirectly. At first I would prefer to concentrate to fix implementation to expected behavior and mainly to don't break anything.
But of course if/when I will see patterns for reusable solution I will propose it.
In the function sysdb_error_to_errno ldb error LDB_ERR_NO_SUCH_OBJECT is transformed to the ENOENT. Could you explain why it did not work?
I don't think there's any problem with function sysdb_error_to_errno, it converts LDB_SUCCESS to EOK perfectly.
LS
From a587d4c9013cbf7dd099a9d1c961b4157bea6e05 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 26 Feb 2014 16:52:26 +0000 Subject: [PATCH 1/5] SYSDB: sysdb_getnetgr returns ENOENT
sysdb_getnetgr did not return ENOENT althought it was expected.
We need to move with this patch.
I wrote tests for some sysdb_search functions [1]. But I realized you wrote patch for different function. They are very similar.
src/db/sysdb_search.c:errno_t sysdb_getnetgr errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *netgroup, struct ldb_result **res);
src/db/sysdb_ops.c: int sysdb_search_netgroup_by_name(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *name, const char **attrs, struct ldb_message **msg);
The part of refactoring should also be to unify these functions (if it is possible) or to reuse one function from another.
Function sysdb_search_netgroup_by_name already returns ENOENT.
LS
[1] https://lists.fedorahosted.org/pipermail/sssd-devel/2014-May/019601.html
On Mon, 2014-05-19 at 13:32 +0200, Lukas Slebodnik wrote:
On (22/04/14 12:02), Pavel Reichl wrote:
On Tue, 2014-04-22 at 08:46 +0200, Lukas Slebodnik wrote:
On (17/04/14 18:44), Pavel Reichl wrote:
Hello,
attached patch is the first of many to solve https://fedorahosted.org/sssd/ticket/1991
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think it would be best if in case that no results were found both ENOENT value and 'properly' empty list were returned.
Thank for opinions or/and review.
Pavel Reichl
yet another comment.
From 1e7f69536acfb50e221807b59446ed9fb584bab9 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 26 Feb 2014 16:52:26 +0000 Subject: [PATCH 1/5] SYSDB: sysdb_getnetgr returns ENOENT
sysdb_getnetgr did not return ENOENT althought it was expected.
Resolves: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 1 + src/db/sysdb_search.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 5d337ffd8c0f7fbe6808d50538192369102caf17..45d486c3fa32e393ddb0bfe38426f9c2a3611955 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -455,6 +455,7 @@ struct sysdb_netgroup_ctx { } value; };
+/* If no group is found ENOENT is returned and result count is set to 0 */ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *netgroup, diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c index 60ad61368d92d12bca3833bc08b0673c581ab882..97192115b90b985cb0ff42516cdc02a44a33acc9 100644 --- a/src/db/sysdb_search.c +++ b/src/db/sysdb_search.c @@ -827,7 +827,12 @@ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, }
*res = talloc_steal(mem_ctx, result);
- ret = EOK;
- if((*res)->count == 0) {
^^^^^^ I do not see a reason of using output argument here. You can use local variable result.OK, fixed.
ret = ENOENT;- } else {
ret = EOK;- }
done: talloc_zfree(tmp_ctx); -- 1.8.4.2
BTW I would expect more reusable solution for ticket #1991 or do you want to add the same snippet to other function?
Well, actually I don't know yet. I think that there are too many different functions, some of them call directly ldap_search others call it indirectly. At first I would prefer to concentrate to fix implementation to expected behavior and mainly to don't break anything.
But of course if/when I will see patterns for reusable solution I will propose it.
In the function sysdb_error_to_errno ldb error LDB_ERR_NO_SUCH_OBJECT is transformed to the ENOENT. Could you explain why it did not work?
I don't think there's any problem with function sysdb_error_to_errno, it converts LDB_SUCCESS to EOK perfectly.
LS
From a587d4c9013cbf7dd099a9d1c961b4157bea6e05 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 26 Feb 2014 16:52:26 +0000 Subject: [PATCH 1/5] SYSDB: sysdb_getnetgr returns ENOENT
sysdb_getnetgr did not return ENOENT althought it was expected.
We need to move with this patch.
I wrote tests for some sysdb_search functions [1]. But I realized you wrote patch for different function. They are very similar.
src/db/sysdb_search.c:errno_t sysdb_getnetgr errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *netgroup, struct ldb_result **res);
src/db/sysdb_ops.c: int sysdb_search_netgroup_by_name(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *name, const char **attrs, struct ldb_message **msg);
The part of refactoring should also be to unify these functions (if it is possible) or to reuse one function from another.
I've spent some time studying these functions and I really don't feel like changing the way they are implemented. Both were introduced (even sysdb_get_netgroup_attr function) in one single commit. So I guess from begging they were devised to serve different purposes which was amplified with many patches that were applied mainly to sysdb_getnetgr.
I don't say that your proposal is not a good idea, but I'm afraid that it is too much effort outside main purpose of #1991.
PS: I think it would be possible to replace sysdb_get_netgroup_attr as it is used in tests only IIRC.
Function sysdb_search_netgroup_by_name already returns ENOENT.
LS
[1] https://lists.fedorahosted.org/pipermail/sssd-devel/2014-May/019601.html _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, Apr 22, 2014 at 12:02:55PM +0200, Pavel Reichl wrote:
On Tue, 2014-04-22 at 08:46 +0200, Lukas Slebodnik wrote:
On (17/04/14 18:44), Pavel Reichl wrote:
Hello,
attached patch is the first of many to solve https://fedorahosted.org/sssd/ticket/1991
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think it would be best if in case that no results were found both ENOENT value and 'properly' empty list were returned.
Thank for opinions or/and review.
Pavel Reichl
yet another comment.
From 1e7f69536acfb50e221807b59446ed9fb584bab9 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 26 Feb 2014 16:52:26 +0000 Subject: [PATCH 1/5] SYSDB: sysdb_getnetgr returns ENOENT
sysdb_getnetgr did not return ENOENT althought it was expected.
Resolves: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 1 + src/db/sysdb_search.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 5d337ffd8c0f7fbe6808d50538192369102caf17..45d486c3fa32e393ddb0bfe38426f9c2a3611955 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -455,6 +455,7 @@ struct sysdb_netgroup_ctx { } value; };
+/* If no group is found ENOENT is returned and result count is set to 0 */ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *netgroup, diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c index 60ad61368d92d12bca3833bc08b0673c581ab882..97192115b90b985cb0ff42516cdc02a44a33acc9 100644 --- a/src/db/sysdb_search.c +++ b/src/db/sysdb_search.c @@ -827,7 +827,12 @@ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, }
*res = talloc_steal(mem_ctx, result);
- ret = EOK;
- if((*res)->count == 0) {
^^^^^^ I do not see a reason of using output argument here. You can use local variable result.OK, fixed.
ret = ENOENT;- } else {
ret = EOK;- }
done: talloc_zfree(tmp_ctx); -- 1.8.4.2
BTW I would expect more reusable solution for ticket #1991 or do you want to add the same snippet to other function?
Well, actually I don't know yet. I think that there are too many different functions, some of them call directly ldap_search others call it indirectly. At first I would prefer to concentrate to fix implementation to expected behavior and mainly to don't break anything.
But of course if/when I will see patterns for reusable solution I will propose it.
In the function sysdb_error_to_errno ldb error LDB_ERR_NO_SUCH_OBJECT is transformed to the ENOENT. Could you explain why it did not work?
I don't think there's any problem with function sysdb_error_to_errno, it converts LDB_SUCCESS to EOK perfectly.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I think the sss_ldb_search utility function (with a unit test) would be a good idea. We talked with Sumit about releasing beta2 before I vanish for vacation next week, so no need to hurry. I think it would be fine to postpone this work even further.
From a587d4c9013cbf7dd099a9d1c961b4157bea6e05 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 26 Feb 2014 16:52:26 +0000 Subject: [PATCH 1/5] SYSDB: sysdb_getnetgr returns ENOENT
sysdb_getnetgr did not return ENOENT althought it was expected.
Resolves: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 1 + src/db/sysdb_search.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 5d337ffd8c0f7fbe6808d50538192369102caf17..45d486c3fa32e393ddb0bfe38426f9c2a3611955 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -455,6 +455,7 @@ struct sysdb_netgroup_ctx { } value; };
+/* If no group is found ENOENT is returned and result count is set to 0 */
^^^^^ If no netgroup is found
errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *netgroup, diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c index 60ad61368d92d12bca3833bc08b0673c581ab882..c911445c95a666bb399b85e41799226d0cb433fd 100644 --- a/src/db/sysdb_search.c +++ b/src/db/sysdb_search.c @@ -827,7 +827,12 @@ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, }
*res = talloc_steal(mem_ctx, result);
- ret = EOK;
- if(result->count == 0) {
Please put a space after the if
ret = ENOENT;- } else {
ret = EOK;- }
done: talloc_zfree(tmp_ctx); -- 1.8.4.2
It would be nice to have a unit test. I'm going to push the patch with test_sysdb_search_return_ENOENT, so maybe add it there.
On Tue, 2014-05-27 at 17:01 +0200, Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 12:02:55PM +0200, Pavel Reichl wrote:
On Tue, 2014-04-22 at 08:46 +0200, Lukas Slebodnik wrote:
On (17/04/14 18:44), Pavel Reichl wrote:
Hello,
attached patch is the first of many to solve https://fedorahosted.org/sssd/ticket/1991
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think it would be best if in case that no results were found both ENOENT value and 'properly' empty list were returned.
Thank for opinions or/and review.
Pavel Reichl
yet another comment.
From 1e7f69536acfb50e221807b59446ed9fb584bab9 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 26 Feb 2014 16:52:26 +0000 Subject: [PATCH 1/5] SYSDB: sysdb_getnetgr returns ENOENT
sysdb_getnetgr did not return ENOENT althought it was expected.
Resolves: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 1 + src/db/sysdb_search.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 5d337ffd8c0f7fbe6808d50538192369102caf17..45d486c3fa32e393ddb0bfe38426f9c2a3611955 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -455,6 +455,7 @@ struct sysdb_netgroup_ctx { } value; };
+/* If no group is found ENOENT is returned and result count is set to 0 */ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *netgroup, diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c index 60ad61368d92d12bca3833bc08b0673c581ab882..97192115b90b985cb0ff42516cdc02a44a33acc9 100644 --- a/src/db/sysdb_search.c +++ b/src/db/sysdb_search.c @@ -827,7 +827,12 @@ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, }
*res = talloc_steal(mem_ctx, result);
- ret = EOK;
- if((*res)->count == 0) {
^^^^^^ I do not see a reason of using output argument here. You can use local variable result.OK, fixed.
ret = ENOENT;- } else {
ret = EOK;- }
done: talloc_zfree(tmp_ctx); -- 1.8.4.2
BTW I would expect more reusable solution for ticket #1991 or do you want to add the same snippet to other function?
Well, actually I don't know yet. I think that there are too many different functions, some of them call directly ldap_search others call it indirectly. At first I would prefer to concentrate to fix implementation to expected behavior and mainly to don't break anything.
But of course if/when I will see patterns for reusable solution I will propose it.
In the function sysdb_error_to_errno ldb error LDB_ERR_NO_SUCH_OBJECT is transformed to the ENOENT. Could you explain why it did not work?
I don't think there's any problem with function sysdb_error_to_errno, it converts LDB_SUCCESS to EOK perfectly.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I think the sss_ldb_search utility function (with a unit test) would be a good idea. We talked with Sumit about releasing beta2 before I vanish for vacation next week, so no need to hurry. I think it would be fine to postpone this work even further.
Hello,
I attached patches reworked as proposed by Lukas.
Thanks, Pavel Reichl
On Thu, Jun 19, 2014 at 05:53:37PM +0200, Pavel Reichl wrote:
On Tue, 2014-05-27 at 17:01 +0200, Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 12:02:55PM +0200, Pavel Reichl wrote:
On Tue, 2014-04-22 at 08:46 +0200, Lukas Slebodnik wrote:
On (17/04/14 18:44), Pavel Reichl wrote:
Hello,
attached patch is the first of many to solve https://fedorahosted.org/sssd/ticket/1991
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think it would be best if in case that no results were found both ENOENT value and 'properly' empty list were returned.
Thank for opinions or/and review.
Pavel Reichl
yet another comment.
From 1e7f69536acfb50e221807b59446ed9fb584bab9 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 26 Feb 2014 16:52:26 +0000 Subject: [PATCH 1/5] SYSDB: sysdb_getnetgr returns ENOENT
sysdb_getnetgr did not return ENOENT althought it was expected.
Resolves: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 1 + src/db/sysdb_search.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 5d337ffd8c0f7fbe6808d50538192369102caf17..45d486c3fa32e393ddb0bfe38426f9c2a3611955 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -455,6 +455,7 @@ struct sysdb_netgroup_ctx { } value; };
+/* If no group is found ENOENT is returned and result count is set to 0 */ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *netgroup, diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c index 60ad61368d92d12bca3833bc08b0673c581ab882..97192115b90b985cb0ff42516cdc02a44a33acc9 100644 --- a/src/db/sysdb_search.c +++ b/src/db/sysdb_search.c @@ -827,7 +827,12 @@ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, }
*res = talloc_steal(mem_ctx, result);
- ret = EOK;
- if((*res)->count == 0) {
^^^^^^ I do not see a reason of using output argument here. You can use local variable result.OK, fixed.
ret = ENOENT;- } else {
ret = EOK;- }
done: talloc_zfree(tmp_ctx); -- 1.8.4.2
BTW I would expect more reusable solution for ticket #1991 or do you want to add the same snippet to other function?
Well, actually I don't know yet. I think that there are too many different functions, some of them call directly ldap_search others call it indirectly. At first I would prefer to concentrate to fix implementation to expected behavior and mainly to don't break anything.
But of course if/when I will see patterns for reusable solution I will propose it.
In the function sysdb_error_to_errno ldb error LDB_ERR_NO_SUCH_OBJECT is transformed to the ENOENT. Could you explain why it did not work?
I don't think there's any problem with function sysdb_error_to_errno, it converts LDB_SUCCESS to EOK perfectly.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I think the sss_ldb_search utility function (with a unit test) would be a good idea. We talked with Sumit about releasing beta2 before I vanish for vacation next week, so no need to hurry. I think it would be fine to postpone this work even further.
Hello,
I attached patches reworked as proposed by Lukas.
Thanks, Pavel Reichl
From 6e2af6d696fd0e731467c50dbb715756a4bcb0a3 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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.
From b021855c45a60760a7f7af5b6411dd8a41d58e14 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 19 Jun 2014 15:38:59 +0100 Subject: [PATCH 2/4] TESTS: add tests for sss_ldb_search
Resolves: https://fedorahosted.org/sssd/ticket/1991
[...]
- /* Filter yeilding no results */
- ret = sss_ldb_search(test_ctx->sysdb->ldb, test_ctx, &res, group_dn,
LDB_SCOPE_BASE, NULL, "objectClass=user");
If you want a negative test here, then I think it would be better to use a nonexistent objectclass instead of relying on the fact that there are no users in sysdb yet. You can make up anything, ldb is schemaless.
- 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, "");- 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
From 657274de1a0f5f7ab787ec641080f5490cef704a Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 26 Feb 2014 16:52:26 +0000 Subject: [PATCH 3/4] 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.
ACK
From 31837f5bb025e1210dedfc89faf9ab4f489df725 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 19 Jun 2014 16:18:12 +0100 Subject: [PATCH 4/4] TESTS: sysdb_getnetgr - return ENOENT
ACK
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:
On Tue, 2014-05-27 at 17:01 +0200, Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 12:02:55PM +0200, Pavel Reichl wrote:
On Tue, 2014-04-22 at 08:46 +0200, Lukas Slebodnik wrote:
On (17/04/14 18:44), Pavel Reichl wrote:
Hello,
attached patch is the first of many to solve https://fedorahosted.org/sssd/ticket/1991
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think it would be best if in case that no results were found both ENOENT value and 'properly' empty list were returned.
Thank for opinions or/and review.
Pavel Reichl
yet another comment.
From 1e7f69536acfb50e221807b59446ed9fb584bab9 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 26 Feb 2014 16:52:26 +0000 Subject: [PATCH 1/5] SYSDB: sysdb_getnetgr returns ENOENT
sysdb_getnetgr did not return ENOENT althought it was expected.
Resolves: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 1 + src/db/sysdb_search.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 5d337ffd8c0f7fbe6808d50538192369102caf17..45d486c3fa32e393ddb0bfe38426f9c2a3611955 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -455,6 +455,7 @@ struct sysdb_netgroup_ctx { } value; };
+/* If no group is found ENOENT is returned and result count is set to 0 */ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *netgroup, diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c index 60ad61368d92d12bca3833bc08b0673c581ab882..97192115b90b985cb0ff42516cdc02a44a33acc9 100644 --- a/src/db/sysdb_search.c +++ b/src/db/sysdb_search.c @@ -827,7 +827,12 @@ errno_t sysdb_getnetgr(TALLOC_CTX *mem_ctx, }
*res = talloc_steal(mem_ctx, result);
- ret = EOK;
- if((*res)->count == 0) {
^^^^^^ I do not see a reason of using output argument here. You can use local variable result.OK, fixed.
ret = ENOENT;- } else {
ret = EOK;- }
done: talloc_zfree(tmp_ctx); -- 1.8.4.2
BTW I would expect more reusable solution for ticket #1991 or do you want to add the same snippet to other function?
Well, actually I don't know yet. I think that there are too many different functions, some of them call directly ldap_search others call it indirectly. At first I would prefer to concentrate to fix implementation to expected behavior and mainly to don't break anything.
But of course if/when I will see patterns for reusable solution I will propose it.
In the function sysdb_error_to_errno ldb error LDB_ERR_NO_SUCH_OBJECT is transformed to the ENOENT. Could you explain why it did not work?
I don't think there's any problem with function sysdb_error_to_errno, it converts LDB_SUCCESS to EOK perfectly.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I think the sss_ldb_search utility function (with a unit test) would be a good idea. We talked with Sumit about releasing beta2 before I vanish for vacation next week, so no need to hurry. I think it would be fine to postpone this work even further.
Hello,
I attached patches reworked as proposed by Lukas.
Thanks, Pavel Reichl
From 6e2af6d696fd0e731467c50dbb715756a4bcb0a3 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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.
From b021855c45a60760a7f7af5b6411dd8a41d58e14 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 19 Jun 2014 15:38:59 +0100 Subject: [PATCH 2/4] TESTS: add tests for sss_ldb_search
Resolves: https://fedorahosted.org/sssd/ticket/1991
[...]
- /* Filter yeilding no results */
- ret = sss_ldb_search(test_ctx->sysdb->ldb, test_ctx, &res, group_dn,
LDB_SCOPE_BASE, NULL, "objectClass=user");If you want a negative test here, then I think it would be better to use a nonexistent objectclass instead of relying on the fact that there are no users in sysdb yet. You can make up anything, ldb is schemaless.
OK, although group_dn has value where no user should be I agree that better be safe than sorry, so "objectClass=nonExistingObjectClass" is used instead.
- 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, "");- 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
From 657274de1a0f5f7ab787ec641080f5490cef704a Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 26 Feb 2014 16:52:26 +0000 Subject: [PATCH 3/4] 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.
ACK
Unfortunately it is expected by calling side that in case of ENOENT it is OK to access result->count. It's not easy to completely fix this expectation (5th patch only partially solves this issue) so function will continue to support this behavior.
From 31837f5bb025e1210dedfc89faf9ab4f489df725 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Thu, 19 Jun 2014 16:18:12 +0100 Subject: [PATCH 4/4] TESTS: sysdb_getnetgr - return ENOENT
ACK _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Patch 5 removes needles check on result->count values if ENOENT was returned.
Thanks,
PR
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@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@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@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@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@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@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
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@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@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.
}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@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@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).
- }
done: talloc_zfree(tmp_ctx); -- 1.8.4.2
From 94a3501fa6d0f238c889e2e05858c0a42579815d Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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.
From 44c4d576c840c01bade8f7f7aa6c980793d79505 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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 readI think conditionc could be more flatten if (ret == EOK) { ... } else if (ret == ENOENT) { ... } else { ... }
OK.
Or switch/case/default.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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@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@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@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@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@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 readI 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@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@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@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@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@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
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@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@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@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@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@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 readI 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@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@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@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@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@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
On Wed, 2014-06-25 at 15:45 +0200, Lukas Slebodnik wrote: [misc]
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.
OK, ticket #2381 (https://fedorahosted.org/sssd/ticket/2381) was filed.
Regards,
Pavel Reichl
On Tue, 2014-06-24 at 17:28 +0200, Pavel Reichl wrote:
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.
Wouldn't it be simpler to use a macro in this case ? Your new fuinction hjas to go through variadic expansion prematurely, and adds a lot of code:
something like: #define sss_ldbn_seartch(ret, arguments, etc., format, ...) { do { int result = ldb_search(argumets, etc, format, ##__VA_ARGS__); ret = result; if (res->count == 0) { ret = ENOENT; } } while(0);
On (25/06/14 10:51), Simo Sorce wrote:
On Tue, 2014-06-24 at 17:28 +0200, Pavel Reichl wrote:
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.
Wouldn't it be simpler to use a macro in this case ? Your new fuinction hjas to go through variadic expansion prematurely, and adds a lot of code:
something like: #define sss_ldbn_seartch(ret, arguments, etc., format, ...) {
I was thinking about variadic macro, but i wanted to simulate function. ret = macro_sss_ldbn_seartch(...) It is a realy nice trick.
do { int result = ldb_search(argumets, etc, format, ##__VA_ARGS__); ret = result; if (res->count == 0) { ret = ENOENT; } } while(0);
LS
On Wed, 2014-06-25 at 10:51 -0400, Simo Sorce wrote:
On Tue, 2014-06-24 at 17:28 +0200, Pavel Reichl wrote:
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.
Wouldn't it be simpler to use a macro in this case ? Your new fuinction hjas to go through variadic expansion prematurely, and adds a lot of code:
something like: #define sss_ldbn_seartch(ret, arguments, etc., format, ...) { do { int result = ldb_search(argumets, etc, format, ##__VA_ARGS__); ret = result; if (res->count == 0) { ret = ENOENT; } } while(0);
Hello,
sorry for late reply. I guess your approach will definitely save some lines of code and some CPU cycles.
I just must get used to the idea that macros are not considered evil in out environment. :-)
Please see attached patch.
Regards,
Pavel Reichl
On (11/07/14 17:28), Pavel Reichl wrote:
On Wed, 2014-06-25 at 10:51 -0400, Simo Sorce wrote:
On Tue, 2014-06-24 at 17:28 +0200, Pavel Reichl wrote:
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.
Wouldn't it be simpler to use a macro in this case ? Your new fuinction hjas to go through variadic expansion prematurely, and adds a lot of code:
something like: #define sss_ldbn_seartch(ret, arguments, etc., format, ...) { do { int result = ldb_search(argumets, etc, format, ##__VA_ARGS__); ret = result; if (res->count == 0) { ret = ENOENT; } } while(0);
Hello,
sorry for late reply. I guess your approach will definitely save some lines of code and some CPU cycles.
I just must get used to the idea that macros are not considered evil in out environment. :-)
Please see attached patch.
Regards,
Pavel Reichl
From d1293b63adcab4298228af225f3af5803759d34b Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 11 Jul 2014 15:21:59 +0100 Subject: [PATCH] SYSDB: SSS_LDB_SEARCH - macro around ldb_search
This patch amends previous patch 5153e8b9793dea1e212ca08af0f77ea1d023cbb7.
Macro SSS_LDB_SEARCH is used instead of using fuction sss_ldb_search as a wrapper around ldb_search which could lead to premature expansion of variadic parameters.
Part of solution for: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 19 +++++++++++++----- src/db/sysdb_ops.c | 51 ------------------------------------------------- src/db/sysdb_search.c | 10 +++++----- src/tests/sysdb-tests.c | 38 ++++++++++++++++++------------------ 4 files changed, 38 insertions(+), 80 deletions(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 17cd5110c9bdafd8d7b18e621188523ed41e5c8a..a613c84ffa9e4ab6ebdc930a8c4876e69aa3cd13 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -481,11 +481,6 @@ 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() */
@@ -864,4 +859,18 @@ errno_t sysdb_search_object_by_sid(TALLOC_CTX *mem_ctx, const char *sid_str, const char **attrs, struct ldb_result **msg);
+#define SSS_LDB_SEARCH(ret, ldb, mem_ctx, _result, base, scope, attrs, \
exp_fmt, ...) \+{ \
- int lret; \
\- lret = ldb_search(ldb, mem_ctx, _result, base, scope, attrs, exp_fmt, \
##__VA_ARGS__); \- ret = sysdb_error_to_errno(lret); \
- if (ret == EOK && (*_result)->count == 0) { \
ret = ENOENT; \- } \
+} while(0)
Use do { } while(0); for macro, not just { } while(0)
@see macros SYSDB_VERSION_LOWER_ERROR, SYSDB_VERSION_HIGHER_ERROR, DEBUG_PAM_DATA ...
Otherwise patch LGTM. If there are no other objections to this version I will give ACK.
LS
On 08/28/2014 10:31 AM, Lukas Slebodnik wrote:
On (11/07/14 17:28), Pavel Reichl wrote:
On Wed, 2014-06-25 at 10:51 -0400, Simo Sorce wrote:
On Tue, 2014-06-24 at 17:28 +0200, Pavel Reichl wrote:
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.
Wouldn't it be simpler to use a macro in this case ? Your new fuinction hjas to go through variadic expansion prematurely, and adds a lot of code:
something like: #define sss_ldbn_seartch(ret, arguments, etc., format, ...) { do { int result = ldb_search(argumets, etc, format, ##__VA_ARGS__); ret = result; if (res->count == 0) { ret = ENOENT; } } while(0);
Hello,
sorry for late reply. I guess your approach will definitely save some lines of code and some CPU cycles.
I just must get used to the idea that macros are not considered evil in out environment. :-)
Please see attached patch.
Regards,
Pavel Reichl
From d1293b63adcab4298228af225f3af5803759d34b Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 11 Jul 2014 15:21:59 +0100 Subject: [PATCH] SYSDB: SSS_LDB_SEARCH - macro around ldb_search
This patch amends previous patch 5153e8b9793dea1e212ca08af0f77ea1d023cbb7.
Macro SSS_LDB_SEARCH is used instead of using fuction sss_ldb_search as a wrapper around ldb_search which could lead to premature expansion of variadic parameters.
Part of solution for: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 19 +++++++++++++----- src/db/sysdb_ops.c | 51 ------------------------------------------------- src/db/sysdb_search.c | 10 +++++----- src/tests/sysdb-tests.c | 38 ++++++++++++++++++------------------ 4 files changed, 38 insertions(+), 80 deletions(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 17cd5110c9bdafd8d7b18e621188523ed41e5c8a..a613c84ffa9e4ab6ebdc930a8c4876e69aa3cd13 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -481,11 +481,6 @@ 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() */
@@ -864,4 +859,18 @@ errno_t sysdb_search_object_by_sid(TALLOC_CTX *mem_ctx, const char *sid_str, const char **attrs, struct ldb_result **msg);
+#define SSS_LDB_SEARCH(ret, ldb, mem_ctx, _result, base, scope, attrs, \
exp_fmt, ...) \+{ \
- int lret; \
\- lret = ldb_search(ldb, mem_ctx, _result, base, scope, attrs, exp_fmt, \
##__VA_ARGS__); \- ret = sysdb_error_to_errno(lret); \
- if (ret == EOK && (*_result)->count == 0) { \
ret = ENOENT; \- } \
+} while(0)
Use do { } while(0); for macro, not just { } while(0)
@see macros SYSDB_VERSION_LOWER_ERROR, SYSDB_VERSION_HIGHER_ERROR, DEBUG_PAM_DATA ...
Otherwise patch LGTM. If there are no other objections to this version I will give ACK.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks, updated patch attached.
On Thu, Aug 28, 2014 at 11:49:38AM +0200, Pavel Reichl wrote:
Hello,
sorry for late reply. I guess your approach will definitely save some lines of code and some CPU cycles.
I just must get used to the idea that macros are not considered evil in out environment. :-)
Please see attached patch.
Regards,
Pavel Reichl
I have one small comment, sorry for coming late into the thread...
-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() */
@@ -901,4 +896,16 @@ errno_t sysdb_gpo_get_gpos(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, struct ldb_result **_result);
+#define SSS_LDB_SEARCH(ret, ldb, mem_ctx, _result, base, scope, attrs, \
exp_fmt, ...) do { \- int lret; \
Can you rename the variable "lret" here? Calling it "lret" might clash with some other variable from outer scope..
Maybe "_sls_lret" ? (_sss_ldb_search_lret)
\- lret = ldb_search(ldb, mem_ctx, _result, base, scope, attrs, exp_fmt, \
##__VA_ARGS__); \- ret = sysdb_error_to_errno(lret); \
- if (ret == EOK && (*_result)->count == 0) { \
ret = ENOENT; \- } \
+} while(0)
On 08/28/2014 06:19 PM, Jakub Hrozek wrote:
On Thu, Aug 28, 2014 at 11:49:38AM +0200, Pavel Reichl wrote:
Hello,
sorry for late reply. I guess your approach will definitely save some lines of code and some CPU cycles.
I just must get used to the idea that macros are not considered evil in out environment. :-)
Please see attached patch.
Regards,
Pavel Reichl
I have one small comment, sorry for coming late into the thread...
-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() */
@@ -901,4 +896,16 @@ errno_t sysdb_gpo_get_gpos(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, struct ldb_result **_result);
+#define SSS_LDB_SEARCH(ret, ldb, mem_ctx, _result, base, scope, attrs, \
exp_fmt, ...) do { \- int lret; \
Can you rename the variable "lret" here? Calling it "lret" might clash with some other variable from outer scope..
Maybe "_sls_lret" ? (_sss_ldb_search_lret)
\- lret = ldb_search(ldb, mem_ctx, _result, base, scope, attrs, exp_fmt, \
##__VA_ARGS__); \- ret = sysdb_error_to_errno(lret); \
- if (ret == EOK && (*_result)->count == 0) { \
ret = ENOENT; \- } \
+} while(0)
Thanks, for noticing. New patch attached.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (28/08/14 18:44), Pavel Reichl wrote:
On 08/28/2014 06:19 PM, Jakub Hrozek wrote:
On Thu, Aug 28, 2014 at 11:49:38AM +0200, Pavel Reichl wrote:
Hello,
sorry for late reply. I guess your approach will definitely save some lines of code and some CPU cycles.
I just must get used to the idea that macros are not considered evil in out environment. :-)
Please see attached patch.
Regards,
Pavel Reichl
I have one small comment, sorry for coming late into the thread...
-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() */
@@ -901,4 +896,16 @@ errno_t sysdb_gpo_get_gpos(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, struct ldb_result **_result); +#define SSS_LDB_SEARCH(ret, ldb, mem_ctx, _result, base, scope, attrs, \
exp_fmt, ...) do { \- int lret; \
Can you rename the variable "lret" here? Calling it "lret" might clash with some other variable from outer scope..
Maybe "_sls_lret" ? (_sss_ldb_search_lret)
\- lret = ldb_search(ldb, mem_ctx, _result, base, scope, attrs, exp_fmt, \
##__VA_ARGS__); \- ret = sysdb_error_to_errno(lret); \
- if (ret == EOK && (*_result)->count == 0) { \
ret = ENOENT; \- } \
+} while(0)
Thanks, for noticing. New patch attached.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From 517a682750eb0358b76486ede763af4a91963347 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 11 Jul 2014 15:21:59 +0100 Subject: [PATCH] SYSDB: SSS_LDB_SEARCH - macro around ldb_search
This patch amends previous patch 5153e8b9793dea1e212ca08af0f77ea1d023cbb7.
Macro SSS_LDB_SEARCH is used instead of using fuction sss_ldb_search as a wrapper around ldb_search which could lead to premature expansion of variadic parameters.
Part of solution for: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 17 ++++++++++++----- src/db/sysdb_ops.c | 51 ------------------------------------------------- src/db/sysdb_search.c | 10 +++++----- src/tests/sysdb-tests.c | 38 ++++++++++++++++++------------------ 4 files changed, 36 insertions(+), 80 deletions(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 3cef1e66bdc5c3870ed1a349c797e66ac4ad3475..2fc8ed529d51a58e949bcb788bba56e8bd7d7e11 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -481,11 +481,6 @@ 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() */
@@ -901,4 +896,16 @@ errno_t sysdb_gpo_get_gpos(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, struct ldb_result **_result);
+#define SSS_LDB_SEARCH(ret, ldb, mem_ctx, _result, base, scope, attrs, \
exp_fmt, ...) do { \- int _sls_lret; \
\- _sls_lret = ldb_search(ldb, mem_ctx, _result, base, scope, attrs, \
exp_fmt, ##__VA_ARGS__); \- ret = sysdb_error_to_errno(_sls_lret); \
- if (ret == EOK && (*_result)->count == 0) { \
ret = ENOENT; \- } \
+} while(0)
#endif /* __SYS_DB_H__ */
You moved new macro to the end of file, but there is "GPO section". @see comment in sysdb.h "/* === Functions related to GPOs === */"
More suitable would be section "/* Search Entry */"
LS
On 08/29/2014 10:14 AM, Lukas Slebodnik wrote:
On (28/08/14 18:44), Pavel Reichl wrote:
On 08/28/2014 06:19 PM, Jakub Hrozek wrote:
On Thu, Aug 28, 2014 at 11:49:38AM +0200, Pavel Reichl wrote:
Hello,
sorry for late reply. I guess your approach will definitely save some lines of code and some CPU cycles.
I just must get used to the idea that macros are not considered evil in out environment. :-)
Please see attached patch.
Regards,
Pavel Reichl
I have one small comment, sorry for coming late into the thread...
-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() */
@@ -901,4 +896,16 @@ errno_t sysdb_gpo_get_gpos(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, struct ldb_result **_result); +#define SSS_LDB_SEARCH(ret, ldb, mem_ctx, _result, base, scope, attrs, \
exp_fmt, ...) do { \- int lret; \
Can you rename the variable "lret" here? Calling it "lret" might clash with some other variable from outer scope..
Maybe "_sls_lret" ? (_sss_ldb_search_lret)
\- lret = ldb_search(ldb, mem_ctx, _result, base, scope, attrs, exp_fmt, \
##__VA_ARGS__); \- ret = sysdb_error_to_errno(lret); \
- if (ret == EOK && (*_result)->count == 0) { \
ret = ENOENT; \- } \
+} while(0)
Thanks, for noticing. New patch attached.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From 517a682750eb0358b76486ede763af4a91963347 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 11 Jul 2014 15:21:59 +0100 Subject: [PATCH] SYSDB: SSS_LDB_SEARCH - macro around ldb_search
This patch amends previous patch 5153e8b9793dea1e212ca08af0f77ea1d023cbb7.
Macro SSS_LDB_SEARCH is used instead of using fuction sss_ldb_search as a wrapper around ldb_search which could lead to premature expansion of variadic parameters.
Part of solution for: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 17 ++++++++++++----- src/db/sysdb_ops.c | 51 ------------------------------------------------- src/db/sysdb_search.c | 10 +++++----- src/tests/sysdb-tests.c | 38 ++++++++++++++++++------------------ 4 files changed, 36 insertions(+), 80 deletions(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 3cef1e66bdc5c3870ed1a349c797e66ac4ad3475..2fc8ed529d51a58e949bcb788bba56e8bd7d7e11 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -481,11 +481,6 @@ 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() */
@@ -901,4 +896,16 @@ errno_t sysdb_gpo_get_gpos(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, struct ldb_result **_result);
+#define SSS_LDB_SEARCH(ret, ldb, mem_ctx, _result, base, scope, attrs, \
exp_fmt, ...) do { \- int _sls_lret; \
\- _sls_lret = ldb_search(ldb, mem_ctx, _result, base, scope, attrs, \
exp_fmt, ##__VA_ARGS__); \- ret = sysdb_error_to_errno(_sls_lret); \
- if (ret == EOK && (*_result)->count == 0) { \
ret = ENOENT; \- } \
+} while(0)
#endif /* __SYS_DB_H__ */
You moved new macro to the end of file, but there is "GPO section". @see comment in sysdb.h "/* === Functions related to GPOs === */"
More suitable would be section "/* Search Entry */"
OK, makes sense. Please see attached patch.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (01/09/14 11:20), Pavel Reichl wrote:
On 08/29/2014 10:14 AM, Lukas Slebodnik wrote:
On (28/08/14 18:44), Pavel Reichl wrote:
On 08/28/2014 06:19 PM, Jakub Hrozek wrote:
On Thu, Aug 28, 2014 at 11:49:38AM +0200, Pavel Reichl wrote:
>Hello, > >sorry for late reply. I guess your approach will definitely save some >lines of code and some CPU cycles. > >I just must get used to the idea that macros are not considered evil in >out environment. :-) > >Please see attached patch. > >Regards, > >Pavel Reichl >
I have one small comment, sorry for coming late into the thread...
-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() */
@@ -901,4 +896,16 @@ errno_t sysdb_gpo_get_gpos(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, struct ldb_result **_result); +#define SSS_LDB_SEARCH(ret, ldb, mem_ctx, _result, base, scope, attrs, \
exp_fmt, ...) do { \- int lret; \
Can you rename the variable "lret" here? Calling it "lret" might clash with some other variable from outer scope..
Maybe "_sls_lret" ? (_sss_ldb_search_lret)
\- lret = ldb_search(ldb, mem_ctx, _result, base, scope, attrs, exp_fmt, \
##__VA_ARGS__); \- ret = sysdb_error_to_errno(lret); \
- if (ret == EOK && (*_result)->count == 0) { \
ret = ENOENT; \- } \
+} while(0)
Thanks, for noticing. New patch attached.
You moved new macro to the end of file, but there is "GPO section". @see comment in sysdb.h "/* === Functions related to GPOs === */"
More suitable would be section "/* Search Entry */"
OK, makes sense. Please see attached patch.
From 0eb33210da528cedfb423db2107e87a9ef61cccc Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 11 Jul 2014 15:21:59 +0100 Subject: [PATCH] SYSDB: SSS_LDB_SEARCH - macro around ldb_search
This patch amends previous patch 5153e8b9793dea1e212ca08af0f77ea1d023cbb7.
Macro SSS_LDB_SEARCH is used instead of using fuction sss_ldb_search as a wrapper around ldb_search which could lead to premature expansion of variadic parameters.
Part of solution for: https://fedorahosted.org/sssd/ticket/1991
ACK
LS
On Wed, Sep 03, 2014 at 03:12:52PM +0200, Lukas Slebodnik wrote:
From 0eb33210da528cedfb423db2107e87a9ef61cccc Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 11 Jul 2014 15:21:59 +0100 Subject: [PATCH] SYSDB: SSS_LDB_SEARCH - macro around ldb_search
This patch amends previous patch 5153e8b9793dea1e212ca08af0f77ea1d023cbb7.
Macro SSS_LDB_SEARCH is used instead of using fuction sss_ldb_search as a wrapper around ldb_search which could lead to premature expansion of variadic parameters.
Part of solution for: https://fedorahosted.org/sssd/ticket/1991
ACK
LS
* master: 61602026ed8c91efd166000562899670449f1b50
On Thu, 2014-08-28 at 10:31 +0200, Lukas Slebodnik wrote:
On (11/07/14 17:28), Pavel Reichl wrote:
On Wed, 2014-06-25 at 10:51 -0400, Simo Sorce wrote:
On Tue, 2014-06-24 at 17:28 +0200, Pavel Reichl wrote:
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.
Wouldn't it be simpler to use a macro in this case ? Your new fuinction hjas to go through variadic expansion prematurely, and adds a lot of code:
something like: #define sss_ldbn_seartch(ret, arguments, etc., format, ...) { do { int result = ldb_search(argumets, etc, format, ##__VA_ARGS__); ret = result; if (res->count == 0) { ret = ENOENT; } } while(0);
Hello,
sorry for late reply. I guess your approach will definitely save some lines of code and some CPU cycles.
I just must get used to the idea that macros are not considered evil in out environment. :-)
Please see attached patch.
Regards,
Pavel Reichl
From d1293b63adcab4298228af225f3af5803759d34b Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 11 Jul 2014 15:21:59 +0100 Subject: [PATCH] SYSDB: SSS_LDB_SEARCH - macro around ldb_search
This patch amends previous patch 5153e8b9793dea1e212ca08af0f77ea1d023cbb7.
Macro SSS_LDB_SEARCH is used instead of using fuction sss_ldb_search as a wrapper around ldb_search which could lead to premature expansion of variadic parameters.
Part of solution for: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 19 +++++++++++++----- src/db/sysdb_ops.c | 51 ------------------------------------------------- src/db/sysdb_search.c | 10 +++++----- src/tests/sysdb-tests.c | 38 ++++++++++++++++++------------------ 4 files changed, 38 insertions(+), 80 deletions(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 17cd5110c9bdafd8d7b18e621188523ed41e5c8a..a613c84ffa9e4ab6ebdc930a8c4876e69aa3cd13 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -481,11 +481,6 @@ 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() */
@@ -864,4 +859,18 @@ errno_t sysdb_search_object_by_sid(TALLOC_CTX *mem_ctx, const char *sid_str, const char **attrs, struct ldb_result **msg);
+#define SSS_LDB_SEARCH(ret, ldb, mem_ctx, _result, base, scope, attrs, \
exp_fmt, ...) \+{ \
- int lret; \
\- lret = ldb_search(ldb, mem_ctx, _result, base, scope, attrs, exp_fmt, \
##__VA_ARGS__); \- ret = sysdb_error_to_errno(lret); \
- if (ret == EOK && (*_result)->count == 0) { \
ret = ENOENT; \- } \
+} while(0)
Use do { } while(0); for macro, not just { } while(0)
@see macros SYSDB_VERSION_LOWER_ERROR, SYSDB_VERSION_HIGHER_ERROR, DEBUG_PAM_DATA ...
Otherwise patch LGTM. If there are no other objections to this version I will give ACK.
Sorry to come late, but why a macro and not just a function (possibly inlined but doesn't seem necessary to me) ?
Simo.
On (28/08/14 09:48), Simo Sorce wrote:
On Thu, 2014-08-28 at 10:31 +0200, Lukas Slebodnik wrote:
On (11/07/14 17:28), Pavel Reichl wrote:
On Wed, 2014-06-25 at 10:51 -0400, Simo Sorce wrote:
On Tue, 2014-06-24 at 17:28 +0200, Pavel Reichl wrote:
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.
Wouldn't it be simpler to use a macro in this case ? Your new fuinction hjas to go through variadic expansion prematurely, and adds a lot of code:
something like: #define sss_ldbn_seartch(ret, arguments, etc., format, ...) { do { int result = ldb_search(argumets, etc, format, ##__VA_ARGS__); ret = result; if (res->count == 0) { ret = ENOENT; } } while(0);
Hello,
sorry for late reply. I guess your approach will definitely save some lines of code and some CPU cycles.
I just must get used to the idea that macros are not considered evil in out environment. :-)
Please see attached patch.
Regards,
Pavel Reichl
From d1293b63adcab4298228af225f3af5803759d34b Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 11 Jul 2014 15:21:59 +0100 Subject: [PATCH] SYSDB: SSS_LDB_SEARCH - macro around ldb_search
This patch amends previous patch 5153e8b9793dea1e212ca08af0f77ea1d023cbb7.
Macro SSS_LDB_SEARCH is used instead of using fuction sss_ldb_search as a wrapper around ldb_search which could lead to premature expansion of variadic parameters.
Part of solution for: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 19 +++++++++++++----- src/db/sysdb_ops.c | 51 ------------------------------------------------- src/db/sysdb_search.c | 10 +++++----- src/tests/sysdb-tests.c | 38 ++++++++++++++++++------------------ 4 files changed, 38 insertions(+), 80 deletions(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 17cd5110c9bdafd8d7b18e621188523ed41e5c8a..a613c84ffa9e4ab6ebdc930a8c4876e69aa3cd13 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -481,11 +481,6 @@ 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() */
@@ -864,4 +859,18 @@ errno_t sysdb_search_object_by_sid(TALLOC_CTX *mem_ctx, const char *sid_str, const char **attrs, struct ldb_result **msg);
+#define SSS_LDB_SEARCH(ret, ldb, mem_ctx, _result, base, scope, attrs, \
exp_fmt, ...) \+{ \
- int lret; \
\- lret = ldb_search(ldb, mem_ctx, _result, base, scope, attrs, exp_fmt, \
##__VA_ARGS__); \- ret = sysdb_error_to_errno(lret); \
- if (ret == EOK && (*_result)->count == 0) { \
ret = ENOENT; \- } \
+} while(0)
Use do { } while(0); for macro, not just { } while(0)
@see macros SYSDB_VERSION_LOWER_ERROR, SYSDB_VERSION_HIGHER_ERROR, DEBUG_PAM_DATA ...
Otherwise patch LGTM. If there are no other objections to this version I will give ACK.
Sorry to come late, but why a macro and not just a function (possibly inlined but doesn't seem necessary to me) ?
https://lists.fedorahosted.org/pipermail/sssd-devel/2014-June/020338.html
LS
On Thu, 2014-08-28 at 15:50 +0200, Lukas Slebodnik wrote:
On (28/08/14 09:48), Simo Sorce wrote:
On Thu, 2014-08-28 at 10:31 +0200, Lukas Slebodnik wrote:
On (11/07/14 17:28), Pavel Reichl wrote:
On Wed, 2014-06-25 at 10:51 -0400, Simo Sorce wrote:
On Tue, 2014-06-24 at 17:28 +0200, Pavel Reichl wrote:
> 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.
Wouldn't it be simpler to use a macro in this case ? Your new fuinction hjas to go through variadic expansion prematurely, and adds a lot of code:
something like: #define sss_ldbn_seartch(ret, arguments, etc., format, ...) { do { int result = ldb_search(argumets, etc, format, ##__VA_ARGS__); ret = result; if (res->count == 0) { ret = ENOENT; } } while(0);
Hello,
sorry for late reply. I guess your approach will definitely save some lines of code and some CPU cycles.
I just must get used to the idea that macros are not considered evil in out environment. :-)
Please see attached patch.
Regards,
Pavel Reichl
From d1293b63adcab4298228af225f3af5803759d34b Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 11 Jul 2014 15:21:59 +0100 Subject: [PATCH] SYSDB: SSS_LDB_SEARCH - macro around ldb_search
This patch amends previous patch 5153e8b9793dea1e212ca08af0f77ea1d023cbb7.
Macro SSS_LDB_SEARCH is used instead of using fuction sss_ldb_search as a wrapper around ldb_search which could lead to premature expansion of variadic parameters.
Part of solution for: https://fedorahosted.org/sssd/ticket/1991
src/db/sysdb.h | 19 +++++++++++++----- src/db/sysdb_ops.c | 51 ------------------------------------------------- src/db/sysdb_search.c | 10 +++++----- src/tests/sysdb-tests.c | 38 ++++++++++++++++++------------------ 4 files changed, 38 insertions(+), 80 deletions(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 17cd5110c9bdafd8d7b18e621188523ed41e5c8a..a613c84ffa9e4ab6ebdc930a8c4876e69aa3cd13 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -481,11 +481,6 @@ 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() */
@@ -864,4 +859,18 @@ errno_t sysdb_search_object_by_sid(TALLOC_CTX *mem_ctx, const char *sid_str, const char **attrs, struct ldb_result **msg);
+#define SSS_LDB_SEARCH(ret, ldb, mem_ctx, _result, base, scope, attrs, \
exp_fmt, ...) \+{ \
- int lret; \
\- lret = ldb_search(ldb, mem_ctx, _result, base, scope, attrs, exp_fmt, \
##__VA_ARGS__); \- ret = sysdb_error_to_errno(lret); \
- if (ret == EOK && (*_result)->count == 0) { \
ret = ENOENT; \- } \
+} while(0)
Use do { } while(0); for macro, not just { } while(0)
@see macros SYSDB_VERSION_LOWER_ERROR, SYSDB_VERSION_HIGHER_ERROR, DEBUG_PAM_DATA ...
Otherwise patch LGTM. If there are no other objections to this version I will give ACK.
Sorry to come late, but why a macro and not just a function (possibly inlined but doesn't seem necessary to me) ?
https://lists.fedorahosted.org/pipermail/sssd-devel/2014-June/020338.html
Lol, ok :-)
Siomo.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Reichl -
Simo Sorce