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(a)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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel