[SSSD] [ding-libs] Patches for #2267, build issue and tight loop

Lukas Slebodnik lslebodn at redhat.com
Mon Mar 17 21:00:43 UTC 2014


On (08/03/14 19:11), Dmitri Pal wrote:
>Hello,
>
>The following patches address the request to be able to read config
>data from a memory buffer rather than a file.
>Ticket: https://fedorahosted.org/sssd/ticket/2267
>
>It also addresses a build problem that I found on my system and fixes
>a tight loop in parsing when we ignore errors and file is empty.
>
>
>-- 
>Thank you,
>Dmitri Pal
>
>Sr. Engineering Manager for IdM portfolio
>Red Hat Inc.
>
>
>-------------------------------
>Looking to carve out IT costs?
>www.redhat.com/carveoutcosts/
>
>
>

>From 30a92ababaa9148a7354ebf52bf318d37b5451d2 Mon Sep 17 00:00:00 2001
>From: Dmitri Pal <dpal at redhat.com>
>Date: Sat, 8 Mar 2014 19:00:57 -0500
>Subject: [PATCH 2/4] [INI] New entry to read data from mem
>
>The commit adds a new entry point to read configuration
>data from a memory buffer instead of file.
>---
> ini/ini_configobj.h   |   25 ++++++
> ini/ini_fileobj.c     |  204 +++++++++++++++++++++++++++++++++++++------------
> ini/libini_config.sym |    1 +
> 3 files changed, 182 insertions(+), 48 deletions(-)
>
>diff --git a/ini/ini_configobj.h b/ini/ini_configobj.h
>index 9a4eb58..4ef98dc 100644
>--- a/ini/ini_configobj.h
>+++ b/ini/ini_configobj.h
>@@ -483,6 +483,31 @@ int ini_config_file_open(const char *filename,
>                          struct ini_cfgfile **file_ctx);
> 
> /**
>+ * @brief Create a configuration file object using memory buffer.
>+ *
>+ * Create a file object for parsing a configuration file.
>+ * Configuration will use provided memory instead of the actual file.
>+ *
>+ * A "configuration file object" is different from
>+ * a "configuration object". The former stores metadata
>+ * about a file the configuration data is read from,
>+ * while the latter holds the configuration itself.
>+ *
>+ * @param[in]  data_buf         In memory configuration data.
>+ *                              Needs to be NULL terminated.
                                              ^^^^^^^^^^^^^^^^
                            In my opinion, it is little bit confusing.
                            Because argument data_buf has type "void *".
                            What about: byte array null byte ('\0')

Do we really need null byte terminated string?
I think fmemopen will need null byte terminated string only if buffer is opened
in append mode.

>+ * @param[in]  data_len         Length of memory data
>+ *                              not including terminating NULL.
>+ * @param[out] file_ctx         Configuration file object.
>+ *
>+ * @return 0 - Success.
>+ * @return EINVAL - Invalid parameter.
>+ * @return ENOMEM - No memory.
>+ */
>+int ini_config_file_from_mem(void *data_buf,
>+                             uint32_t data_len,
>+                             struct ini_cfgfile **file_ctx);
>+

>+/**
>  * @brief Close configuration file after parsing
>  *
>  * Closes file but keeps the context. File can be reopened
>diff --git a/ini/ini_fileobj.c b/ini/ini_fileobj.c
>index d87956a..fbb1a81 100644
>--- a/ini/ini_fileobj.c
>+++ b/ini/ini_fileobj.c
>@@ -46,6 +46,12 @@ enum index_utf_t {
>     INDEX_UTF8 = 4
> };
> 
>+/* Trick to work around the fact that
>+ * fmemopen failes if buffer is 0 length
>+ */
>+unsigned char alt_buffer[2] = {0, 0};
                                {'\n', 0};
It would be better to have string with length 1.
(gdb) print strlen("\n")
$1 = 1

>+uint32_t alt_buffer_len = 1;
Could you use static modifier (both variable)?

sh-4.2$ nm ini/ini_fileobj.o | grep " D "
0000000000000004 D alt_buffer
0000000000000000 D alt_buffer_len

