Re: [PATCH] Initialize m_hFile in FileMapping constructor.

On 03/22/2012 10:32 PM, Catalin Iacob wrote:
> From 78d778a79cfe2292d03ac30bee825147cc56ead9 Mon Sep 17 00:00:00 2001
> From: Catalin Iacob<iacobcatalin@gmail.com>
> Date: Wed, 14 Mar 2012 23:35:49 +0100
> Subject: [PATCH] Initialize m_hFile in FileMapping constructor.
>
> GCC gives the following warning which breaks compilation when using --enable-werror:
> lockbyte.cxx: In function 'storeError 
> store::FileLockBytes_createInstance(rtl::Reference<store::ILockBytes>&, rtl_uString*, 
> storeAccessMode)':
> lockbyte.css:512:37: error: 'prephitmp.221' may be used uninitialized in this function 
> [-Werror=uninitialized]
> lockbyte.cxx:906:1: note: 'prephitmp.221' was declared here

That looks like a compiler error.  Have you tried with a plain 
(non-OpenSUSE) GCC 4.6.2, too?  (What's also odd is the mention of 
"lockbyte.css" -- is that a typo of yours, or is that a temporary file 
that specific compiler generates internally?)

> It's not clear from GCC's message, but what it warns about is
> FileMapping::m_hFile. This is because of the following sequence:
> * xMapping.release() makes xMapping.m_value be a default constructed
>    FileMapping
> * the xMapping local variable in store::FileLockBytes_createInstance
>    gets destructed
> * ~ResourceHolder() calls ResourceHolder::reset
> * ResourceHolder::reset() calls FileMapping::UnmapFile::operator()
>    passing m_value as rMapping

...but only if tmp != value, which is not the case here, as both tmp and 
value are default-constructed.  And FileMapping::operator!= is careful 
not to use the potentially uninitialized m_hFile.

> * FileMapping::UnmapFile::operator() uses rMapping.m_hFile but
>    rMapping is a default constructed FileMapping and therefore has
>    m_hFile uninitialized
> ---
>   store/source/lockbyte.cxx |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/store/source/lockbyte.cxx b/store/source/lockbyte.cxx
> index e28bffe..1a8bf81 100644
> --- a/store/source/lockbyte.cxx
> +++ b/store/source/lockbyte.cxx
> @@ -478,7 +478,7 @@ struct FileMapping
>       sal_uInt32  m_nSize;
>       oslFileHandle m_hFile;
>
> -    FileMapping() : m_pAddr(0), m_nSize(0) {}
> +    FileMapping() : m_pAddr(0), m_nSize(0), m_hFile() {}

...but always initializing m_hFile is probably not a bad idea, anyway. 
And if it helps your compiler, all the better.  ;)  (Though I would 
prefer an explicit m_hFile(0) there, esp. given the other members do not 
rely on implicit value-initialization, either.)

So pushed this now.

Thanks,
Stephan