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

Michal Židek mzidek at redhat.com
Fri Nov 30 12:32:39 UTC 2012


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

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


More information about the sssd-devel mailing list