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

Pavel Březina pbrezina at redhat.com
Mon Dec 3 15:11:36 UTC 2012


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-failover-Protect-against-empty-host-names.patch
Type: text/x-patch
Size: 16559 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20121203/e6772c87/attachment.bin>


More information about the sssd-devel mailing list