Re: [Xen-devel] [PATCH] tools: libxl: Simplify logic in libxl__realloc

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

(CC list adjusted)

Ian Campbell writes ("Re: [PATCH] tools: libxl: Simplify logic in 
libxl__realloc"):
> On Fri, 2016-02-19 at 15:34 +0000, Ian Jackson wrote:
> > Replace the loop exit and separate test for loop overrun with an
> > assert in the loop body.
> > 
> > This simplifies the code.  It also (hopefully) avoids Coverity
> > thinking that gc->alloc_maxsize might change, resulting in the loop
> > failing to find the right answer but also failing to abort.
> 
> It succeeded in this regard, but now coverity thinks that
> libxl__gc_is_real(gc) might return false, which I think is reasonable of it
> since it cannot possibly tell from this context if gc is NOGC (and hence
> has alloc_maxsize==0) or not.
> 
> I'm not sure what can be done now, I doubt any kind of check on e.g.
>       gc == &gc->owner->no_gc
> would be something Coverity could reason about either.

Well, I think this is a more general problem.

Let us consider CID 1343307.  Coverity is complaining that
libxl__dm_runas_helper might leak buf because it calls libxl__realloc
to allocate it, and then allows it to go out of scope without freeing
it.

Now we `know' somehow that it is not legal to call
libxl__dm_runas_helper with NOGC.  But if you did so, it would indeed
have this resource leak.

I'm tempted to suggest that we should contrive, somehow, to make
writing such bugs impossible.  But I'm not sure how.


We could separate out libxl__gc into two types.  Let's call them
   libxl__gc        /* as at present but never NOGC */
   libxl__gc_maybe  /* might be a `real' gc or might be NOGC */

But we want to freely turn a libxl__gc into a libxl__gc_maybe.  Maybe
libxl__gc should contain a libxl__gc_maybe.  Then you'd have

  libxl__realloc(libxl__gc_maybe*, ...)

and

  libxl__dm_runas_helper(libxl__gc *gc, ...)
   ...   buf = libxl__realloc(gc.maybe,   ...

I think this might well end up quite clumsy.  But it would eliminate
the possibility of this class of bug (and perhaps allow us to tell
Coverity that a gc.maybe is always real).


Other suggestions welcome.

Ian.

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