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

[Matthew Daley <mattjd@xxxxxxxxx>]

On Fri, Sep 27, 2013 at 7:37 AM, Andrew Cooper
<andrew.cooper3@xxxxxxxxxx> wrote:
> 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@xxxxxxxxx>
>>> ---
>>>   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.

Also, we don't have automated scanning set up yet, and this patch is
still in staging anyway.

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

Indeed, I'll send out a patch.

- Matthew

>
> ~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel