From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matthew Daley Subject: Re: [PATCH 04/28] libxl: fix libxl_string_list_length and its only caller Date: Fri, 27 Sep 2013 12:46:05 +1200 Message-ID: References: <1379475484-25993-1-git-send-email-mattjd@gmail.com> <1379475484-25993-5-git-send-email-mattjd@gmail.com> <52448B0A.6030808@oracle.com> <52448CEB.9050109@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <52448CEB.9050109@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: Ian Campbell , Stefano Stabellini , Ian Jackson , "xen-devel@lists.xen.org" , David Vrabel , Boris Ostrovsky List-Id: xen-devel@lists.xenproject.org On Fri, Sep 27, 2013 at 7:37 AM, Andrew Cooper 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 >>> --- >>> 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