Re: [edk2] Does a double Page free report "ConvertPages: Incompatible memory types", maybe we could do better.

2017-08-10 Thread Andrew Fish 
Also I forgot to mention the double page free was in the DXE Core LoadImage() 
on an error path. 


CoreLoadPeImage()
...
  return EFI_SUCCESS;

Done:

  //
  // Free memory.
  //
  if (DstBufAlocated) {
CoreFreePages (Image->ImageContext.ImageAddress, Image->NumberOfPages);
  }
...

CoreUnloadAndCloseImage()
...
  if ((Image->ImageBasePage != 0) && FreePage) {
CoreFreePages (Image->ImageBasePage, Image->NumberOfPages);
  }
...


I fixed by setting Image->ImageContext.ImageAddress and Image->ImageBasePage to 
0 after the free in CoreLoadPeImage().

Bug 667 

Thanks,

Andrew Fish


> On Aug 9, 2017, at 6:33 PM, Zeng, Star  wrote:
> 
> Andrew,
> 
> Another path may hit this DEBUG message is AllocatePages() by AllocateAddress 
> type.
> I think it is a good suggestion to enhance the DEBUG message. How about the 
> update like below?
> 
> -DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "ConvertPages: Incompatible memory 
> types\n"));
> +DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "ConvertPages: Incompatible memory 
> types, "));
> +if (Entry->Type == EfiConventionalMemory) {
> +  DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "the pages to free have been 
> freed\n"));
> +} else {
> +  DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "the pages to allocate have been 
> allocated\n"));
> +}
> 
> 
> Thanks,
> Star
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Andrew 
> Fish
> Sent: Thursday, August 10, 2017 9:04 AM
> To: edk2-devel 
> Subject: [edk2] Does a double Page free report "ConvertPages: Incompatible 
> memory types", maybe we could do better.
> 
> It looks to me like if you Free pages, after you free pages you hit this 
> DEBUG message. 
> 
> 
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Mem/Page.c#L790
> 
>  if (!(NewType == EfiConventionalMemory ? 1 : 0) ^ (Entry->Type == 
> EfiConventionalMemory ? 1 : 0)) {
>DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "ConvertPages: Incompatible memory 
> types\n"));
>return EFI_NOT_FOUND;
>  }
> 
> I'm not sure I've thought out all the paths, but would it make more sense if 
> you are trying to convert EfiConventionalMemory to EfiConventionalMemory that 
> you are trying to free pages that are already freed. That is not very obvious 
> from the above DEBUG print.  Could there be an if in the error path to print 
> a better DEBUG message for a free pages bug? 
> 
> Also to be pedantic the function change names to: CoreConvertPagesEx(). 
> 
> Thanks,
> 
> 
> Andrew Fish
> 
> 
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Does a double Page free report "ConvertPages: Incompatible memory types", maybe we could do better.

2017-08-10 Thread Laszlo Ersek 
On 08/10/17 03:03, Andrew Fish wrote:
> It looks to me like if you Free pages, after you free pages you hit this 
> DEBUG message. 
> 
> 
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Mem/Page.c#L790
> 
>   if (!(NewType == EfiConventionalMemory ? 1 : 0) ^ (Entry->Type == 
> EfiConventionalMemory ? 1 : 0)) {
> DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "ConvertPages: Incompatible memory 
> types\n"));
> return EFI_NOT_FOUND;
>   }
> 
> I'm not sure I've thought out all the paths, but would it make more sense if 
> you are trying to convert EfiConventionalMemory to EfiConventionalMemory that 
> you are trying to free pages that are already freed. That is not very obvious 
> from the above DEBUG print.  Could there be an if in the error path to print 
> a better DEBUG message for a free pages bug? 
> 
> Also to be pedantic the function change names to: CoreConvertPagesEx(). 

(Reacting only to this side question:)

I generally prefer:

  DEBUG ((
DebugMask,
"%a: ...",
__FUNCTION__,
...
));

This way when the function is renamed, or the DEBUG is moved to another
function, the log will update itself.

Taking it one step further: if the DEBUG is in a widely used library
instance, then I like

  DEBUG ((
DebugMask,
"%a:%a: ...",
gEfiCallerBaseName,
__FUNCTION__,
...
));

Because this identifies the calling driver module as well (using the
BASE_NAME from the module's INF file).

("%g" with  would be more robust than "%a" with
gEfiCallerBaseName, but the log should also be readable...)

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel