On 12/03/2012 01:35 PM, Pavel Březina wrote:
On 11/30/2012 01:32 PM, Michal Židek wrote:
On 11/30/2012 11:35 AM, Pavel Březina wrote:
On 11/29/2012 05:08 PM, Michal Židek wrote:
On 11/27/2012 03:18 PM, Pavel Březina wrote:
On 11/26/2012 02:40 PM, Michal Židek wrote:
On 11/23/2012 10:11 AM, Pavel Březina wrote:
On 10/17/2012 04:01 PM, Michal Židek wrote:
On 10/16/2012 11:36 AM, Pavel Březina wrote:
On 10/15/2012 12:30 PM, Michal Židek wrote:
Added new parameter to split_on_separator that allows to skip
empty values.
https://fedorahosted.org/sssd/ticket/1484
Patch is in attachment.
Michal
Nack. See below.
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -31,7 +31,8 @@
* the separator is a string, and is case-sensitive.
* optionally single values can be trimmed of of spaces and
tabs */
int split_on_separator(TALLOC_CTX *mem_ctx, const char *str,
- const char sep, bool trim, char ***_list,
int
*size)
+ const char sep, bool trim, bool
skip_empty,
+ char ***_list, int *size)
{
const char *t, *p, *n;
size_t l, len;
@@ -80,6 +81,11 @@ int split_on_separator(TALLOC_CTX *mem_ctx,
const
char *str,
}
if (len == 0) {
+ if (skip_empty) {
+ /* Move to next string and continue */
+ t = n;
+ continue;
+ }
Move this^ condition before talloc_realloc.
I did not like the function at all, it seemed to be written for
different purpose but slightly modified to fit our needs. I wrote a
new
version in this patch and added test for it.
New patch attached.
Michal
Hi,
can you rebase it atop the current master please?
Sure, here is the new patch.
Thanks
Michal
Thanks.
Nack.
Test issues:
+ {
+ ", ,,",
+ (const char*[]){NULL,},
+ true, true,
+ 0, 0
+ },
What is the meaning of the comma after NULL?
It had no meaning. Deleted.
+ for (i = 0; str_ref = sts[a].expected_list[i], str_out =
list[i]; i++) {
+ fail_unless(!strcmp(str_ref, str_out),
+ "Expected:%s Got:%s\n", str_ref, str_out);
+ }
Please use strcmp() == 0.
Function issues:
char **r;
Please, choose a better name for this variable. Something that
explains
its meaning. Maybe 'reallocated_list'?
Done.
/* Trim leading whitespace if needed */
while (trim && isspace(*substr_begin) &&
substr_begin < substr_end) {
substr_begin++;
substr_len--;
}
/* Trim trailing whitespace if needed */
while (trim && substr_end - 1 > substr_begin &&
isspace(*(substr_end-1))) {
substr_end--;
substr_len--;
}
Put the condition on one line. And separate trim from while please.
if (trim) {
while () // leading
while () // trailing
}
if (!(skip_empty && substr_len == 0)) {
if (!skip_empty || substr_len == 0) is more readable in my opinion.
Done.
if (substr_len == 0) {
list[num_strings] = talloc_strdup(list, "");
^
} else {
list[num_strings] = talloc_strndup(list,
substr_begin,
substr_len);
^
}
Indentation...
Done.
talloc_free(list);
return ENOMEM;
Can you use "fail" pattern instead of calling the former each time
allocation fails?
Label "done:" added.
Otherwise it looks good. I like the function.
New patch attached.
Thanks
Michal
Hi,
I have few more nitpicks.
In tests:
{
NULL,
NULL,
false,false,
^ missing comma :P
0, EINVAL
},
I can see only missing space :P
Function:
while (isspace(*substr_begin) &&
substr_begin < substr_end) {
substr_begin++;
substr_len--;
}
/* Trim trailing whitespace */
while (substr_end - 1 > substr_begin &&
isspace(*(substr_end-1))) {
substr_end--;
substr_len--;
}
Can you put those conditions on one line? Both fit into 80 char limit
perfectly and separating them doesn't really help clarity.
reallocated_list = talloc_realloc(mem_ctx, list, char*,
num_strings + 2);
^ indentation :)
and
list = talloc(mem_ctx, char *);
Allocate first on NULL and then talloc_steal() it to mem_ctx. This way
we can catch memory leak. (I know it's obvious there are not any at the
moment, but still...)
And last comment - since the function was completely rewritten, can you
pick either ptr == NULL or !ptr so it is consistent?
I'm sorry, I should have came with these earlier.
Thanks.
New patch attached.
Thanks
Michal
Hi,
we're almost there, but still nack :(
tmp_ctx is always NULL this way, so it doesn't work as a talloc context
at all. You could just use:
talloc_realloc(NULL, list, ...);
talloc_free(list);
or initialize tmp_ctx with talloc_new(NULL).
Yes, that is what I thought I did :-D . I see I didn't.
But I like it better with tmp context after all, because you don't have
to use reallocated_list then and the code is more straightforward.
So you can use:
TALLOC_CTX *tmp_ctx = NULL;
tmp_ctx = talloc_new(NULL);
if (tmp_ctx == NULL) {...}
...
list = talloc_realloc(tmp_ctx, list, char*, num_strings + 2);
if (list == NULL) {
ret = ENOMEM;
goto done;
}
...
done:
talloc_free(tmp_ctx)
You are right, with the tmp context, the reallocated_list is not needed.
New patch attached.
Thanks
Michal
>From cba8418af5163daa21f50388f896b6f271cb46d4 Mon Sep 17 00:00:00 2001
From: Michal Zidek <mzi...@redhat.com>
Date: Mon, 15 Oct 2012 12:21:00 +0200
Subject: [PATCH] failover: Protect against empty host names
Added new parameter to split_on_separator that allows to skip
empty values.
The whole function was rewritten. Unit test case was added to
check the new implementation.
https://fedorahosted.org/sssd/ticket/1484
---
src/confdb/confdb.c | 2 +-
src/providers/ad/ad_common.c | 2 +-
src/providers/ipa/ipa_common.c | 2 +-
src/providers/krb5/krb5_common.c | 2 +-
src/providers/ldap/ldap_common.c | 4 +-
src/providers/ldap/ldap_init.c | 2 +-
src/providers/ldap/sdap_async_sudo_hostinfo.c | 4 +-
src/responder/common/responder_common.c | 3 +-
src/tests/util-tests.c | 91 ++++++++++++++
src/util/util.c | 163 ++++++++++++--------------
src/util/util.h | 3 +-
11 files changed, 177 insertions(+), 101 deletions(-)
diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
index 3707f18..600d423 100644
--- a/src/confdb/confdb.c
+++ b/src/confdb/confdb.c
@@ -599,7 +599,7 @@ int confdb_get_string_as_list(struct confdb_ctx *cdb, TALLOC_CTX *ctx,
goto done;
}
- ret = split_on_separator(ctx, values[0], ',', true, result, NULL);
+ ret = split_on_separator(ctx, values[0], ',', true, true, result, NULL);
done:
talloc_free(values);
diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
index 8600dab..dff1071 100644
--- a/src/providers/ad/ad_common.c
+++ b/src/providers/ad/ad_common.c
@@ -156,7 +156,7 @@ ad_servers_init(TALLOC_CTX *mem_ctx,
if (!tmp_ctx) return ENOMEM;
/* Split the server list */
- ret = split_on_separator(tmp_ctx, servers, ',', true, &list, NULL);
+ ret = split_on_separator(tmp_ctx, servers, ',', true, true, &list, NULL);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to parse server list!\n"));
goto done;
diff --git a/src/providers/ipa/ipa_common.c b/src/providers/ipa/ipa_common.c
index eb384a1..be1bd1d 100644
--- a/src/providers/ipa/ipa_common.c
+++ b/src/providers/ipa/ipa_common.c
@@ -770,7 +770,7 @@ errno_t ipa_servers_init(struct be_ctx *ctx,
}
/* split server parm into a list */
- ret = split_on_separator(tmp_ctx, servers, ',', true, &list, NULL);
+ ret = split_on_separator(tmp_ctx, servers, ',', true, true, &list, NULL);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to parse server list!\n"));
goto done;
diff --git a/src/providers/krb5/krb5_common.c b/src/providers/krb5/krb5_common.c
index ed2fffa..c6865c0 100644
--- a/src/providers/krb5/krb5_common.c
+++ b/src/providers/krb5/krb5_common.c
@@ -486,7 +486,7 @@ errno_t krb5_servers_init(struct be_ctx *ctx,
return ENOMEM;
}
- ret = split_on_separator(tmp_ctx, servers, ',', true, &list, NULL);
+ ret = split_on_separator(tmp_ctx, servers, ',', true, true, &list, NULL);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to parse server list!\n"));
goto done;
diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c
index f8b921a..a97dc34 100644
--- a/src/providers/ldap/ldap_common.c
+++ b/src/providers/ldap/ldap_common.c
@@ -561,7 +561,7 @@ errno_t common_parse_search_base(TALLOC_CTX *mem_ctx,
goto done;
}
- ret = split_on_separator(tmp_ctx, unparsed_base, '?', false,
+ ret = split_on_separator(tmp_ctx, unparsed_base, '?', false, false,
&split_bases, &count);
if (ret != EOK) goto done;
@@ -1214,7 +1214,7 @@ errno_t sdap_urls_init(struct be_ctx *ctx,
/* split server parm into a list */
- ret = split_on_separator(tmp_ctx, urls, ',', true, &list, NULL);
+ ret = split_on_separator(tmp_ctx, urls, ',', true, true, &list, NULL);
if (ret != EOK) {
DEBUG(1, ("Failed to parse server list!\n"));
goto done;
diff --git a/src/providers/ldap/ldap_init.c b/src/providers/ldap/ldap_init.c
index 52bd233..807526b 100644
--- a/src/providers/ldap/ldap_init.c
+++ b/src/providers/ldap/ldap_init.c
@@ -294,7 +294,7 @@ int sssm_ldap_access_init(struct be_ctx *bectx,
order = "filter";
}
- ret = split_on_separator(access_ctx, order, ',', true,
+ ret = split_on_separator(access_ctx, order, ',', true, true,
&order_list, &order_list_len);
if (ret != EOK) {
DEBUG(1, ("split_on_separator failed.\n"));
diff --git a/src/providers/ldap/sdap_async_sudo_hostinfo.c b/src/providers/ldap/sdap_async_sudo_hostinfo.c
index 0a695cd..f47e986 100644
--- a/src/providers/ldap/sdap_async_sudo_hostinfo.c
+++ b/src/providers/ldap/sdap_async_sudo_hostinfo.c
@@ -89,7 +89,7 @@ struct tevent_req * sdap_sudo_get_hostinfo_send(TALLOC_CTX *mem_ctx,
conf_ip_addr = dp_opt_get_string(opts->basic, SDAP_SUDO_IP);
if (conf_hostnames != NULL) {
- ret = split_on_separator(state, conf_hostnames, ' ', true,
+ ret = split_on_separator(state, conf_hostnames, ' ', true, true,
&state->hostnames, NULL);
if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,
@@ -102,7 +102,7 @@ struct tevent_req * sdap_sudo_get_hostinfo_send(TALLOC_CTX *mem_ctx,
}
if (conf_ip_addr != NULL) {
- ret = split_on_separator(state, conf_ip_addr, ' ', true,
+ ret = split_on_separator(state, conf_ip_addr, ' ', true, true,
&state->ip_addr, NULL);
if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,
diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
index 50705a3..8117786 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -167,7 +167,8 @@ errno_t csv_string_to_uid_array(TALLOC_CTX *mem_ctx, const char *cvs_string,
char *endptr;
struct passwd *pwd;
- ret = split_on_separator(mem_ctx, cvs_string, ',', true, &list, &list_size);
+ ret = split_on_separator(mem_ctx, cvs_string, ',', true, false,
+ &list, &list_size);
if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, ("split_on_separator failed [%d][%s].\n",
ret, strerror(ret)));
diff --git a/src/tests/util-tests.c b/src/tests/util-tests.c
index ff809a5..e7e04e6 100644
--- a/src/tests/util-tests.c
+++ b/src/tests/util-tests.c
@@ -718,6 +718,96 @@ START_TEST(test_atomicio_read_from_empty_file)
}
END_TEST
+struct split_data {
+ const char *input;
+ const char **expected_list;
+ bool trim;
+ bool skip_empty;
+ int expected_size;
+ int expected_ret;
+};
+
+START_TEST(test_split_on_separator)
+{
+ TALLOC_CTX *mem = global_talloc_context;
+ errno_t ret;
+ char **list = NULL;
+ int size;
+ const char *str_ref;
+ const char *str_out;
+ int i;
+ int a;
+ int num_of_tests;
+ struct split_data sts[] = {
+ {
+ "one,two,three", /* input string */
+ (const char *[]){"one", "two", "three", NULL}, /* expec. output list */
+ false, false, /* trim, skip_empty */
+ 3, 0 /* expec. size, expec. retval */
+ },
+ {
+ "one,two,three",
+ (const char *[]){"one", "two", "three", NULL},
+ true, true,
+ 3, 0
+ },
+ {
+ " one, two ,three ",
+ (const char*[]){"one", "two", "three", NULL},
+ true, true,
+ 3, 0
+ },
+ {
+ /* If skip empty is false, single comma means "empty,empty" */
+ ",",
+ (const char*[]){"", "", NULL, NULL},
+ false, false,
+ 2, 0
+ },
+ {
+ "one, ,",
+ (const char*[]){"one", " ", "NULL", "NULL"},
+ false, true,
+ 2, 0
+ },
+ {
+ ", ,,",
+ (const char*[]){NULL},
+ true, true,
+ 0, 0
+ },
+ {
+ NULL,
+ NULL,
+ false, false,
+ 0, EINVAL
+ },
+ };
+ num_of_tests = sizeof(sts) / sizeof(struct split_data);
+
+ for (a = 0; a < num_of_tests; a++) {
+ ret = split_on_separator(mem, sts[a].input, ',', sts[a].trim,
+ sts[a].skip_empty, &list, &size);
+
+ fail_unless(ret == sts[a].expected_ret,
+ "split_on_separator failed [%d]: %s\n", ret,
+ strerror(ret));
+ if (ret) {
+ continue;
+ }
+ fail_unless(size == sts[a].expected_size, "Returned wrong size %d "
+ "(expected %d).\n", size, sts[a].expected_size);
+
+ for (i = 0; str_ref = sts[a].expected_list[i], str_out = list[i]; i++) {
+ fail_unless(strcmp(str_ref, str_out) == 0,
+ "Expected:%s Got:%s\n", str_ref, str_out);
+ }
+ talloc_free(list);
+ list = NULL;
+ }
+}
+END_TEST
+
Suite *util_suite(void)
{
Suite *s = suite_create("util");
@@ -733,6 +823,7 @@ Suite *util_suite(void)
tcase_add_test (tc_util, test_parse_args);
tcase_add_test (tc_util, test_add_string_to_list);
tcase_add_test (tc_util, test_string_in_list);
+ tcase_add_test (tc_util, test_split_on_separator);
tcase_set_timeout(tc_util, 60);
TCase *tc_utf8 = tcase_create("utf8");
diff --git a/src/util/util.c b/src/util/util.c
index ab98077..2d1e052 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -27,118 +27,101 @@
#include "util/sss_utf8.h"
#include "dhash.h"
-/* split a string into an allocated array of strings.
- * the separator is a string, and is case-sensitive.
- * optionally single values can be trimmed of of spaces and tabs */
int split_on_separator(TALLOC_CTX *mem_ctx, const char *str,
- const char sep, bool trim, char ***_list, int *size)
+ const char sep, bool trim, bool skip_empty,
+ char ***_list, int *size)
{
- const char *t, *p, *n;
- size_t l, len;
- char **list, **r;
- const char sep_str[2] = { sep, '\0'};
-
- if (!str || !*str || !_list) return EINVAL;
-
- t = str;
+ int ret;
+ const char *substr_end = str;
+ const char *substr_begin = str;
+ const char *sep_pos = NULL;
+ size_t substr_len;
+ char **list = NULL;
+ int num_strings = 0;
+ TALLOC_CTX *tmp_ctx = NULL;
+
+ if (str == NULL || *str == '\0' || _list == NULL) {
+ return EINVAL;
+ }
- list = NULL;
- l = 0;
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ return ENOMEM;
+ }
- /* trim leading whitespace */
- if (trim)
- while (isspace(*t)) t++;
+ do {
+ substr_len = 0;
- /* find substrings separated by the separator */
- while (t && (p = strpbrk(t, sep_str))) {
- len = p - t;
- n = p + 1; /* save next string starting point */
- if (trim) {
- /* strip whitespace after the separator
- * so it's not in the next token */
- while (isspace(*t)) {
- t++;
- len--;
- if (len == 0) break;
- }
- p--;
- /* strip whitespace before the separator
- * so it's not in the current token */
- while (len > 0 && (isspace(*p))) {
- len--;
- p--;
- }
+ /* If this is not the first substring, then move from the separator. */
+ if (sep_pos != NULL) {
+ substr_end = sep_pos + 1;
+ substr_begin = sep_pos + 1;
}
- /* Add the token to the array, +2 b/c of the trailing NULL */
- r = talloc_realloc(mem_ctx, list, char *, l + 2);
- if (!r) {
- talloc_free(list);
- return ENOMEM;
- } else {
- list = r;
+ /* Find end of the first substring */
+ while (*substr_end != sep && *substr_end != '\0') {
+ substr_end++;
+ substr_len++;
}
- if (len == 0) {
- list[l] = talloc_strdup(list, "");
- } else {
- list[l] = talloc_strndup(list, t, len);
- }
- if (!list[l]) {
- talloc_free(list);
- return ENOMEM;
- }
- l++;
+ sep_pos = substr_end;
- t = n; /* move to next string */
- }
+ if (trim) {
+ /* Trim leading whitespace */
+ while (isspace(*substr_begin) && substr_begin < substr_end) {
+ substr_begin++;
+ substr_len--;
+ }
- /* Handle the last remaining token */
- if (t) {
- r = talloc_realloc(mem_ctx, list, char *, l + 2);
- if (!r) {
- talloc_free(list);
- return ENOMEM;
- } else {
- list = r;
+ /* Trim trailing whitespace */
+ while (substr_end - 1 > substr_begin && isspace(*(substr_end-1))) {
+ substr_end--;
+ substr_len--;
+ }
}
- if (trim) {
- /* trim leading whitespace */
- len = strlen(t);
- while (isspace(*t)) {
- t++;
- len--;
- if (len == 0) break;
- }
- /* trim trailing whitespace */
- p = t + len - 1;
- while (len > 0 && (isspace(*p))) {
- len--;
- p--;
+ /* Copy the substring to the output list of strings */
+ if (skip_empty == false || substr_len > 0) {
+ list = talloc_realloc(tmp_ctx, list, char*, num_strings + 2);
+ if (list == NULL) {
+ ret = ENOMEM;
+ goto done;
}
- if (len == 0) {
- list[l] = talloc_strdup(list, "");
+ if (substr_len == 0) {
+ list[num_strings] = talloc_strdup(list, "");
} else {
- list[l] = talloc_strndup(list, t, len);
+ list[num_strings] = talloc_strndup(list, substr_begin,
+ substr_len);
}
- } else {
- list[l] = talloc_strdup(list, t);
+ if (list[num_strings] == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+ num_strings++;
}
- if (!list[l]) {
- talloc_free(list);
- return ENOMEM;
+
+ } while (*sep_pos != '\0');
+
+ if (list == NULL) {
+ /* No allocations were done, make space for the NULL */
+ list = talloc(tmp_ctx, char *);
+ if (list == NULL) {
+ ret ENOMEM;
+ goto done;
}
- l++;
}
+ list[num_strings] = NULL;
- list[l] = NULL; /* terminate list */
-
- if (size) *size = l;
- *_list = list;
+ if (size) {
+ *size = num_strings;
+ }
- return EOK;
+ *_list = talloc_steal(mem_ctx, list);
+ ret = EOK;
+done:
+ talloc_free(tmp_ctx);
+ return ret;
}
static void free_args(char **args)
diff --git a/src/util/util.h b/src/util/util.h
index c15ca66..e4cb1a8 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -476,7 +476,8 @@ errno_t check_and_open_readonly(const char *filename, int *fd, const uid_t uid,
/* from util.c */
int split_on_separator(TALLOC_CTX *mem_ctx, const char *str,
- const char sep, bool trim, char ***_list, int *size);
+ const char sep, bool trim, bool skip_empty,
+ char ***_list, int *size);
char **parse_args(const char *str);
--
1.7.11.2
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
