[SSSD] [PATCH] failover: Protect against empty host names

Jakub Hrozek jhrozek at redhat.com
Wed Jan 2 16:52:57 UTC 2013


On Wed, Jan 02, 2013 at 12:08:31PM +0100, Pavel Březina wrote:
> On 01/02/2013 12:07 PM, Pavel Březina wrote:
> >On 12/03/2012 04:11 PM, Pavel Březina wrote:
> >>On 12/03/2012 03:02 PM, Michal Židek wrote:
> >>>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
> >>
> >>Hi,
> >>I dared to amend your patch with following changes:
> >>
> >>-            if (substr_len == 0) {
> >>-                list[num_strings] = talloc_strdup(list, "");
> >>-            } else {
> >>-                list[num_strings] = talloc_strndup(list, substr_begin,
> >>-                                                   substr_len);
> >>-            }
> >>+            /* empty string is stored for substr_len == 0 */
> >>+            list[num_strings] = talloc_strndup(list, substr_begin,
> >>substr_len);
> >>              if (list[num_strings] == NULL) {
> >>                  ret = ENOMEM;
> >>                  goto done;
> >>
> >>talloc_strndup(list, substr_begin, 0) == talloc_strdup(list, "")
> >>so we don't need to branch here
> >>
> >>  -107,7 +102,7 @@ int split_on_separator(TALLOC_CTX *mem_ctx, const
> >>char *str,
> >>          /* No allocations were done, make space for the NULL */
> >>          list = talloc(tmp_ctx, char *);
> >>          if (list == NULL) {
> >>-            ret ENOMEM;
> >>+            ret = ENOMEM;
> >>              goto done;
> >>          }
> >>      }
> >>
> >>You've somehow managed to delete = which was present in the previous
> >>iteration.
> >>
> >>I tried this patch with various weird ldap uris and it works splendidly.
> >>
> >>ACK from me
> >
> >Just a remainder. This patch has been waiting for some time.
> 
> s/remainder/reminder :-)

I agree with how you amended Michal's patch. I haven't reviewed the
patch fully, though, I trust your good judgement there :-)

Pushed to master.



More information about the sssd-devel mailing list