[PATCH] tools: libxl: Simplify logic in libxl__realloc

[PATCH] tools: libxl: Simplify logic in libxl__realloc

[Ian Jackson]

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.

(gc->alloc_maxsize can't change because gcs are all singlethreaded:
either they are on the stack of a specific thread, or they belong to
an ao and are covered by the ctx lock.)

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_internal.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index fc81130..e7b765b 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -116,16 +116,13 @@ void *libxl__realloc(libxl__gc *gc, void *ptr, size_t new_size)
     if (ptr == NULL) {
         libxl__ptr_add(gc, new_ptr);
     } else if (new_ptr != ptr && libxl__gc_is_real(gc)) {
-        for (i = 0; i < gc->alloc_maxsize; i++) {
+        for (i = 0; ; i++) {
+            assert(i < gc->alloc_maxsize);
             if (gc->alloc_ptrs[i] == ptr) {
                 gc->alloc_ptrs[i] = new_ptr;
                 break;
             }
         }
-        if (i == gc->alloc_maxsize) {
-            LOG(CRITICAL, "pointer is not tracked by the given gc");
-            abort();
-        }
     }
 
     return new_ptr;
-- 
1.7.10.4

Re: [PATCH] tools: libxl: Simplify logic in libxl__realloc

[Ian Campbell]

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.

> 
> (gc->alloc_maxsize can't change because gcs are all singlethreaded:
> either they are on the stack of a specific thread, or they belong to
> an ao and are covered by the ctx lock.)
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxl/libxl_internal.c |    7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index fc81130..e7b765b 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -116,16 +116,13 @@ void *libxl__realloc(libxl__gc *gc, void *ptr,
> size_t new_size)
>      if (ptr == NULL) {
>          libxl__ptr_add(gc, new_ptr);
>      } else if (new_ptr != ptr && libxl__gc_is_real(gc)) {
> -        for (i = 0; i < gc->alloc_maxsize; i++) {
> +        for (i = 0; ; i++) {
> +            assert(i < gc->alloc_maxsize);
>              if (gc->alloc_ptrs[i] == ptr) {
>                  gc->alloc_ptrs[i] = new_ptr;
>                  break;
>              }
>          }
> -        if (i == gc->alloc_maxsize) {
> -            LOG(CRITICAL, "pointer is not tracked by the given gc");
> -            abort();
> -        }
>      }
>  
>      return new_ptr;

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

Re: [PATCH] tools: libxl: Simplify logic in libxl__realloc

[Ian Jackson]

(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@lists.xen.org
http://lists.xen.org/xen-devel