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

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

[Don Slutz]

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>
---

Here is the patch for:

In-Reply-To: <A3CD31A5D207064088A18AC2AF7B5DC6C27A1E9F@MIA20725MBX891B.apps.tmrk.corp>
References: <9AAE0902D5BC7E449B7C8E4E778ABCD0258434BF@AMSPEX01CL01.citrite.net>
             <887729013.1172497.1426540794859.JavaMail.yahoo@mail.yahoo.com>
              <A3CD31A5D207064088A18AC2AF7B5DC6C27A1E9F@MIA20725MBX891B.apps.tmrk.corp>
Subject: Re: [Xen-devel] Compliling Xen 4.5.0 Fails with error:
 =?UTF-8?Q?=E2=80=98bufioreq=5Fpfn=E2=80=99?= may be used uninitialised in
 this function  [-Werror=uninitialized]


 xen/arch/x86/hvm/hvm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 4734d71..dd6b0d0 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -733,7 +733,8 @@ static int hvm_ioreq_server_map_pages(struct hvm_ioreq_server *s,
                                       bool_t is_default, bool_t handle_bufioreq)
 {
     struct domain *d = s->domain;
-    unsigned long ioreq_pfn, bufioreq_pfn;
+    unsigned long ioreq_pfn;
+    unsigned long bufioreq_pfn = ~0UL; /* gcc uninitialised var warning */
     int rc;
 
     if ( is_default )
-- 
1.8.4

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

[Andrew Cooper]

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).

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.

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.

No combination of parameters should be able to cause UB like this.  In
this case, the error handling is incredibly fragile.  All the unmapping
should be based on information in s->{,buf}ioreq which is set upon
successful map, rather than the booleans indicating whether the function
should have set up the mappings.  This allows all the fail$N labels to
collapse into a single, more simple, exit path.

~Andrew

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

[Jan Beulich]

>>> 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).

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.
> 
> 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.

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.

Jan

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

[Don Slutz]

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
> 

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

[Jan Beulich]

>>> On 25.03.15 at 21:03, <dslutz@verizon.com> wrote:
> On 03/25/15 11:48, Jan Beulich wrote:
>>>>> On 25.03.15 at 16:02, <andrew.cooper3@citrix.com> wrote:
>>> 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%

No, that's a wrong interpretation of the wording - it instead means
that there is at least one path where the variable gets initialized,
and at least one where that's not provably the case. As opposed
to a variable being used uninitialized no matter what path leads to
the use site.

Jan

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

[Ian Murray]

----- Original Message -----
> From: Jan Beulich <JBeulich@suse.com>
> To: Don Slutz <dslutz@verizon.com>
> Cc: Ian Murray <murrayie@yahoo.co.uk>; Andrew Cooper <andrew.cooper3@citrix.com>; Keir Fraser <keir@xen.org>; Ian Campbell <ian.campbell@citrix.com>; xen-devel@lists.xen.org
> Sent: Thursday, 26 March 2015, 7:48
> Subject: Re: [Xen-devel] [PATCH 1/1] hvm.c: Prevent gcc uninitialised var	warning
> 
>>>>  On 25.03.15 at 21:03, <dslutz@verizon.com> wrote:
>>  On 03/25/15 11:48, Jan Beulich wrote:
>>>>>>  On 25.03.15 at 16:02, <andrew.cooper3@citrix.com> 
> wrote:
>>>>  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%
> 
> No, that's a wrong interpretation of the wording - it instead means
> that there is at least one path where the variable gets initialized,
> and at least one where that's not provably the case. As opposed
> to a variable being used uninitialized no matter what path leads to
> the use site.

> 


FWIW, I upgraded one of the boxes with the issue to Ubuntu 14.04.2 and gcc no longer complained.


# gcc --version
gcc-4.8.real (Ubuntu 4.8.2-19ubuntu1) 4.8.2



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