Re: [edk2] FreePool crashes the system when called with a NULL pointer

Subject: Re: [edk2] FreePool crashes the system when called with a NULL pointer

From: Andrew Fish <afish@apple.com>

To: edk2-devel@lists.sourceforge.net

Date: 2013-09-18 16:57:44


On Sep 18, 2013, at 5:59 AM, "Cohen, Eugene" <eugene@hp.com> wrote:

  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.
 

For OVFM I think you would want to add a stack walk to https://svn.code.sf.net/p/edk2/code/trunk/edk2/UefiCpuPkg/CpuDxe/CpuDxe.c

Actually it would be good to add the DefaultExceptionHandlerLib, like the ArmPkg. That way the #ifdef's for the register dump could removed. At a minimum code could be added to print out which image faulted. That code is here https://svn.code.sf.net/p/edk2/code/trunk/edk2/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerUefi.c.

The stack trace could be added in gcc specific code, if it works, and you can use the GetImageName() function to print out name, and figure out the offsite in the module of the PC. 

https://svn.code.sf.net/p/edk2/code/trunk/edk2/ArmPkg/Include/Library/DefaultExceptionHandlerLib.h
https://svn.code.sf.net/p/edk2/code/trunk/edk2/ArmPkg/Library/DefaultExceptionHandlerLib/

  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, its 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.
 

I don't have an issue with making FreePool() treat NULL the same way as free(). But a change is up to the package maintainer. 

If the purpose of the ASSERT is trying to catch people using NULL pointers then wed 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.
 

This is doable, buy there needs to be a way to turn it off if you launch a CSM. 

Eugene
 
From: Ryan Harkin [mailto:ryan.harkin@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 <afish@apple.com> wrote:
 
On Sep 16, 2013, at 11:36 PM, Ryan Harkin <ryan.harkin@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. 
 
/**
  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