On 06/24/14 20:11, Bill Paul wrote:
> There are actually 5 places in FwBlockService.c where FwhInstance isn't
> initialized.
These are the call chains to GetFvbInstance():
FvbProtocolGetPhysicalAddress() -- protocol member
FvbGetPhysicalAddress()
GetFvbInstance()
FvbProtocolGetAttributes() -- protocol member
FvbGetVolumeAttributes()
GetFvbInstance()
FvbProtocolGetBlockSize() -- protocol member
FvbGetLbaAddress()
GetFvbInstance()
FvbProtocolSetAttributes() -- protocol member
FvbSetVolumeAttributes()
GetFvbInstance()
FvbProtocolEraseBlocks() -- protocol member
GetFvbInstance()
Facts:
- The GetFvbInstance() function always conforms to the following
postcondition (note that Instance is an input parameter):
(Instance >= Global->NumFv &&
retval == EFI_INVALID_PARAMETER &&
*FwhInstance is not touched) ||
(Instance < Global->NumFv &&
retval == EFI_SUCCESS &&
*FwhInstance is set)
In English, a valid Instance argument guarantees that you get
EFI_SUCCESS and *FwhInstance gets set; otherwise you get
EFI_INVALID_PARAMETER and *FwhInstance is not touched.
- All five call chains above pass FvbDevice->Instance as Instance. Each
outermost protocol member ensures this. If there's another function
between the outermost protoco member and the inner GetFvbInstance()
function, then the middle function just passes along Instance
unmodified.
- "FvbDevice->Instance" is set in FvbInitialize():
FvbDevice->Instance = mFvbModuleGlobal->NumFv;
mFvbModuleGlobal->NumFv++;
- In the protocol member functions, the FvbDevice pointer is derived
from the This parameter (the protocol interface). It is the protocol
user's responsibility to pass a valid This argument to the member
functions. The CR() macro, underlying the FVB_DEVICE_FROM_THIS()
macro, ASSERT()s in DEBUG builds that the signature matches. An
invalid This argument is a breach of contract.
These particular five callchains rightfully demand that GetFvbInstance()
succeed; a retval different from EFI_SUCCESS Should Never Happen (TM)
and deserves an ASSERT(). And, EFI_SUCCESS implies that *FwhInstance is set.
GCC may not see it, but the ASSERT()s are not laziness; they are
correct. They mean "I'm passing a correct Instance argument; if you fail
then the world has ended".
Shutting up GCC is the right thing to do.
> 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.
This should never happen (assuming that the user of the protocol passes
in the correct protocol address as This, which is something the code is
allowed to assume; hence the ASSERT()s are justified).
> 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.
They are false positives, given the protocol user's responsibilities
(ie. "pass in the valid This argument, then everything will work").
GCC's data flow analysis can't see the above reasoning.
I'm not arguing that it should be able to derive the above argument --
gcc errs on the safe side, but then it's justified for the programmer to
shut it up.
> 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.
Coverity is awesome, but it also generates reports chock full of
impossible cases (which are impossible due to constraints and
responsibilities that are not visible to Coverity).
> 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,
Indeed it isn't. Initializing the pointers is a kludge that shuts up the
false positives of the compiler, so that it doesn't trip us up in these
particular locations, and we can continue to requests its warnings,
because in some cases they are valid. The right fix would be either
enhancing the compiler, or annotating the code with extra information
for the compiler.
I hate such kludges with a passion, but GCC's diagnostics pragmas (which
would allow you to suppress warnings only in specific spots) are not
allowed in edk2. I had suggested to use special values in such kludge
initializers / assignments that communicate clearly that the actual
value is meaningless (eg. (VOID*)0xDEADC0DE rather than NULL). The
proposal was rejected for style reasons.
Thanks
Laszlo
------------------------------------------------------------------------------
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
