Qiu, Shumin [mailto:shumin....@intel.com] wrote:
Hi Andrew,
ASSERT is here to make sure developer will not change code to return a NULL
pointer. In release it has no impact as HiiGetString can always return proper
string back or the build will fail.
-Shumin
Hello Shumin,
I don't understand your comment,
HiiGetString can always return proper string back or the build
will fail.
That statement seems optimistic. HiiGetString allocates memory and memory
allocation can fail. Maybe it doesn't fail in the QA lab, but it could
conceivably fail after someone runs a shell app that leaks memory for example.
If the HiiGetString calls in UefiHandleParsingLib.c fail (return NULL), a hang
requiring a reboot is all but certain. Asserts don't change that. On the other
hand, proper error handling might allow the system to keep running.
The real problem I see is a lack of EDK2 policy on error handling. Nowhere is
it stated that return values must be checked and handled in a way that allows
the system to keep running. Why is this? It may be because those who wrote the
EDK2 documents consider proper error handling so essential and undebatable that
it doesn't need to be stated. An analogy is the unwritten policy on freeing
allocated memory. If my code allocates memory and never frees it, I can argue
that it is OK. I can say the allocation is small and infrequent, so there is no
need to free it. While EDK2 has no written policy about freeing allocated
memory (that I know of), I think many engineers will object to my claim that
freeing the memory is unneeded.
Looking at EDK2 code shows that there is disagreement about handling system
call fails. Some code skips error checking altogether. Other code uses ASSERT
only. Some uses error handling only. Some uses both ASSERT and error handling.
To me, the best solution is to include error recovery for all system call
failures (and to free allocated memory). ASSERT could be optional. But in any
case, it is clear an official policy is needed.
Thanks,
Scott
From: Andrew Fish [mailto:af...@apple.com]
Sent: Wednesday, December 03, 2014 11:09 AM
To: edk2-devel@lists.sourceforge.net <mailto:edk2-devel@lists.sourceforge.net>
Subject: Re: [edk2] [Patch] ShellPkg: Add ASSERT to check pointer to avoid
being dereferenced
On Dec 2, 2014, at 6:56 PM, Qiu, Shumin <shumin....@intel.com
<mailto:shumin....@intel.com> > wrote:
Hi Jaben,
Could you help review the patch? This patch adds ‘ASSERT’ to check the pointer
returned from HiiGetString to avoid pointer dereferenced.
Why are we adding ASSERT to check pointers. It is likely turned off in release
builds. ASSERTs are a debugging aid, not a robustness check.
The shell is an application that runs on a large number of system so it should
handle the errors in C code, and it could also ASSERT() on a debug build.
Thanks,
Andrew Fish
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Qiu Shumin < <mailto:shumin....@intel.com> shumin....@intel.com>
Thanks
Shumin
<UefiHandleParsingLib.c.patch>------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
<http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk_______________________________________________>
http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk_______________________________________________
edk2-devel mailing list
<mailto:edk2-devel@lists.sourceforge.net> edk2-devel@lists.sourceforge.net
<https://lists.sourceforge.net/lists/listinfo/edk2-devel>
https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
