From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v2] tools: libxl: make it illegal to pass libxl__realloc(gc) a non-gc ptr Date: Thu, 18 Feb 2016 11:55:01 +0000 Message-ID: <56C5B115.5040508@citrix.com> References: <1455182634-19386-1-git-send-email-ian.campbell@citrix.com> <1455795020.6225.38.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1455795020.6225.38.camel@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: Ian Campbell , ian.jackson@eu.citrix.com, wei.liu2@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 18/02/16 11:30, Ian Campbell wrote: > On Thu, 2016-02-11 at 09:23 +0000, Ian Campbell wrote: >> That is, if gc is not NOGC and ptr is not NULL then ptr must be >> associated gc. >> >> Currently in this case the new_ptr would not be registered with any >> gc, which Coverity rightly points out (in various different places) >> would be a memory leak. > This change wasn't sufficient to placate Coverity. I think it's analysis > now is a false positive, see e.g CID 1343307, but a second opinion would be > appreciated. > > It doesn't seem to spot that this loop: > for (i = 0; i < gc->alloc_maxsize; i++) { > if (gc->alloc_ptrs[i] == ptr) { > gc->alloc_ptrs[i] = new_ptr; > break; > } > } > either exits with i < gc->alloc_maxsize having updated alloc_ptrs or exits > with i == gc->alloc_maxsize. > > Having exited the loop with "Condition i < gc->alloc_maxsize, taking false > branch" it then does "Condition i == gc->alloc_maxsize, taking false > branch", avoiding the assert (which I had hoped would be sufficient to > quash the issue). Presumably it either cannot track the possible values of > i at all or it considers the possibility that i > gc->alloc_maxsize might > be true here. > > Perhaps changing the test to i >= gc->alloc_maxsize might be enough of a > hint to it? Or maybe asserting "i<=gc->alloc_maxsize" after the loop? > > Or maybe this really requires modelling? > > Unfortunately the actual CIDs are reported in the callers of libxl__realloc > and determining for sure that each such issue is indeed down to this mis- > analysis of the behaviour of libxl__realloc is not entirely trivial. If the compiler didn't pull gc->alloc_maxsize into a local variable, it is quite possible that the two reads would observe different values. ~Andrew