Re: [edk2] [PATCH] Fix GCC44 build failure in OVMF platform

Subject: Re: [edk2] [PATCH] Fix GCC44 build failure in OVMF platform

From: Bill Paul <wpaul@windriver.com>

To: edk2-devel@lists.sourceforge.net

Date: 2014-06-24 19:11:04

Of all the gin joints in all the towns in all the world, Jordan Justen had to 
walk into mine at 09:24:20 on Tuesday 24 June 2014 and say:

> On Tue, Jun 24, 2014 at 12:29 AM, Gao, Liming  wrote:
> >  When I use GCC build OVMF IA32 platform, the error that FwhInstance
> >  may
> > 
> > be used uninitialized in function FvbSetVolumeAttributes will be
> > reported in OvmfPkg/QemuFlashFvbServicesRuntimeDxe. This patch fixes it.
> > Please help review it.
> > 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Gao, Liming 
> 
> The code looks good, but I'd like to see a full commit message before
> saying reviewed-by:
> http://tianocore.sourceforge.net/wiki/index.php/Code_Style/Commit_Message
> (I hope the new wiki is working! :)
> 
> Using git with git format-patch + send-email puts the commit message
> and patch into a nicely formatted email for the list. It is also a bit
> easier to code review by just replying directly to the patch contents.
> 
> So long as we're stuck using svn, you might find info on this page
> helpful for using git-svn with EDK II:
> http://tianocore.sourceforge.net/wiki/index.php/EDK_II_git-svn
> 
> -Jordan

From http://people.freebsd.org/~wpaul/edk2/README.txt :

[...]
NOTE: Compilation may halt due to warnings when building the
      edk2/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
      source file.

      On lines 249, 289, 342, 428 and 692, you will see the following
      declaration:

  EFI_FW_VOL_INSTANCE *FwhInstance;

      To correct the warnings, change the these declarations to this:

  EFI_FW_VOL_INSTANCE *FwhInstance = NULL;

      This may be fixed in future versions.
[...]

There are actually 5 places in FwBlockService.c where FwhInstance isn't 
initialized. Whether they all get flagged by the compiler or not depends on 
what version of GCC you use and just how aggressively it chooses to do 
inlining. You can actually defeat the warnings by adding -fno-inline to 
CFLAGS. (GCC only notices that the uninitialized pointers are a problem when 
the smaller functions containing the FwhInstance declaration are inlined into 
other functions.)

However that just shuts up the compiler; I think the correct thing to do is to 
actually clean up the way the code does error handling. Right now everything 
depends on asserts, e.g.:

  //
  // Find the right instance of the FVB private data
  //
  Status = GetFvbInstance (Instance, Global, &FwhInstance, Virtual);
  ASSERT_EFI_ERROR (Status);
  *Address = FwhInstance->FvBase[Virtual];

The problem is that if you decide to do a build with MDEPKG_NDEBUG defined, 
then ASSERT_EFI_ERROR() resolves to a no-op, and then you could end up 
dereferencing a NULL or garbage pointer if GetFvbInstance() fails.

Note that for me this always happens because I do RELEASE builds using the 
UNIXGCC toolchain (the native toolchain on FreeBSD 9.x doesn't quite cut it), 
and as it says in the OVMF README:

[...]
=== UNIXGCC Debug ===

If you build with the UNIXGCC toolchain, then debugging will be disabled
due to larger image sizes being produced by the UNIXGCC toolchain. The
first choice recommendation is to use GCC44 or newer instead.
[...]

And indeed, in OvmfPkgIa32.dsc, it says:

[BuildOptions]
  GCC:*_UNIXGCC_*_CC_FLAGS             = -DMDEPKG_NDEBUG
  GCC:RELEASE_*_*_CC_FLAGS             = -DMDEPKG_NDEBUG
  INTEL:RELEASE_*_*_CC_FLAGS           = /D MDEPKG_NDEBUG
  MSFT:RELEASE_*_*_CC_FLAGS            = /D MDEPKG_NDEBUG
  GCC:*_*_*_CC_FLAGS                   = -mno-mmx -mno-sse

Now, please don't tell me that the fact that the warnings only show on some 
versions of GCC is a compiler problem and I should be using a newer one 
instead. The GCC toolchain generated by the mingw-gcc-build.py works perfectly 
well. It isn't buggy. The warnings are not false positives. Using a newer GCC 
doesn't "fix" the problem: it _hides_ the problem. (And for all you know, a 
future version of GCC might start emitting the warnings too.) I'm pretty sure 
a static analysis tool like Coverity would barf all over this too.

And please also don't tell me: "Hey Bill, that's great, if you could just 
submit a patch..." I'm not submitting a patch. Because just initializing the 
pointers isn't the right fix, and I'm not entirely sure what is, and I'm not 
about to get into a protracted discussion over it. Somebody with a better 
understanding of the FwBlockService.c code should submit a patch instead.

-Bill

-- 
=============================================================================
-Bill Paul            (510) 749-2329 | Senior Member of Technical Staff,
                 wpaul@windriver.com | Master of Unix-Fu - Wind River Systems
=============================================================================
   "I put a dollar in a change machine. Nothing changed." - George Carlin
=============================================================================

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel