Hi Scott and Sergey
1) I think <Code Complete> by Steve McConnell, gives a good guide line for
ASSERT. Use error handling code for conditions you expect to occur; use
assertions for conditions that should never occur.
In this case, I also think we need add error handling code to handle real error
- especially shell application.
And, I think to use error handling code in all firmware code might be
unnecessary. Because 1) some conditions should never happen, 2) it increases
code size.
Sometimes, the failure is not acceptable and causes system stuck even continue
to boot. For example, allocate memory in early PEI phase. I think we had better
fail at the place, not later, for better debug purpose.
Each module owner should have his/her own judgment on if the error should
*never* occur, or *expect to*occur. Then he/she can choose ASSERT or error
handling.
Again, in this specific case, I vote for later. I agree with you.
2) If used memory is not freed, I believe it is bug and it should be fixed. -
That is different story.
Thank you
Yao Jiewen
From: Sergey Isakov [mailto:isakov...@bk.ru]
Sent: Thursday, December 04, 2014 3:03 PM
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] [Patch] ShellPkg: Add ASSERT to check pointer to avoid
being dereferenced
Fully agree with Scott.
I replaced many ASSERT occurrence in ShellPkg by conditional Return statement
and got the application working as never before.
I hope developers will use ASSERT() only as comments and will not use then as
check NULL pointers especially for function arguments that can't guarantee how
the procedure will be called.
Thanks,
Sergey
On 04.12.2014, at 5:51, Scott Duplichan wrote:
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 <shumin....@intel.com<mailto: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_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net<mailto:edk2-devel@lists.sourceforge.net>
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<mailto:edk2-devel@lists.sourceforge.net>
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
