On (29/09/14 07:45), Dmitri Pal wrote:
>On 09/10/2014 11:31 PM, Dmitri Pal wrote:
>>Hello,
>>
>>Extensive travel in recent months allowed me to finish this code.
>>
>>Here is the updated design:
>>https://fedorahosted.org/sssd/wiki/DesignDocs/ding-libs/INIConfigMerge
>>
>>Patches:
>>0001 - Fix in case the Ref array is empty and we need to print/debug it
>>0002 - Declaration of the new function to do access checks
>>0003 - Big patch with core functionality
>>0004 - Updated access check code to use new internal access control
>>function
>>0005 - File with expected output for unit test validation
>>0006 - Makefile and related changes to start building new code
>
>I am attaching a new patch on top of the other patches.
>It can be squashed with patch 3 after review.
>It improves sorting and scanning. See patch comment.
>
>
>>
>>No rush, take your time. :-)
>>
>>
>>
>>_______________________________________________
>>sssd-devel mailing list
>>sssd-devel@lists.fedorahosted.org
>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
>
>--
>Thank you,
>Dmitri Pal
>
>Sr. Engineering Manager IdM portfolio
>Red Hat, Inc.
>
>From aff0f323b2996df7a8f1539f46ca651d2d9fd5e8 Mon Sep 17 00:00:00 2001
>From: Dmitri Pal <d...@dpal.csb>
>Date: Sun, 7 Sep 2014 16:38:19 +0200
>Subject: [PATCH 1/6] [RA] Print info when array is empty
>
>---
> refarray/ref_array.c | 5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)
>
ACK
>From 9f80182fecbf1c1800a22ff279bf8915f4196f42 Mon Sep 17 00:00:00 2001
>From: Dmitri Pal <d...@dpal.csb>
>Date: Sun, 7 Sep 2014 16:39:58 +0200
>Subject: [PATCH 2/6] [INI] Declaring new internal access check function
>
>---
> ini/ini_config_priv.h | 7 +++++++
> 1 files changed, 7 insertions(+), 0 deletions(-)
>
>diff --git a/ini/ini_config_priv.h b/ini/ini_config_priv.h
ACK
>From 8f0209cb995fd88e71e96efb0b98a0256df58d5d Mon Sep 17 00:00:00 2001
>From: Dmitri Pal <d...@dpal.csb>
>Date: Sun, 7 Sep 2014 16:43:31 +0200
>Subject: [PATCH 4/6] [INI] Refactored access control check
>
>The patch includes implementation of the new internal function.
>---
> ini/ini_fileobj.c | 66 +++++++++++++++++++++++++++++++++++++---------------
> 1 files changed, 47 insertions(+), 19 deletions(-)
>
>diff --git a/ini/ini_fileobj.c b/ini/ini_fileobj.c
ACK,
but it would be better to swap order of 3rd patch and this patch (4th)
reason :
* the 2nd patch add declaration to the private header file
* the 3rd patch use this function
* the 4th patch add definition of this function.
>From 09a23b41da6f7a4584f652c4a5a95bb158b0dfe2 Mon Sep 17 00:00:00 2001
>From: Dmitri Pal <d...@dpal.csb>
>Date: Sun, 7 Sep 2014 16:42:00 +0200
>Subject: [PATCH 3/6] [INI] New function to merge snippets
>
>The patch includes the implementation of the function
>and the unit test
>---
> ini/ini_augment.c | 924 ++++++++++++++++++++++++++++++++++++++++++++++++++
> ini/ini_augment_ut.c | 353 +++++++++++++++++++
> 2 files changed, 1277 insertions(+), 0 deletions(-)
> create mode 100644 ini/ini_augment.c
> create mode 100644 ini/ini_augment_ut.c
>
>diff --git a/ini/ini_augment.c b/ini/ini_augment.c
//snip
>+/* Prepare array of regular expressions */
>+static int ini_aug_regex_prepare(const char **patterns,
>+ struct ref_array *ra_err,
>+ struct ref_array **ra_regex)
>+{
//snip
>+ /* Run through the list and save precompiled patterns */
>+ while (*pat) {
>+ TRACE_INFO_STRING("Pattern:", *pat);
>+
>+ preg = calloc(1, sizeof(regex_t));
>+ if (preg == NULL) {
>+ TRACE_ERROR_NUMBER("Failed to create array.", ENOMEM);
>+ ref_array_destroy(ra);
>+ return ENOMEM;
>+ }
>+ reg_err = regcomp(preg, *pat, REG_NOSUB);
>+ if (reg_err) {
>+ /* Get size, allocate buffer, record error... */
>+ buf_size = regerror(reg_err, preg, NULL, 0);
>+ err_str = malloc (buf_size);
>+ if (err_str == NULL) {
>+ TRACE_ERROR_NUMBER("Failed to create array.", ENOMEM);
>+ ref_array_destroy(ra);
>+ free(preg);
>+ return ENOMEM;
>+ }
>+ regerror(reg_err, preg, err_str, buf_size);
>+ free(preg);
>+ ini_aug_add_string(ra_err,
>+ "Failed to process expression: %s."
>+ " Compilation returned error: %s",
>+ *pat, err_str);
>+
>+ /* All error processing is done - advance to next pattern */
>+ pat++;
>+ continue;
// memory allocated in err_str should be released
// before continue.
>+ }
>+ /* In case of no error add compiled expression into the buffer */
>+ error = ref_array_append(ra, (void *)&preg);
>+ if (error) {
>+ TRACE_ERROR_NUMBER("Failed to add element to array.", error);
>+ ref_array_destroy(ra);
>+ free(preg);
>+ return error;
>+ }
>+ /* Advance */
>+ pat++;
>+ }
>+ }
>+
>+ *ra_regex = ra;
>+ /* ref_array_debug(*ra_regex, 1); */
>+
>+ TRACE_FLOW_EXIT();
>+ return EOK;
>+}
>+
//snip
>+
>+/* Construct snippet lists based on the directory */
>+static int ini_aug_construct_list(char *dirname ,
>+ const char **patterns,
>+ struct access_check *check_perm,
>+ struct ref_array *ra_list,
>+ struct ref_array *ra_err)
>+{
>+
>+ int error = EOK;
>+ DIR *dir = NULL;
>+ struct dirent *entry = NULL;
>+ char *snipname = NULL;
>+ char fullname[PATH_MAX + 1] = {0};
>+ struct ref_array *ra_regex = NULL;
>+ bool match = false;
>+
>+ TRACE_FLOW_ENTRY();
>+
>+ /* Prepare patterns */
>+ error = ini_aug_regex_prepare(patterns,
>+ ra_err,
>+ &ra_regex);
>+ if (error) {
>+ TRACE_ERROR_NUMBER("Failed to prepare regex array.", error);
>+ return error;
>+ }
>+
>+ /* Open directory */
>+ errno = 0;
>+ dir = opendir(dirname);
>+ if (!dir) {
>+ error = errno;
>+ if (error == ENOMEM) {
>+ TRACE_ERROR_NUMBER("No memory to open dir.", ENOMEM);
>+ ref_array_destroy(ra_regex);
>+ return ENOMEM;
>+ }
>+ /* Log an error, it is a recoverable error */
>+ add_dir_open_error(error, dirname, ra_err);
//ra_regex should be destroyed here.
>+ return EOK;
>+ }
A directory is opened, but it is not closed in case of errors in next lines.
It would be cause of potential resource leak.
>+
>+ /* Loop through the directory */
>+ while ((entry = readdir(dir)) != NULL)
>+ {
>+ TRACE_INFO_STRING("Processing", entry->d_name);
>+
>+ /* Always skip current and parent dirs */
>+ if ((strncmp(entry->d_name,
>+ INI_CURRENT_DIR,
>+ sizeof(INI_CURRENT_DIR)) == 0) ||
>+ (strncmp(entry->d_name,
>+ INI_PARENT_DIR,
>+ sizeof(INI_PARENT_DIR)) == 0)) continue;
>+
>+ /* Match names */
>+ match = ini_aug_match_name(entry->d_name, ra_regex);
>+
>+ if (match) {
>+
>+ snprintf(fullname, PATH_MAX, "%s/%s", dirname, entry->d_name);
>+
>+ if(ini_check_file_perm(fullname, check_perm, ra_err)) {
>+
>+ /* Dup name and add to the array */
>+ snipname = NULL;
>+ snipname = strdup(fullname);
>+ if (error) {
^^^^^
snipname shouldbe tested here.
>+ TRACE_ERROR_NUMBER("Failed to dup string.", ENOMEM);
>+ ref_array_destroy(ra_regex);
//add closedir(dir) here
>+ return ENOMEM;
>+ }
>+
>+ error = ref_array_append(ra_list, (void *)&snipname);
>+ if (error) {
>+ TRACE_ERROR_NUMBER("No memory to add file to "
>+ "the snippet list.",
>+ ENOMEM);
>+ ref_array_destroy(ra_regex);
//add closedir(dir) here
>+ return ENOMEM;
>+ }
>+ }
>+ }
>+ else {
>+ ini_aug_add_string(ra_err,
>+ "File %s did not match provided patterns."
>+ " Skipping.",
>+ entry->d_name);
>+ }
>+ }
>+
>+ closedir(dir);
>+ ref_array_destroy(ra_regex);
>+
>+ ini_aug_sort_list(ra_list);
>+
>+ TRACE_FLOW_EXIT();
>+ return EOK;
>+}
>From d91b8bd98854a00448c021a42a551e5700a6ab82 Mon Sep 17 00:00:00 2001
>From: Dmitri Pal <d...@dpal.csb>
>Date: Sun, 7 Sep 2014 22:53:19 +0200
>Subject: [PATCH 5/6] [INI] Test file for unit test
>
>---
> ini/ini.d/merge.validator | 60 +++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 60 insertions(+), 0 deletions(-)
> create mode 100644 ini/ini.d/merge.validator
>
>diff --git a/ini/ini.d/merge.validator b/ini/ini.d/merge.validator
>
ACK
>From 9ce3ef526118ee8cf5e5dd8d28ddce971bba606e Mon Sep 17 00:00:00 2001
>From: Dmitri Pal <d...@dpal.csb>
>Date: Sun, 7 Sep 2014 22:55:02 +0200
>Subject: [PATCH 6/6] [INI] Make the merge function build
>
>---
> Makefile.am | 15 +++++-
> ini/ini_configobj.h | 115 +++++++++++++++++++++++++++++++++++++++++++++++++
> ini/libini_config.sym | 1 +
> 3 files changed, 128 insertions(+), 3 deletions(-)
>
>diff --git a/Makefile.am b/Makefile.am
>index 32fcfae..7f38e50 100644
>--- a/Makefile.am
>+++ b/Makefile.am
>@@ -255,6 +255,7 @@ libini_config_la_SOURCES = \
> ini/ini_get_valueobj.c \
> ini/ini_get_array_valueobj.c \
> ini/ini_list_valueobj.c \
>+ ini/ini_augment.c \
> trace/trace.h
> libini_config_la_DEPENDENCIES = ini/libini_config.sym
> libini_config_la_LIBADD = \
>@@ -286,19 +287,23 @@ dist_noinst_DATA += \
> ini/ini.d/real32be.conf \
> ini/ini.d/real32le.conf \
> ini/ini.d/symbols.conf \
>- ini/ini.d/new_line.conf
>+ ini/ini.d/new_line.conf \
>+ ini/ini.d/merge.validator
>
> check_PROGRAMS += \
> ini_config_ut \
> ini_comment_ut \
> ini_valueobj_ut \
>- ini_parse_ut
>+ ini_parse_ut \
>+ ini_augment_ut
>
> TESTS += \
> ini_config_ut \
> ini_comment_ut \
> ini_valueobj_ut \
>- ini_parse_ut
>+ ini_parse_ut \
>+ ini_augment_ut
>+
>
> ini_config_ut_SOURCES = ini/ini_config_ut.c
> ini_config_ut_LDADD = \
>@@ -314,6 +319,9 @@ ini_valueobj_ut_LDADD = libini_config.la libbasicobjects.la
> ini_parse_ut_SOURCES = ini/ini_parse_ut.c
> ini_parse_ut_LDADD = libini_config.la libcollection.la libbasicobjects.la
>
>+ini_augment_ut_SOURCES = ini/ini_augment_ut.c
>+ini_augment_ut_LDADD = libini_config.la libcollection.la libbasicobjects.la
>+
The test ini_augment_ut calls functions from libpath_utils libref_array
therefore it should be linked with this libraries. Otherwise it causes problems
on distributions with disabled lik all deplibs.
This version works. // you can use different style of wrapping long lines.
ini_augment_ut_LDADD = libini_config.la libcollection.la \
libpath_utils.la libref_array.la
> ini_config-docs:
> if HAVE_DOXYGEN
> cd ini; \
>@@ -324,6 +332,7 @@ clean-local-ini_config:
> rm -f ./*.out
> rm -f test.ini
> rm -f ./foo.conf ./bom* #From ini_parse_ut
>+ rm -f ./merge.validator.in #From ini_augment_ut
>
> ##############################################################################
> # Additional rules
//snip
>diff --git a/ini/libini_config.sym b/ini/libini_config.sym
>index 3ca15bb..33d2246 100644
>--- a/ini/libini_config.sym
>+++ b/ini/libini_config.sym
>@@ -62,6 +62,7 @@ global:
> ini_config_parse;
> ini_config_copy;
> ini_config_merge;
>+ ini_config_augment;
> ini_config_set_wrap;
> ini_config_serialize;
> ini_get_section_list;
It works butit is not right solution. The purpose of version symbol file is to
help linker distiguish between two version of libraries with backward
compatible changes. (just new functions were added).
Without version symbol file, linker cannot detect differences due to the same
soname.
sh$ objdump -p .libs/libini_config.so | grep SONAME
SONAME libini_config.so.5
The right approach is to add new version as in refarray/libref_array.sym.
I will attach file which can be squased to your patches.
>From 78c072f8be307fd5a37944274cd509b22aefa5c2 Mon Sep 17 00:00:00 2001
>From: Dmitri Pal <d...@dpal.csb>
>Date: Mon, 29 Sep 2014 07:38:17 -0400
>Subject: [PATCH 7/7] [INI] Improve sorting and scanning
>
>The sort function is replced with locale based one.
>The function to read directory entries is replaced with the reentrant one.
>---
> ini/ini_augment.c | 30 ++++++++++++++++++++++++++----
> 1 files changed, 26 insertions(+), 4 deletions(-)
>
>diff --git a/ini/ini_augment.c b/ini/ini_augment.c
>index 7f59d38..13ab882 100644
>--- a/ini/ini_augment.c
>+++ b/ini/ini_augment.c
>@@ -28,9 +28,11 @@
> #include <dirent.h>
> #include <stdio.h>
> #include <string.h>
>+#include <stddef.h>
> #include <limits.h>
> #include <sys/types.h>
> #include <regex.h>
>+#include <unistd.h>
> #include "trace.h"
> #include "collection.h"
> #include "collection_tools.h"
>@@ -320,9 +322,9 @@ static void ini_aug_sort_list(struct ref_array *ra_list)
> i = k;
> j = k + 1;
>
>- while ((j > 0) && (strncmp(*((char **) ref_array_get(ra_list, i,
>NULL)),
>- *((char **) ref_array_get(ra_list, j,
>NULL)),
>- PATH_MAX) > 0)) {
>+ while ((j > 0) &&
>+ (strcoll(*((char **) ref_array_get(ra_list, i, NULL)),
>+ *((char **) ref_array_get(ra_list, j, NULL)))) > 0) {
> ref_array_swap(ra_list, i, j);
> i--;
> j--;
I should wrote in 3rd patch. This version cause warning in static analysers.
Variable "i" can be underflowed. It is not problem, because "j" will be zero.
On the other hand, difference between "i" and "j" is constant (1). We can
remove one variable.
>@@ -354,10 +356,12 @@ static int ini_aug_construct_list(char *dirname ,
> int error = EOK;
> DIR *dir = NULL;
> struct dirent *entry = NULL;
>+ struct dirent *entryp = NULL;
> char *snipname = NULL;
> char fullname[PATH_MAX + 1] = {0};
> struct ref_array *ra_regex = NULL;
> bool match = false;
>+ int len = 0;
>
> TRACE_FLOW_ENTRY();
>
>@@ -385,9 +389,24 @@ static int ini_aug_construct_list(char *dirname ,
> return EOK;
> }
>
>+ /* Allocate memory for entry (as said in man pages)*/
>+ len = offsetof(struct dirent, d_name) + pathconf(dirname, _PC_NAME_MAX) +
>1;
>+ entry = malloc(len);
^^^^^
//result allocation should be tested here.
In case of failure, directory should be closed and ra_regex destroyed.
>+
> /* Loop through the directory */
>- while ((entry = readdir(dir)) != NULL)
>+ while (true)
> {
>+ error = readdir_r(dir, entry, &entryp);
>+ if (error) {
>+ TRACE_ERROR_NUMBER("Failed to rid directory.", error);
^^^
s/rid/read/ ?
>+ ref_array_destroy(ra_regex);
>+ free(entry);
// directory should be closed here as well. (resource leak)
>+ return error;
>+ }
>+
>+ /* Stop looping if we reached the end */
>+ if (entryp == NULL) break;
>+
> TRACE_INFO_STRING("Processing", entry->d_name);
>
> /* Always skip current and parent dirs */
>@@ -413,6 +432,7 @@ static int ini_aug_construct_list(char *dirname ,
> if (error) {
> TRACE_ERROR_NUMBER("Failed to dup string.", ENOMEM);
> ref_array_destroy(ra_regex);
>+ free(entry);
> return ENOMEM;
> }
>
>@@ -422,6 +442,7 @@ static int ini_aug_construct_list(char *dirname ,
> "the snippet list.",
> ENOMEM);
> ref_array_destroy(ra_regex);
>+ free(entry);
> return ENOMEM;
> }
> }
>@@ -434,6 +455,7 @@ static int ini_aug_construct_list(char *dirname ,
> }
> }
>
>+ free(entry);
> closedir(dir);
> ref_array_destroy(ra_regex);
>
>--
>1.7.1
>
There is also lots of coverity warnings, becaue return value of function
ref_array_get is not tested in this module, but it is tested in other modules.
Error: NULL_RETURNS (CWE-476): [#def1]
ding-libs-0.4.0/ini/ini_augment.c:236: returned_null: "ref_array_get" returns
null.
ding-libs-0.4.0/refarray/ref_array.c:226:9: return_null: Explicitly returning
null.
ding-libs-0.4.0/ini/ini_augment.c:236: dereference: Dereferencing a null
pointer "ref_array_get(ra_regex, i, NULL)".
Error: FORWARD_NULL (CWE-476): [#def2]
ding-libs-0.4.0/ini/ini_augment.c:237: var_deref_model: Passing "NULL" to
"regexec", which dereferences it.
Error: NULL_RETURNS (CWE-476): [#def3]
ding-libs-0.4.0/ini/ini_augment.c:325: returned_null: "ref_array_get" returns
null.
ding-libs-0.4.0/refarray/ref_array.c:226:9: return_null: Explicitly returning
null.
ding-libs-0.4.0/ini/ini_augment.c:325: dereference: Dereferencing a null
pointer "ref_array_get(ra_list, j - 1U, NULL)".
Error: NULL_RETURNS (CWE-476): [#def4]
ding-libs-0.4.0/ini/ini_augment.c:325: returned_null: "ref_array_get" returns
null.
ding-libs-0.4.0/refarray/ref_array.c:226:9: return_null: Explicitly returning
null.
ding-libs-0.4.0/ini/ini_augment.c:325: dereference: Dereferencing a null
pointer "ref_array_get(ra_list, j, NULL)".
Error: CHECKED_RETURN (CWE-252): [#def5]
ding-libs-0.4.0/ini/ini_augment.c:66: check_return: Calling "ref_array_append"
without checking return value (as is done elsewhere 11 out of 12 times).
ding-libs-0.4.0/ini/ini_augment.c:194: example_assign: Example 1: Assigning:
"error" = return value from "ref_array_append(ra, (void *)&preg)".
ding-libs-0.4.0/ini/ini_augment.c:195: example_checked: Example 1 (cont.):
"error" has its value checked in "error".
ding-libs-0.4.0/ini/ini_comment.c:653: example_assign: Example 2: Assigning:
"error" = return value from "ref_array_append(ic->ra, (void *)&sb_new)".
ding-libs-0.4.0/ini/ini_comment.c:654: example_checked: Example 2 (cont.):
"error" has its value checked in "error".
ding-libs-0.4.0/ini/ini_comment.c:280: example_assign: Example 3: Assigning:
"error" = return value from "ref_array_append(ic->ra, (void *)&elem)".
ding-libs-0.4.0/ini/ini_comment.c:281: example_checked: Example 3 (cont.):
"error" has its value checked in "error".
ding-libs-0.4.0/ini/ini_valueobj.c:148: example_assign: Example 4: Assigning:
"error" = return value from "ref_array_append(raw_lines, (void *)©)".
ding-libs-0.4.0/ini/ini_valueobj.c:149: example_checked: Example 4 (cont.):
"error" has its value checked in "error".
ding-libs-0.4.0/ini/ini_valueobj.c:503: example_assign: Example 5: Assigning:
"error" = return value from "ref_array_append(raw_lines, (void *)&strvalue)".
ding-libs-0.4.0/ini/ini_valueobj.c:504: example_checked: Example 5 (cont.):
"error" has its value checked in "error".
Attaching two small promised patches
LS
>From 05713f92013637530dbf1945765b231696a30b51 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lsleb...@redhat.com>
Date: Wed, 8 Oct 2014 09:33:24 +0200
Subject: [PATCH] Fix ini version symbol file
---
ini/libini_config.sym | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/ini/libini_config.sym b/ini/libini_config.sym
index
33d2246bf711fff3be2b37e68d01563ea6261942..2c662c38facaa780a073a69980b6430921e4a34a
100644
--- a/ini/libini_config.sym
+++ b/ini/libini_config.sym
@@ -62,7 +62,6 @@ global:
ini_config_parse;
ini_config_copy;
ini_config_merge;
- ini_config_augment;
ini_config_set_wrap;
ini_config_serialize;
ini_get_section_list;
@@ -136,3 +135,9 @@ global:
local:
*;
};
+
+INI_CONFIG_1.2.0 {
+global:
+ /* ini_configobj.h */
+ ini_config_augment;
+} INI_CONFIG_1.1.0;
--
2.1.0
>From 2b275a62e83363c2ce9a548ee36bcf74cb548063 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lsleb...@redhat.com>
Date: Mon, 6 Oct 2014 16:37:08 +0200
Subject: [PATCH] Fix integre underflow
---
ini/ini_augment.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/ini/ini_augment.c b/ini/ini_augment.c
index
5d1eae8f13deb922e92e2e418e0c9d2fc0112a6b..5c4c572dec706a71139a39e2133b1c0ea8a956c7
100644
--- a/ini/ini_augment.c
+++ b/ini/ini_augment.c
@@ -301,7 +301,7 @@ static bool ini_check_file_perm(char *name,
/* Sort array */
static void ini_aug_sort_list(struct ref_array *ra_list)
{
- unsigned len = 0, i = 0, j = 0, k = 0;
+ unsigned len = 0, j = 0, k = 0;
TRACE_FLOW_ENTRY();
@@ -320,14 +320,12 @@ static void ini_aug_sort_list(struct ref_array *ra_list)
*/
for (k = 0; k < len-1; k++) {
- i = k;
j = k + 1;
- while ((j > 0) &&
- (strcoll(*((char **) ref_array_get(ra_list, i, NULL)),
+ while ((j > 0) &&
+ (strcoll(*((char **) ref_array_get(ra_list, j - 1, NULL)),
*((char **) ref_array_get(ra_list, j, NULL)))) > 0) {
- ref_array_swap(ra_list, i, j);
- i--;
+ ref_array_swap(ra_list, j - 1, j);
j--;
}
}
--
2.1.0
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
