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
Hi,
On top of the original 6+1 patches which I did not touch and squash (I
just changed the order of them as suggested by Lukas plus put 7th patch
right after the change to module ini_augment.c)
I added 3 patches:
- 0006-Fixing-the-main.path - addresses all feedback provided above. I
Hope I did not miss anything. I did not squash it so it is easy to see
that things have been fixed.
- 0008-Cleaning-Unit-test.patch - does some cleaning of the code in the
unit test and also makes it not fail in your case by sorting the results
before comparison. See the not in the patch
- 0010-Correction-to-make-file.patch - corrects the make file and fixes
symbols file as Lukas proposed. The only thing that is not addressed is
the recommendation for linking ref_array explicitly. I am not sure I get
it. The library libini_config_la already includes everything needed see
below (line 261 Makefile.am):
libini_config_la_LIBADD = \
libcollection.la \
libpath_utils.la \
libref_array.la \
libbasicobjects.la
Also I did not merge the proposed sorting function patch. I had to
address de-referencing issue to I blended it in first and then add
additional cleanup on top.
Once reviewed the patches 4-5-6, 7-8, 9-10 can be squashed respectfully
before pushing.
Thanks for review!
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IdM portfolio
Red Hat, Inc.