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(a)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 <dpal(a)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 <dpal(a)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 <dpal(a)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 <dpal(a)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 <dpal(a)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 <dpal(a)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 <dpal(a)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