Ø 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.
A call stack dump, either with a debugger or some additional crash handling
code should do the trick.
Ø 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.
Ryan, I agree. Given that developers will be coming from a libc background
where they expect freeing a NULL pointer to be allowed, it's probably easier
overall to modify the various memory allocation libraries to either remove the
ASSERT or add a special-case to only call gBS->FreePool if the pointer is not
NULL.
If the purpose of the ASSERT is trying to catch people using NULL pointers then
we'd be better served by using the MMU to unmap the page at address zero and
generate exceptions at the moment the first access attempt happens.
Eugene
From: Ryan Harkin [mailto:ryan.har...@linaro.org]
Sent: Wednesday, September 18, 2013 12:37 AM
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] FreePool crashes the system when called with a NULL pointer
On 17 September 2013 16:52, Andrew Fish
<af...@apple.com<mailto:af...@apple.com>> wrote:
On Sep 16, 2013, at 11:36 PM, Ryan Harkin
<ryan.har...@linaro.org<mailto: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<mailto: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<mailto: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
