On Jun 24, 2014, at 11:11 AM, Bill Paul <[email protected]> wrote:
> 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 <[email protected]> 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 <[email protected]> >> >> 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]; > I think this is the correct pattern. if (Address == NULL) { return EFI_INVALID_PARAMETER; } Status = GetFvbInstance (Instance, Global, &FwhInstance, Virtual); if (EFI_ERROR (Status)) { *Address = 0; ASSERT_EFI_ERROR (Status); } else { *Address = FwhInstance->FvBase[Virtual]; } return Status; Looks like some one used an ASSERT() in place of error handling in the code. > 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, > [email protected] | 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 > [email protected] > https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ 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 [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
