Re: [PATCH 1/1] hvm.c: Prevent gcc uninitialised var warning

[Don Slutz <dslutz@verizon.com>]

On 03/25/15 11:48, Jan Beulich wrote:
>>>> On 25.03.15 at 16:02, <andrew.cooper3@citrix.com> wrote:
>> On 23/03/15 15:01, Don Slutz wrote:
>>> gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3 reports:
>>> ----------------------------------------------------------------------
>>> hvm.c: In function `hvm_create_ioreq_server':
>>> hvm.c:487:18: error: `bufioreq_pfn' may be used uninitialised in this 
>> function
>>> [-Werror=uninitialized]
>>> hvm.c:718:30: note: `bufioreq_pfn' was declared here
>>> ----------------------------------------------------------------------
>>>
>>> My code analysis says that gcc is wrong, but initilize the variable
>>> to prevent the gcc warning.
>>>
>>> Reported-by: Ian Murray <murrayie@yahoo.co.uk>
>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>>
>> GCC is correct and there is path through this function where
>> bufioreq_pfn is used while uninitialised (non-debug build, is_default =
>> 1, handle_bufioreq = 0).
> 

Looks like you are talking about the ASSERT(handle_bufioreq).  But
that does not leave bufioreq_pfn uninitialised.

Currently you cannot get here in that state.  The only way to
have is_default=1 is:


                rc = hvm_create_ioreq_server(d, domid, 1, 1, NULL);

Which should prevent "handle_bufioreq == 0"

> I'm not seeing it: When is_default is set, bufioreq_pfn gets
> initialized in the first if()'s body.
> 
>> As an aside, the compiler is in a very easy position to spot this.  The
>> error means that GCC has positively identified a basic block which does
>> use bufioreq_pfn before it has been initialised.
>>

If the compiler is right, how come the messages says:

may be used

which to me says that it's determination is not 100%


>> I observe that the patch has been committed, but it merely hides the
>> problem and doesn't fix it; hvm_free_ioreq_gmfn(d, bufioreq_pfn) will
>> still clobber arbitrary memory.
> 

hvm_free_ioreq_gmfn() does have issues.  And since it is
possible to call it with

d->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_PFN]

which can be anything.

> I committed the patch based on me not being able to find a path
> where the variable would actually be used uninitialized (and it is
> known that various gcc versions have varying levels of problems
> with correctly identifying such issues). If indeed there is a path
> that I'm not seeing, then I'd indeed favor reverting and putting
> in a proper, backportable fix.
> 

I still cannot find a path where this fails.  However I can
provide a patch that does 2 things:

1) Change the "ASSERT(handle_bufioreq)" to "BUG_ON(handle_bufioreq);"
   (not sure it is required).

2) Add checking to hvm_free_ioreq_gmfn() to prevent memory clobber
   since it's arg may come from an HVM_PARAM.

   -Don Slutz

> Jan
>