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

Pavel Březina pbrezina at redhat.com
Wed Jan 2 11:08:31 UTC 2013


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 :-)




More information about the sssd-devel mailing list