Re: [Xen-devel] [PATCH v2] tools: libxl: make it illegal to pass libxl__realloc(gc) a non-gc ptr

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Thu, 2016-02-18 at 11:55 +0000, Andrew Cooper wrote:
> 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.

Which would indicate a lack of locking, except we happen to know that the
gc is thread local, but I bet coverity can't see that, but it means that
gc->alloc_maxsize can't really change behind the back of this code.

Ian.

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