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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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