* [PATCH v2] tools: libxl: make it illegal to pass libxl__realloc(gc) a non-gc ptr
@ 2016-02-11 9:23 Ian Campbell
2016-02-11 11:37 ` Wei Liu
2016-02-18 11:30 ` Ian Campbell
0 siblings, 2 replies; 6+ messages in thread
From: Ian Campbell @ 2016-02-11 9:23 UTC (permalink / raw)
To: ian.jackson, wei.liu2, xen-devel; +Cc: Ian Campbell
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.
It would also be possible to fix this by adding a libxl__ptr_add() at
the same point, however semantically it seems like a programming error
to gc-realloc a pointer which is not associated with the gc in
question, so treat it as such.
Compile tested only, this change could expose latent bugs.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: Check we actually didn't find the ptr in the gc
Correct log message and shorten since LOG will inject the
function name.
---
tools/libxl/libxl_internal.c | 4 ++++
tools/libxl/libxl_internal.h | 4 +++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 328046b..fc81130 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -122,6 +122,10 @@ void *libxl__realloc(libxl__gc *gc, void *ptr, size_t new_size)
break;
}
}
+ if (i == gc->alloc_maxsize) {
+ LOG(CRITICAL, "pointer is not tracked by the given gc");
+ abort();
+ }
}
return new_ptr;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index fc1b558..650a958 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -617,7 +617,9 @@ _hidden void *libxl__zalloc(libxl__gc *gc_opt, size_t size) NN1;
_hidden void *libxl__calloc(libxl__gc *gc_opt, size_t nmemb, size_t size) NN1;
/* change the size of the memory block pointed to by @ptr to @new_size bytes.
* unlike other allocation functions here any additional space between the
- * oldsize and @new_size is not initialised (similar to a gc'd realloc(3)). */
+ * oldsize and @new_size is not initialised (similar to a gc'd realloc(3)).
+ * if @ptr is non-NULL and @gc_opt is not nogc_gc then @ptr must have been
+ * registered with @gc_opt previously. */
_hidden void *libxl__realloc(libxl__gc *gc_opt, void *ptr, size_t new_size) NN1;
/* print @fmt into an allocated string large enoughto contain the result.
* (similar to gc'd asprintf(3)). */
--
2.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] tools: libxl: make it illegal to pass libxl__realloc(gc) a non-gc ptr
2016-02-11 9:23 [PATCH v2] tools: libxl: make it illegal to pass libxl__realloc(gc) a non-gc ptr Ian Campbell
@ 2016-02-11 11:37 ` Wei Liu
2016-02-11 15:38 ` Ian Campbell
2016-02-18 11:30 ` Ian Campbell
1 sibling, 1 reply; 6+ messages in thread
From: Wei Liu @ 2016-02-11 11:37 UTC (permalink / raw)
To: Ian Campbell; +Cc: wei.liu2, ian.jackson, xen-devel
On Thu, Feb 11, 2016 at 09:23:54AM +0000, Ian Campbell wrote:
> That is, if gc is not NOGC and ptr is not NULL then ptr must be
> associated gc.
>
"associated with gc"?
Anyway, I get the idea.
> 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.
>
> It would also be possible to fix this by adding a libxl__ptr_add() at
> the same point, however semantically it seems like a programming error
> to gc-realloc a pointer which is not associated with the gc in
> question, so treat it as such.
>
> Compile tested only, this change could expose latent bugs.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
> ---
> v2: Check we actually didn't find the ptr in the gc
> Correct log message and shorten since LOG will inject the
> function name.
> ---
> tools/libxl/libxl_internal.c | 4 ++++
> tools/libxl/libxl_internal.h | 4 +++-
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index 328046b..fc81130 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -122,6 +122,10 @@ void *libxl__realloc(libxl__gc *gc, void *ptr, size_t new_size)
> break;
> }
> }
> + if (i == gc->alloc_maxsize) {
> + LOG(CRITICAL, "pointer is not tracked by the given gc");
> + abort();
> + }
> }
>
> return new_ptr;
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index fc1b558..650a958 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -617,7 +617,9 @@ _hidden void *libxl__zalloc(libxl__gc *gc_opt, size_t size) NN1;
> _hidden void *libxl__calloc(libxl__gc *gc_opt, size_t nmemb, size_t size) NN1;
> /* change the size of the memory block pointed to by @ptr to @new_size bytes.
> * unlike other allocation functions here any additional space between the
> - * oldsize and @new_size is not initialised (similar to a gc'd realloc(3)). */
> + * oldsize and @new_size is not initialised (similar to a gc'd realloc(3)).
> + * if @ptr is non-NULL and @gc_opt is not nogc_gc then @ptr must have been
> + * registered with @gc_opt previously. */
> _hidden void *libxl__realloc(libxl__gc *gc_opt, void *ptr, size_t new_size) NN1;
> /* print @fmt into an allocated string large enoughto contain the result.
> * (similar to gc'd asprintf(3)). */
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] tools: libxl: make it illegal to pass libxl__realloc(gc) a non-gc ptr
2016-02-11 11:37 ` Wei Liu
@ 2016-02-11 15:38 ` Ian Campbell
0 siblings, 0 replies; 6+ messages in thread
From: Ian Campbell @ 2016-02-11 15:38 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.jackson, xen-devel
On Thu, 2016-02-11 at 11:37 +0000, Wei Liu wrote:
> On Thu, Feb 11, 2016 at 09:23:54AM +0000, Ian Campbell wrote:
> > That is, if gc is not NOGC and ptr is not NULL then ptr must be
> > associated gc.
> >
>
> "associated with gc"?
>
> Anyway, I get the idea.
Yeah, apparently I was having language issues yesterday (v1 had a different
mangling elsewhere)
> > 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.
> >
> > It would also be possible to fix this by adding a libxl__ptr_add() at
> > the same point, however semantically it seems like a programming error
> > to gc-realloc a pointer which is not associated with the gc in
> > question, so treat it as such.
> >
> > Compile tested only, this change could expose latent bugs.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
Thanks. I corrected the above to "with a gc" and applied.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] tools: libxl: make it illegal to pass libxl__realloc(gc) a non-gc ptr
2016-02-11 9:23 [PATCH v2] tools: libxl: make it illegal to pass libxl__realloc(gc) a non-gc ptr Ian Campbell
2016-02-11 11:37 ` Wei Liu
@ 2016-02-18 11:30 ` Ian Campbell
2016-02-18 11:55 ` Andrew Cooper
1 sibling, 1 reply; 6+ messages in thread
From: Ian Campbell @ 2016-02-18 11:30 UTC (permalink / raw)
To: ian.jackson, wei.liu2, xen-devel; +Cc: Andrew Cooper
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.
Ian.
>
> It would also be possible to fix this by adding a libxl__ptr_add() at
> the same point, however semantically it seems like a programming error
> to gc-realloc a pointer which is not associated with the gc in
> question, so treat it as such.
>
> Compile tested only, this change could expose latent bugs.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v2: Check we actually didn't find the ptr in the gc
> Correct log message and shorten since LOG will inject the
> function name.
> ---
> tools/libxl/libxl_internal.c | 4 ++++
> tools/libxl/libxl_internal.h | 4 +++-
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index 328046b..fc81130 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -122,6 +122,10 @@ void *libxl__realloc(libxl__gc *gc, void *ptr,
> size_t new_size)
> break;
> }
> }
> + if (i == gc->alloc_maxsize) {
> + LOG(CRITICAL, "pointer is not tracked by the given gc");
> + abort();
> + }
> }
>
> return new_ptr;
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index fc1b558..650a958 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -617,7 +617,9 @@ _hidden void *libxl__zalloc(libxl__gc *gc_opt, size_t
> size) NN1;
> _hidden void *libxl__calloc(libxl__gc *gc_opt, size_t nmemb, size_t
> size) NN1;
> /* change the size of the memory block pointed to by @ptr to @new_size
> bytes.
> * unlike other allocation functions here any additional space between
> the
> - * oldsize and @new_size is not initialised (similar to a gc'd
> realloc(3)). */
> + * oldsize and @new_size is not initialised (similar to a gc'd
> realloc(3)).
> + * if @ptr is non-NULL and @gc_opt is not nogc_gc then @ptr must have
> been
> + * registered with @gc_opt previously. */
> _hidden void *libxl__realloc(libxl__gc *gc_opt, void *ptr, size_t
> new_size) NN1;
> /* print @fmt into an allocated string large enoughto contain the
> result.
> * (similar to gc'd asprintf(3)). */
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] tools: libxl: make it illegal to pass libxl__realloc(gc) a non-gc ptr
2016-02-18 11:30 ` Ian Campbell
@ 2016-02-18 11:55 ` Andrew Cooper
2016-02-18 12:02 ` Ian Campbell
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2016-02-18 11:55 UTC (permalink / raw)
To: Ian Campbell, ian.jackson, wei.liu2, xen-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] tools: libxl: make it illegal to pass libxl__realloc(gc) a non-gc ptr
2016-02-18 11:55 ` Andrew Cooper
@ 2016-02-18 12:02 ` Ian Campbell
0 siblings, 0 replies; 6+ messages in thread
From: Ian Campbell @ 2016-02-18 12:02 UTC (permalink / raw)
To: Andrew Cooper, ian.jackson, wei.liu2, xen-devel
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@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-02-18 12:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-11 9:23 [PATCH v2] tools: libxl: make it illegal to pass libxl__realloc(gc) a non-gc ptr Ian Campbell
2016-02-11 11:37 ` Wei Liu
2016-02-11 15:38 ` Ian Campbell
2016-02-18 11:30 ` Ian Campbell
2016-02-18 11:55 ` Andrew Cooper
2016-02-18 12:02 ` Ian Campbell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.