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

Simo Sorce simo at redhat.com
Thu Aug 28 14:02:38 UTC 2014


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 at 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.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list