Re: [PATCH 04/28] libxl: fix libxl_string_list_length and its only caller

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 26/09/13 20:29, Boris Ostrovsky wrote:
> On 09/17/2013 11:37 PM, Matthew Daley wrote:
>> The wrong amount of indirections were being taken in
>> libxl_string_list_length, and its only caller was miscounting the amount
>> of initial non-list arguments, seemingly since the initial commit
>> (599c784).
>>
>> This has been seen and reported in the wild (##xen):
>> < Trixboxer> Hi, any idea why would I get
>> < Trixboxer> xl: libxl_bootloader.c:42: bootloader_arg: Assertion
>> `bl->nargs < bl->argsspace' failed.
>> < Trixboxer> 4.2.2-23.el6
>>
>> Coverity-ID: 1054954
>> Signed-off-by: Matthew Daley<mattjd@gmail.com>
>> ---
>>   tools/libxl/libxl.c            |    2 +-
>>   tools/libxl/libxl_bootloader.c |    2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 0879f23..ca24ca3 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -202,7 +202,7 @@ int libxl_string_list_length(const
>> libxl_string_list *psl)
>>   {
>>       if (!psl) return 0;
>
> This should be
>     if (!psl || !(*psl)) return 0;
>
> We segfault otherwise at the changed line below if no arguments are
> passed to the bootloader (as is the case with pvgrub).
>
>>       int i = 0;
>> -    while (*psl++) i++;
>> +    while ((*psl)[i]) i++;
>>       return i;
>>   }
>
> (I am surprised Coverity didn't flag this)
>
> -boris

For this bug, Coveritys specific objection was that the caller passes a
singleton pointer into this function, and this function uses it as an
array.  This is a side effect of using the wrong indirection.

However, your spot of the crash is good, and needs fixing as well.

~Andrew