It is not big problem because symbols will be hidden by version-script file.
But It it will be better to hide them explicitly.

> /* Close file but not destroy the object */
> void ini_config_file_close(struct ini_cfgfile *file_ctx)
> {
>@@ -138,7 +144,7 @@ static enum index_utf_t check_bom(enum index_utf_t ind,
>     return ind;
> }
> 
>-static int read_chunk(int raw_file, size_t left, size_t increment,
>+static int read_chunk(FILE *file, size_t left, size_t increment,
>                       char *position, size_t *read_num)
> {
>     int error = EOK;
>@@ -149,22 +155,28 @@ static int read_chunk(int raw_file, size_t left, size_t increment,
> 
>     to_read = how_much_to_read(left, increment);
> 
>-    TRACE_INFO_NUMBER("About to read", to_read);
>-    errno = 0;
>-    read_cnt = read(raw_file, position, to_read);
>-    if (read_cnt == -1) {
>-        error = errno;
>-        TRACE_ERROR_NUMBER("Failed to read data from file", error);
>-        return error;
>-    }
>+    TRACE_INFO_NUMBER("About to read", (unsigned int)to_read);
                                         ^^^^^^^^^^^^^^
                                casting is not necessary in this case.
                        because in macro TRACE_NUMBER variable is casted to
                                      ((unsigned long int))
> 
>-    if (read_cnt != to_read) {
>-        error = EIO;
>-        TRACE_ERROR_NUMBER("Read less than required", error);
>-        return error;
>+    Read_cnt = fread(position, to_read, 1, file);
>+
>+    TRACE_INFO_NUMBER("Read", (unsigned int)(read_cnt * to_read));
                                ^^^^^^^^^^^^^^
                                the same here.
>+
>+    if (read_cnt == 0) {
>+        error = ferror(file);
>+        if (error) {
>+            TRACE_ERROR_NUMBER("Failed to read data from file", error);
>+            return error;
>+        }
>+        error = feof(file);
>+        if(error) {
>+            TRACE_FLOW_EXIT();
>+            return EOK;
>+        }
It would be better to call feof before ferror.

>+        TRACE_ERROR_NUMBER("Failed to read data from file", EIO);
>+        return EIO;
>     }
> 
>-    *read_num = read_cnt;
>+    *read_num = to_read;
> 
>     TRACE_FLOW_EXIT();
>     return error;
>@@ -229,7 +241,9 @@ static int initialize_conv(unsigned char *read_buf,
> }
> 
> /* Internal conversion part */
>-static int common_file_convert(int raw_file, struct ini_cfgfile *file_ctx)
>+static int common_file_convert(FILE *file,
>+                               struct ini_cfgfile *file_ctx,
>+                               uint32_t size)
> {
>     int error = EOK;
>     size_t read_cnt = 0;
>@@ -249,8 +263,8 @@ static int common_file_convert(int raw_file, struct ini_cfgfile *file_ctx)
> 
>     do {
>         /* print_buffer(read_buf, ICONV_BUFFER); */
>-        error = read_chunk(raw_file,
>-                           file_ctx->file_stats.st_size - total_read,
>+        error = read_chunk(file,
>+                           size - total_read,
>                            ICONV_BUFFER - in_buffer,
>                            read_buf + in_buffer,
>                            &read_cnt);
>@@ -305,9 +319,8 @@ static int common_file_convert(int raw_file, struct ini_cfgfile *file_ctx)
>                     /* We need to just read more if we can */
>                     TRACE_INFO_NUMBER("Incomplete sequence len",
>                                       src - read_buf);
>-                    TRACE_INFO_NUMBER("File size.",
>-                                        file_ctx->file_stats.st_size);
>-                    if (total_read == file_ctx->file_stats.st_size) {
>+                    TRACE_INFO_NUMBER("File size.", size);
>+                    if (total_read == size) {
>                         /* Or return error if we can't */
>                         TRACE_ERROR_NUMBER("Incomplete sequence", error);
>                         iconv_close(conv);
>@@ -356,7 +369,7 @@ static int common_file_convert(int raw_file, struct ini_cfgfile *file_ctx)
>         }
>         while (1);
>     }
>-    while (total_read < file_ctx->file_stats.st_size);
>+    while (total_read < size);
> 
>     iconv_close(conv);
> 
>@@ -365,8 +378,7 @@ static int common_file_convert(int raw_file, struct ini_cfgfile *file_ctx)
>                       (char *)simplebuffer_get_vbuf(file_ctx->file_data));
>     TRACE_INFO_NUMBER("File len",
>                       simplebuffer_get_len(file_ctx->file_data));
>-    TRACE_INFO_NUMBER("Size",
>-                      file_ctx->file_data->size);
>+    TRACE_INFO_NUMBER("Size", size);
>     errno = 0;
>     file_ctx->file = fmemopen(simplebuffer_get_vbuf(file_ctx->file_data),
>                               simplebuffer_get_len(file_ctx->file_data),
>@@ -383,59 +395,98 @@ static int common_file_convert(int raw_file, struct ini_cfgfile *file_ctx)
> 
> 
> /* Internal common initialization part */
>-static int common_file_init(struct ini_cfgfile *file_ctx)
>+static int common_file_init(struct ini_cfgfile *file_ctx,
>+                            void *data_buf,
>+                            uint32_t data_len)
> {
>     int error = EOK;
>-    int raw_file = 0;
>+    FILE *file = NULL;
>     int stat_ret = 0;
>+    uint32_t size = 0;
>+    void *internal_data = NULL;
>+    uint32_t internal_len = 0;
> 
>     TRACE_FLOW_ENTRY();
> 
>-    TRACE_INFO_STRING("File", file_ctx->filename);
>+    if (data_buf) {
> 
>-    /* Open file in binary mode first */
>-    errno = 0;
>-    raw_file = open(file_ctx->filename, O_RDONLY);
>-    if (raw_file == -1) {
>-        error = errno;
>-        TRACE_ERROR_NUMBER("Failed to open file in binary mode", error);
>-        return error;
>+        if(data_len) {
>+            internal_data = data_buf;
>+            internal_len = data_len;
>+        }
>+        else {
>+            /* If buffer is empty fmemopen will return an error.
>+             * This will prevent creation of adefault config object.
>+             * Instead we will use buffer that has at least one character. */
>+            internal_data = (void *)alt_buffer;
                              ^^^^^^^^
                       Any pointer can be stored to variable with type "void *"
                       without explicit casting.

>+            internal_len = alt_buffer_len;
>+        }
>+
>+        TRACE_INFO_NUMBER("Inside file_init len", internal_len);
>+        TRACE_INFO_STRING("Inside file_init data:", (char *)internal_data);
>+
>+        file = fmemopen(internal_data, internal_len, "r");
>+        if (!file) {
>+            error = errno;
>+            TRACE_ERROR_NUMBER("Failed to memmap file", error);
>+            return error;
>+        }
>+        size = internal_len;
>     }
>+    else {
> 
>-    /* Get the size of the file */
>-    errno = 0;
>-    stat_ret = fstat(raw_file, &(file_ctx->file_stats));
>-    if (stat_ret == -1) {
>-        error = errno;
>-        close(raw_file);
>-        TRACE_ERROR_NUMBER("Failed to get file stats", error);
>-        return error;
>+        TRACE_INFO_STRING("File", file_ctx->filename);
>+
>+        /* Open file to get its size */
>+        errno = 0;
>+        file = fopen(file_ctx->filename, "r");
>+        if (!file) {
>+            error = errno;
>+            TRACE_ERROR_NUMBER("Failed to open file", error);
>+            return error;
>+        }
>+
>+        /* Get the size of the file */
>+        errno = 0;
>+        stat_ret = fstat(fileno(file), &(file_ctx->file_stats));
>+        if (stat_ret == -1) {
>+            error = errno;
>+            fclose(file);
>+            TRACE_ERROR_NUMBER("Failed to get file stats", error);
>+            return error;
>+        }
>+        size = file_ctx->file_stats.st_size;
>     }
> 
>     /* Trick to overcome the fact that
>      * fopen and fmemopen behave differently when file
>      * is 0 length
>      */
>-    if (file_ctx->file_stats.st_size) {
>-        error = common_file_convert(raw_file, file_ctx);
>-        close(raw_file);
>+    if (size) {
>+        error = common_file_convert(file, file_ctx, size);
>         if (error) {
>             TRACE_ERROR_NUMBER("Failed to convert file",
>                                 error);
>+            fclose(file);
>             return error;
>         }
>     }
>     else {
>+
>+        TRACE_INFO_STRING("File is 0 length","");
>         errno = 0;
>-        file_ctx->file = fdopen(raw_file, "r");
>+
>+        file_ctx->file = fdopen(fileno(file), "r");
>         if (!(file_ctx->file)) {
>             error = errno;
>-            close(raw_file);
>+            fclose(file);
>             TRACE_ERROR_NUMBER("Failed to fdopen file", error);
>             return error;
>         }
>     }
> 
>+    fclose(file);
>+
>     /* Collect stats */
>     if (file_ctx->metadata_flags & INI_META_STATS) {
>         file_ctx->stats_read = 1;
>@@ -505,7 +556,7 @@ int ini_config_file_open(const char *filename,
>     }
> 
>     /* Do common init */
>-    error = common_file_init(new_ctx);
>+    error = common_file_init(new_ctx, NULL, 0);
>     if(error) {
>         TRACE_ERROR_NUMBER("Failed to do common init", error);
>         ini_config_file_destroy(new_ctx);
>@@ -517,6 +568,63 @@ int ini_config_file_open(const char *filename,
>     return error;
> }
> 
>+/* Create a file object from a memory buffer */
>+int ini_config_file_from_mem(void *data_buf,
>+                             uint32_t data_len,
>+                             struct ini_cfgfile **file_ctx)
>+{
>+    int error = EOK;
>+    struct ini_cfgfile *new_ctx = NULL;
>+
>+    TRACE_FLOW_ENTRY();
>+
>+    if ((!data_buf) || (!file_ctx)) {
>+        TRACE_ERROR_NUMBER("Invalid parameter.", EINVAL);
>+        return EINVAL;
>+    }
>+
>+    /* Allocate structure */
>+    new_ctx = malloc(sizeof(struct ini_cfgfile));
>+    if (!new_ctx) {
>+        TRACE_ERROR_NUMBER("Failed to allocate file ctx.", ENOMEM);
>+        return ENOMEM;
>+    }
>+
>+    new_ctx->filename = NULL;
>+    new_ctx->file = NULL;
>+    new_ctx->file_data = NULL;
>+    new_ctx->metadata_flags = 0;
Could we use calloc instead of malloc.
Memory will be set to zero autmatically. We could remove these lines in this
case.

>+
>+    error = simplebuffer_alloc(&(new_ctx->file_data));
>+    if (error) {
>+        TRACE_ERROR_NUMBER("Failed to allocate buffer ctx.", error);
>+        ini_config_file_destroy(new_ctx);
>+        return error;
>+    }
>+
>+    /* Put an empty string into the file name */
>+    new_ctx->filename = strdup("");
>+    if (!(new_ctx->filename)) {
>+        ini_config_file_destroy(new_ctx);
>+        TRACE_ERROR_NUMBER("Failed to put empty string into filename.", ENOMEM);
>+        return ENOMEM;
>+    }
>+
>+    /* Do common init */
>+    error = common_file_init(new_ctx, data_buf, data_len);
>+    if(error) {
>+        TRACE_ERROR_NUMBER("Failed to do common init", error);
>+        ini_config_file_destroy(new_ctx);
>+        return error;
>+    }
>+
>+    *file_ctx = new_ctx;
>+    TRACE_FLOW_EXIT();
>+    return error;
>+}
>+
>+
>+
> /* Create a file object from existing one */
> int ini_config_file_reopen(struct ini_cfgfile *file_ctx_in,
>                            struct ini_cfgfile **file_ctx_out)
>@@ -564,7 +672,7 @@ int ini_config_file_reopen(struct ini_cfgfile *file_ctx_in,
>     }
> 
>     /* Do common init */
>-    error = common_file_init(new_ctx);
>+    error = common_file_init(new_ctx, NULL, 0);
>     if(error) {
>         TRACE_ERROR_NUMBER("Failed to do common init", error);
>         ini_config_file_destroy(new_ctx);
>diff --git a/ini/libini_config.sym b/ini/libini_config.sym
>index d38b17b..d0035f2 100644
>--- a/ini/libini_config.sym
>+++ b/ini/libini_config.sym
>@@ -46,6 +46,7 @@ global:
>     ini_config_destroy;
>     ini_config_clean_state;
>     ini_config_file_open;
>+    ini_config_file_from_mem;
>     ini_config_file_close;
>     ini_config_file_reopen;
>     ini_config_file_destroy;
>-- 
>1.7.1
>

>From 16c9140ada8461250d2aa8ac484e29c5237fdeaa Mon Sep 17 00:00:00 2001
>From: Dmitri Pal <dpal at redhat.com>
>Date: Sat, 8 Mar 2014 19:04:02 -0500
>Subject: [PATCH 4/4] [INI] Unit test for the new interface
>
>This patch adds unit test for the interface that allows
>reading configuration data from memory.
>---
> ini/ini_parse_ut.c |  154 +++++++++++++++++++++++++++++++++++++++++++++++-----
> 1 files changed, 140 insertions(+), 14 deletions(-)
>
>diff --git a/ini/ini_parse_ut.c b/ini/ini_parse_ut.c
>index 1fa2624..721ee4a 100644
>--- a/ini/ini_parse_ut.c
>+++ b/ini/ini_parse_ut.c
>@@ -50,7 +50,8 @@ typedef int (*test_fn)(void);
> 
> static int test_one_file(const char *in_filename,
>                          const char *out_filename,
>-                         int edge)
>+                         int edge,
>+                         int mem)
    I think type bool would be better. For example: bool from_memory
    In my opinion, it is easier to read code: "if (from_memory)"
    type bool is just a cosmetic change. (it needn't be changed)


> {
>     int error = EOK;
>     struct ini_cfgfile *file_ctx = NULL;
>@@ -60,6 +61,11 @@ static int test_one_file(const char *in_filename,
>     char **error_list = NULL;
>     struct simplebuffer *sbobj = NULL;
>     uint32_t left = 0;
>+    uint32_t stream_len = 0;
>+    void *stream_data = NULL;
>+    struct stat file_stats;
>+    int stat_ret = 0;
>+    FILE *file = NULL;
> 
>     INIOUT(printf("<==== Testing file %s ====>\n", in_filename));
> 
>@@ -70,14 +76,73 @@ static int test_one_file(const char *in_filename,
>         return error;
>     }
> 
>-    error = ini_config_file_open(in_filename,
>-                                 0, /* TBD */
>-                                 &file_ctx);
>-    if (error) {
>-        printf("Failed to open file %s for reading. Error %d.\n",
>-               in_filename, error);
>-        ini_config_destroy(ini_config);
>-        return error;
>+    if(mem) {
      ^^^
You forget to add space between "if" and the bracket.

>+        /* Get file stats */
>+        errno = 0;
>+        stat_ret = stat(in_filename, &file_stats);
>+        if (stat_ret == -1) {
          ^^^^
        with space
>+            error = errno;
>+            printf("Failed to get file stats. Error %d.\n", error);
>+            ini_config_destroy(ini_config);
>+            return error;
>+        }
>+        /* Allocate memory to store file */
>+        errno = 0;
>+        stream_len = file_stats.st_size;
>+        stream_data = malloc(file_stats.st_size + 1);
>+        if(!stream_data) {
          ^^^^
        without space
>+            error = errno;
>+            printf("Failed to allocate memory for stream. Error %d.\n", error);
>+            ini_config_destroy(ini_config);
>+            return error;
>+        }
>+        *((unsigned char *)(stream_data) + stream_len) = '\0';
>+        /* Open file */
>+        errno = 0;
>+        file = fopen(in_filename, "r");
>+        if(!stream_data) {
          ^^^^
        without space
>+            error = errno;
>+            printf("Failed to open file to prepare stream. Error %d.\n", error);
>+            free(stream_data);
>+            ini_config_destroy(ini_config);
>+            return error;
>+        }
>+        /* Read file into memory */
>+        errno = 0;
>+        fread(stream_data, stream_len, 1, file);
>+        error = ferror(file);
>+        if(error) {
          ^^^^
        without space
>+            printf("Failed to read stream data. Error %d.\n", error);
>+            free(stream_data);
>+            fclose(file);
>+            ini_config_destroy(ini_config);
>+            return error;
>+        }
>+        fclose(file);
>+
>+        INIOUT(printf("Data len %u\n", stream_len));
>+        INIOUT(printf("Data:\n%s\n", (char *)stream_data));
>+
>+        error = ini_config_file_from_mem(stream_data,
>+                                         stream_len,
>+                                         &file_ctx);
>+        if (error) {
>+            printf("Failed to open from memory. Error %d.\n", error);
>+            free(stream_data);
>+            ini_config_destroy(ini_config);
>+            return error;
>+        }
>+    }
>+    else {
>+        error = ini_config_file_open(in_filename,
>+                                     0, /* TBD */
>+                                     &file_ctx);
>+        if (error) {
>+            printf("Failed to open file %s for reading. Error %d.\n",
>+                   in_filename, error);
>+            ini_config_destroy(ini_config);
>+            return error;
>+        }
>     }
> 
>     error = ini_config_parse(file_ctx,
>@@ -89,15 +154,29 @@ static int test_one_file(const char *in_filename,
>         INIOUT(printf("Failed to parse configuration. Error %d.\n", error));
> 
>         if (ini_config_error_count(ini_config)) {
>-            INIOUT(printf("Errors detected while parsing: %s\n",
>-                   ini_config_get_filename(file_ctx)));
>+            if (mem) {
>+                INIOUT(printf("Errors detected while parsing"
>+                              " configuration stored in memory: %s\n",
>+                              in_filename));
>+            }
>+            else {
>+                INIOUT(printf("Errors detected while parsing: %s\n",
>+                        ini_config_get_filename(file_ctx)));
>+            }
>             ini_config_get_errors(ini_config, &error_list);
>             INIOUT(ini_config_print_errors(stdout, error_list));
>             ini_config_free_errors(error_list);
>         }
>-        /* We do not return here intentionally */
>+        /* If we are testing memory mapped, return error */
>+        if (mem) {
>+            free(stream_data);
>+            ini_config_file_destroy(file_ctx);
>+            ini_config_destroy(ini_config);
>+            return error;
>+        }
>     }
> 
>+    if (mem) free(stream_data);
>     ini_config_file_destroy(file_ctx);
> 
>     INIOUT(col_debug_collection(ini_config->cfg, COL_TRAVERSE_DEFAULT));
>@@ -200,8 +279,9 @@ static int read_save_test(void)
>             snprintf(infile, PATH_MAX, "%s/ini/ini.d/%s.conf",
>                     (srcdir == NULL) ? "." : srcdir, files[i]);
>             snprintf(outfile, PATH_MAX, "./%s_%d.conf.out", files[i], edge);
>-            error = test_one_file(infile, outfile, edge);
>+            error = test_one_file(infile, outfile, edge, 0);
>             INIOUT(printf("Test for file: %s returned %d\n", files[i], error));
>+            if (error) return error;
>         }
>         i++;
>     }
>@@ -211,6 +291,51 @@ static int read_save_test(void)
>     return EOK;
> }
> 
>+
>+/* Run tests for multiple files */
>+static int read_mem_test(void)
>+{
>+    int error = EOK;
>+    int i = 0;
>+    int edge = 5;
>+    char infile[PATH_MAX];
>+    char outfile[PATH_MAX];
>+    char *srcdir = NULL;
>+    const char *files[] = { "real",
>+                            "mysssd",
>+                            "ipa",
>+                            "test",
>+                            "smerge",
>+                            "real8",
>+                            "real16be",
>+                            "real16le",
>+                            "real32be",
>+                            "real32le",
>+                            "symbols",
I am attaching two files: empty.conf (size is zero)
                          new_line.conf (size is one and contains only '\n'
Could you add them to tests?

>+                            NULL };
>+
>+    INIOUT(printf("<==== Read mem test ====>\n"));
>+
>+    srcdir = getenv("srcdir");
>+
>+    while(files[i]) {
>+        for ( edge = 15; edge < 100; edge +=25) {
>+            snprintf(infile, PATH_MAX, "%s/ini/ini.d/%s.conf",
>+                    (srcdir == NULL) ? "." : srcdir, files[i]);
>+            snprintf(outfile, PATH_MAX, "./%s_%d.conf.mem.out", files[i], edge);
>+            error = test_one_file(infile, outfile, edge, 1);
>+            INIOUT(printf("Test for file: %s returned %d\n", files[i], error));
>+            if ((error) && (strncmp(files[i], "test", 4) != 0)) return error;
>+        }
>+        i++;
>+    }
>+
>+    INIOUT(printf("<==== Read mem test end ====>\n"));
>+
>+    return EOK;
>+}
>+
>+
> /* Run tests for multiple files */
> static int read_again_test(void)
> {
>@@ -241,7 +366,7 @@ static int read_again_test(void)
>             snprintf(infile, PATH_MAX, "./%s_%d.conf.out", files[i], edge);
>             snprintf(outfile, PATH_MAX, "./%s_%d.conf.2.out", files[i], edge);
> 
>-            error = test_one_file(infile, outfile, edge);
>+            error = test_one_file(infile, outfile, edge, 0);
>             INIOUT(printf("Test for file: %s returned %d\n", files[i], error));
>             if (error) break;
>             snprintf(command, PATH_MAX * 3, "diff -q %s %s", infile, outfile);
>@@ -3058,6 +3183,7 @@ int main(int argc, char *argv[])
>     int error = 0;
>     test_fn tests[] = { read_save_test,
>                         read_again_test,
>+                        read_mem_test,
>                         merge_values_test,
>                         merge_section_test,
>                         merge_file_test,
>-- 
>1.7.1
>

>From: Dmitri Pal <dpal at redhat.com>
>Date: Sat, 8 Mar 2014 19:02:29 -0500
>Subject: [PATCH 3/4] [INI] Prevent tight loop
>
>New functionality added revealed a potential tight loop
>with files of the 0 length. This patch addresses this issue.
>---
> ini/ini_parse.c |   24 +++++++++++++++---------
> 1 files changed, 15 insertions(+), 9 deletions(-)
>
>diff --git a/ini/ini_parse.c b/ini/ini_parse.c
>index 7792324..0bab822 100644
>--- a/ini/ini_parse.c
>+++ b/ini/ini_parse.c
>@@ -1534,16 +1534,22 @@ static int parser_error(struct parser_obj *po)
>         else po->ret = EIO;
>     }
>     else if (po->error_level == INI_STOP_ON_NONE) {
>-        action = PARSE_READ;
>-        if (po->ret == 0) {
>-            if (po->last_error & INI_WARNING) po->ret = EILSEQ;
>-            else po->ret = EIO;
>+        if (po->last_error != ERR_READ) {
>+            action = PARSE_READ;
>+            if (po->ret == 0) {
>+                if (po->last_error & INI_WARNING) po->ret = EILSEQ;
>+                else po->ret = EIO;
>+            }
>+            /* It it was warning but now if it is an error
>+             * bump to return code to indicate error. */
>+            else if((po->ret == EILSEQ) &&
>+                    (!(po->last_error & INI_WARNING))) po->ret = EIO;
>+        }
>+        else {
>+            /* Avoid endless loop */
>+            action = PARSE_DONE;
>+            po->ret = EIO;
>         }
>-        /* It it was warning but now if it is an error
>-         * bump to return code to indicate error. */
>-        else if((po->ret == EILSEQ) &&
>-                (!(po->last_error & INI_WARNING))) po->ret = EIO;
>-
>     }
>     else { /* Stop on error */
>         if (po->last_error & INI_WARNING) {

ACK to this patch.

LS
-------------- next part --------------



More information about the sssd-devel mailing list