From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] tools: Fix wild memory allocations from c/s 250f0b4 and 85d78b4 Date: Mon, 18 May 2015 15:48:28 +0100 Message-ID: <5559FBBC.7010606@citrix.com> References: <1431953844-32035-1-git-send-email-andrew.cooper3@citrix.com> <5559F09A.8020505@oracle.com> <5559F287.2060202@citrix.com> <5559F86E.30302@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5559F86E.30302@oracle.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: Boris Ostrovsky , Xen-devel Cc: Wei Liu , Ian Jackson , Ian Campbell List-Id: xen-devel@lists.xenproject.org On 18/05/15 15:34, Boris Ostrovsky wrote: > On 05/18/2015 10:09 AM, Andrew Cooper wrote: >> On 18/05/15 15:00, Boris Ostrovsky wrote: >>> On 05/18/2015 08:57 AM, Andrew Cooper wrote: >>>> These changesets cause the respective libxc functions to >>>> unconditonally >>>> dereference their max_cpus/nodes parameters as part of initial memory >>>> allocations. It will fail at obtaining the correct number of >>>> cpus/nodes from >>>> Xen, as the guest handles will not be NULL. >>>> >>>> Signed-off-by: Andrew Cooper >>>> CC: Ian Campbell >>>> CC: Ian Jackson >>>> CC: Wei Liu >>>> CC: Boris Ostrovsky >>>> >>>> --- >>>> Spotted by XenServers Coverity run. >>>> --- >>>> tools/libxl/libxl.c | 4 ++-- >>>> tools/misc/xenpm.c | 4 ++-- >>>> tools/python/xen/lowlevel/xc/xc.c | 4 ++-- >>>> 3 files changed, 6 insertions(+), 6 deletions(-) >>> xenpm bug is already fixed (commit >>> b315cd9cce5b6da7ca89b2d7bad3fb01e7716044 n the staging tree). >>> >>> I am not sure I understand why Coverity complains about other spots. >>> For example, in libxl_get_cpu_topology() num_cpus can be left >>> uninitialized only if xc_cputopoinfo(ctx->xch, &num_cpus, NULL) fails, >>> in which case we go to 'GC_FREE; return ret;', so it's not ever used. >> xc_cputopoinfo(ctx->xch, &num_cpus, NULL) unconditionally dereferences >> and reads &num_cpus, and performs a memory allocation based on the >> result. > > Ah, OK. xc_cputopoinf() (or, rather, the hypervisor) actually doesn't > use the value of dereferenced num_cpus in this case but obviously > Coverity can't know about this. > > So Coverity cross-checks routines to see how callers use the arguments? xc_cputopoinfo(ctx->xch, &num_cpus, NULL) dereferences &num_cpus as part of its DECLARE_HYPERCALL_BUFFER()s. All of this happens before getting anywhere near the hypervisor. ~Andrew