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

Pavel Březina pbrezina at redhat.com
Fri Nov 30 10:35:03 UTC 2012


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
>         },

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.




More information about the sssd-devel mailing list