On 17 September 2013 16:52, Andrew Fish <af...@apple.com> wrote:
>
> On Sep 16, 2013, at 11:36 PM, Ryan Harkin <ryan.har...@linaro.org> wrote:
>
> Hi,
>
> Tracking down a crash I am seeing shows that I am effectively calling
> FreePool() with a NULL pointer. The system then ASSERTS and hangs.
>
> I've noticed various seemingly "random" asserts like this before and
> suspect it may be related. Of course, the assert output never helps track
> down the culprit, but that's a different issue altogether.
>
> Coming from an environment where I'm able to call malloc() and then free()
> with a NULL pointer, my experience told me that I should be able to call
> FreePool() with a NULL pointer without worrying about checking the pointer
> for NULL first.
>
> However, CoreFreePool() that eventually gets called is explicitly
> returning EFI_INVALID_PARAMETER if it is called with NULL. And FreePool()
> explicitly ASSERTs if the return value from CoreFreePool() is not
> EFI_SUCCESS.
>
> It doesn't strike me as a sensible thing to do.
>
> I think FreePool(NULL) should just return without reporting an error. I
> see a *lot* of calls to FreePool where the pointer isn't first checked for
> NULL. The code path is often simpler, where the code will often return if
> the alloc fails, but in my example, I am retrieving an optional variable
> which may or may not exist and therefore may or may not allocate memory.
>
>
> gBS->FreePool() returns an error, per the spec, as it should.
>
I know, that's why I'm suggesting we change it. I understand that if there
is a real error freeing the pool, that the caller might want to know if
they think they can do something about it. But I think that the NULL case
is "special".
>
> Although systems seems to have got along just fine up to now - the code
> has always been like this - I'd like to change it. Hanging seems
> unnecessarily harsh.
>
> What do others think?
>
> Is there a real advantage to hanging on NULL?
>
>
> Technically the system is not hung, it is likely sitting at a breakpoint
> or a dead loop, so you can step past in a debugger. You also get a nice
> stack trace with a debugger.
>
> There are lots of knobs to control this behavior. You can turn off
> ASSERTs, and you can also make an ASSERT, breakpoint, dead-loop, or
> continue automatically.
>
>
> https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdePkg/Include/Library/DebugLib.h
>
> /**
> Prints an assert message containing a filename, line number, and
> description.
> This may be followed by a breakpoint or a dead loop.
>
> Print a message of the form "ASSERT <FileName>(<LineNumber>):
> <Description>\n"
> to the debug output device. If DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED
> bit of
> PcdDebugProperyMask is set then CpuBreakpoint() is called. Otherwise, if
> DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED bit of PcdDebugProperyMask is set
> then
> CpuDeadLoop() is called. If neither of these bits are set, then this
> function
> returns immediately after the message is printed to the debug output device.
> DebugAssert() must actively prevent recursion. If DebugAssert() is called
> while
> processing another DebugAssert(), then DebugAssert() must return
> immediately.
>
> If FileName is NULL, then a <FileName> string of "(NULL) Filename" is
> printed.
> If Description is NULL, then a <Description> string of "(NULL) Description"
> is printed.
>
> @param FileName The pointer to the name of the source file that
> generated the assert condition.
> @param LineNumber The line number in the source file that generated the
> assert condition
> @param Description The pointer to the description of the assert condition.
>
> **/
> VOID
> EFIAPI
> DebugAssert (
> IN CONST CHAR8 *FileName,
> IN UINTN LineNumber,
> IN CONST CHAR8 *Description
> );
>
>
> /**
> Internal worker macro that calls DebugAssert().
>
> This macro calls DebugAssert(), passing in the filename, line number, and an
> expression that evaluated to FALSE.
>
> @param Expression Boolean expression that evaluated to FALSE
>
> **/
> #define _ASSERT(Expression) DebugAssert (__FILE__, __LINE__, #Expression)
>
>
> #if !defined(MDEPKG_NDEBUG)
> #define ASSERT(Expression) \
> do { \
> if (DebugAssertEnabled ()) { \
> if (!(Expression)) { \
> _ASSERT (Expression); \
> } \
> } \
> } while (FALSE)
> #else
> #define ASSERT(Expression)
> #endif
>
>
>
> Regards,
> Ryan.
>
> ------------------------------------------------------------------------------
> LIMITED TIME SALE - Full Year of Microsoft Training For Just $49.99!
> 1,500+ hours of tutorials including VisualStudio 2012, Windows 8,
> SharePoint
> 2013, SQL 2012, MVC 4, more. BEST VALUE: New Multi-Library Power Pack
> includes
> Mobile, Cloud, Java, and UX Design. Lowest price ever! Ends 9/20/13.
>
> http://pubads.g.doubleclick.net/gampad/clk?id=58041151&iu=/4140/ostg.clktrk_______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
>
>
>
> ------------------------------------------------------------------------------
> LIMITED TIME SALE - Full Year of Microsoft Training For Just $49.99!
> 1,500+ hours of tutorials including VisualStudio 2012, Windows 8,
> SharePoint
> 2013, SQL 2012, MVC 4, more. BEST VALUE: New Multi-Library Power Pack
> includes
> Mobile, Cloud, Java, and UX Design. Lowest price ever! Ends 9/20/13.
> http://pubads.g.doubleclick.net/gampad/clk?id=58041151&iu=/4140/ostg.clktrk
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
>
------------------------------------------------------------------------------
LIMITED TIME SALE - Full Year of Microsoft Training For Just $49.99!
1,500+ hours of tutorials including VisualStudio 2012, Windows 8, SharePoint
2013, SQL 2012, MVC 4, more. BEST VALUE: New Multi-Library Power Pack includes
Mobile, Cloud, Java, and UX Design. Lowest price ever! Ends 9/20/13.
http://pubads.g.doubleclick.net/gampad/clk?id=58041151&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
