On (19/04/16 11:41), Michal Židek wrote:
On 04/13/2016 04:59 PM, Michal Židek wrote:
Hello!
See patches in attachment that are part of solution for https://fedorahosted.org/sssd/ticket/2751
There are few ding libs patches. The first is unrelated, but I just added some missing trace info.
The second is a patch for new parser flag that Dmitri wrote in discussion for ticket https://fedorahosted.org/sssd/ticket/2986
I modified the patch a little so I can not ack it (the check was necessary on one more place, so I added it there).
The third ding libs patch ads unit test for the new flag. I decided to add a new check based test rather than adding the test to ini_parse_ut.c. So it is the only test in the suite for now.
There is also a patch for SSSD attached that can take advantage of this flag to solve GPO issues once the new version of ding libs is released.
Michal
I forgot to put ticket link to description of the ding-libs patches. New patches attached. Codewise the patches are the same.
Michal
From 5d97907f4aba09838d33e31ab85f7bd2adf28024 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 13 Apr 2016 11:10:22 +0200 Subject: [PATCH 1/3] ini_parse: Add missing TRACE_FLOW_EXIT
Some TRACE_FLOW_EXIT macros were missing in handle_kvp() before return.
ini/ini_parse.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/ini/ini_parse.c b/ini/ini_parse.c index 91525a0..0de4e35 100644 --- a/ini/ini_parse.c +++ b/ini/ini_parse.c
LGTM
From 235156822079a6c68c7fa8e2ff13a1fa5803cfa7 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@redhat.com Date: Wed, 13 Apr 2016 11:44:44 +0200 Subject: [PATCH 2/3] ini: Add INI_PARSE_IGNORE_NON_KVP flag
Ticket: https://fedorahosted.org/sssd/ticket/2751
Add INI_PARSE_IGNORE_NON_KVP flag to allow skipping lines that are not key-value pairs.
ini/ini_configobj.h | 2 ++ ini/ini_parse.c | 29 ++++++++++++++++++++++++----- 2 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/ini/ini_configobj.h b/ini/ini_configobj.h index 6f2d692..12920db 100644 --- a/ini/ini_configobj.h +++ b/ini/ini_configobj.h @@ -363,6 +363,8 @@ enum ERR_PARSE { #define INI_PARSE_NOTAB 0x0004 /** @brief Do not allow C-style comments */ #define INI_PARSE_NO_C_COMMENTS 0x0008 +/** @brief Skip lines that are not KVPs */ +#define INI_PARSE_IGNORE_NON_KVP 0x0010
/**
- @}
diff --git a/ini/ini_parse.c b/ini/ini_parse.c index 0de4e35..e5baeca 100644 --- a/ini/ini_parse.c +++ b/ini/ini_parse.c @@ -966,8 +966,18 @@ static int handle_kvp(struct parser_obj *po, uint32_t *action) /* Check if we have the key */ if (*(str) == '=') { TRACE_ERROR_STRING("No key", str);
po->last_error = ERR_NOKEY;*action = PARSE_ERROR;
if (po->parse_flags & INI_PARSE_IGNORE_NON_KVP) {/* Clean everything as if nothing happened */free(po->last_read);po->last_read = NULL;po->last_read_len = 0;*action = PARSE_READ;} else {po->last_error = ERR_NOKEY;*action = PARSE_ERROR;}TRACE_FLOW_EXIT(); return EOK;@@ -975,9 +985,18 @@ static int handle_kvp(struct parser_obj *po, uint32_t *action) /* Find "=" */ eq = strchr(str, '='); if (eq == NULL) {
TRACE_ERROR_STRING("No equal sign", str);po->last_error = ERR_NOEQUAL;*action = PARSE_ERROR;
if (po->parse_flags & INI_PARSE_IGNORE_NON_KVP) {/* Clean everything as if nothing happened */free(po->last_read);po->last_read = NULL;po->last_read_len = 0;*action = PARSE_READ;} else {TRACE_ERROR_STRING("No equal sign", str);po->last_error = ERR_NOEQUAL;*action = PARSE_ERROR;}TRACE_FLOW_EXIT(); return EOK;-- 2.5.0
From 0d4ae5f1a4b062b28305226e44df1c3d1886ac17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 13 Apr 2016 15:57:34 +0200 Subject: [PATCH 3/3] Add unit test for INI_PARSE_IGNORE_NON_KVP
Ticket: https://fedorahosted.org/sssd/ticket/2751
Makefile.am | 10 +++- ini/ini_parse_ut_check.c | 153 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 2 deletions(-) create mode 100644 ini/ini_parse_ut_check.c
diff --git a/Makefile.am b/Makefile.am index 5b1da33..c162580 100644 --- a/Makefile.am +++ b/Makefile.am @@ -69,8 +69,8 @@ libpath_utils_la_LDFLAGS = \ -Wl,--version-script=$(top_srcdir)/path_utils/libpath_utils.sym
if HAVE_CHECK
- check_PROGRAMS += path_utils_ut ini_configmod_ut_check
- TESTS += path_utils_ut ini_configmod_ut_check
- check_PROGRAMS += path_utils_ut ini_configmod_ut_check ini_parse_ut_check
- TESTS += path_utils_ut ini_configmod_ut_check ini_parse_ut_check
It might be a good idea to use the same approach as in sssd one test paer line + $(NULL) at the end of list
BTW thank you for unit test.
From e41fb64fb2a0f326095e9712c62b1b9f9ce4782e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= mzidek@redhat.com Date: Wed, 13 Apr 2016 16:46:24 +0200 Subject: [PATCH] GPO: ignore non-KVP lines if possible
Ticket: https://fedorahosted.org/sssd/ticket/2751
Non-KVP break GPO processing. They are used for values we are not interested in so it is safe to ignore them.
src/providers/ad/ad_gpo.c | 3 ++- src/providers/ad/ad_gpo_child.c | 3 ++- src/util/util.h | 5 +++++ 3 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c index 3bd9ab0..859c5df 100644 --- a/src/providers/ad/ad_gpo.c +++ b/src/providers/ad/ad_gpo.c @@ -1136,7 +1136,8 @@ ad_gpo_store_policy_settings(struct sss_domain_info *domain, goto done; }
- ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0, 0, ini_config);
- ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0,
INI_PARSE_IGNORE_NON_KVP, ini_config);
It will fix the bug with #2751. But we will ignore parsing errors in all sections. and I worry about security bugs due to deny rules.
IIRC Sumit proposed to ignore such failures only in special sections or to parse only special section for GPO. It does not meand that ding-libs patches are not useful.
Other opinions are welcomed.
diff --git a/src/providers/ad/ad_gpo_child.c b/src/providers/ad/ad_gpo_child.c index e37abe3..fcb8dca 100644 --- a/src/providers/ad/ad_gpo_child.c +++ b/src/providers/ad/ad_gpo_child.c @@ -469,7 +469,8 @@ ad_gpo_parse_ini_file(const char *smb_path, goto done; }
- ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0, 0, ini_config);
- ret = ini_config_parse(file_ctx, INI_STOP_ON_NONE, 0,
INI_PARSE_IGNORE_NON_KVP, ini_config);diff --git a/src/util/util.h b/src/util/util.h index 05ee875..f070a84 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -96,6 +96,11 @@ #endif #endif
+/* If INI_PARSE_IGNORE_NON_KVP is not defined, use 0 (no effect) */ +#ifndef INI_PARSE_IGNORE_NON_KVP +#define INI_PARSE_IGNORE_NON_KVP 0 +#endif
If we decide to use approach with INI_PARSE_IGNORE_NON_KVP Then I would rather fail if we cannot detect INI_PARSE_IGNORE_NON_KVP. or at least print warning. (the same as with robust mutexes)
$git grep -n "#warning" src/sss_client/common.c:1101:#warning Robust mutexes are not supported on this platform. src/sss_client/common.c:1113:#warning Robust mutexes are not supported on this platform.
Or rather mlso move this to gpo code. We needn't print a warning if sssd is built without samba.
LS