> On Dec 3, 2014, at 11:37 PM, Yao, Jiewen <jiewen....@intel.com> wrote:
>
> 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.
>
Good book!
> In this case, I also think we need add error handling code to handle real
> error – especially shell application.
>
I agree. We should also take the same approach with EFI Runtime and Boot
Services, EFI Driver Model driver, and any code processing 3rd party input.
I’d point out it is not just for correctness, but it also enhances security, as
attacks against secure boot will likely try to exploit a bug in the code.
> 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 <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 <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_______________________________________________
>
> <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
> <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_______________________________________________
>
> <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
> <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
------------------------------------------------------------------------------
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
