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
> 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> 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>
>
> 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
> 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
