| Message ID | 1456348432-18818-5-git-send-email-lersek@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
On 02/25/16 09:56, Shia, Cinnamon wrote: > Reviewed-By: Cinnamon Shia <cinnamon.shia@hpe.com> Thanks, but actually I noticed an error in this patch: > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, February 25, 2016 5:14 AM > To: edk2-devel-01 > Cc: Sheng, Cecil (HPS SW); Shia, Cinnamon; Eric Dong; Qiu Shumin; El-Haj-Mahmoud, Samer; Yao Jiewen > Subject: [PATCH 4/4] MdeModulePkg: RegularExpressionDxe: support free(NULL) > > The ISO C standard says about free(), > > If ptr is a null pointer, no action occurs. > > This is not true of the FreePool() interface of the MemoryAllocationLib > class: > > Buffer must have been allocated on a previous call to the pool > allocation services of the Memory Allocation Library. [...] If Buffer > was not allocated with a pool allocation function in the Memory > Allocation Library, then ASSERT(). > > Therefore we must not forward the argument of free() to FreePool() without > checking. > > Cc: Cecil Sheng <cecil.sheng@hpe.com> > Cc: Cinnamon Shia <cinnamon.shia@hpe.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Qiu Shumin <shumin.qiu@intel.com> > Cc: Samer El-Haj-Mahmoud <elhaj@hpe.com> > Cc: Yao Jiewen <Jiewen.Yao@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > > Notes: > Build-tested only. > > MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.h | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.h b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.h > index cb791f8c84c6..b970aaa48c0e 100644 > --- a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.h > +++ b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.h > @@ -30,7 +30,17 @@ typedef UINTN size_t; > > #define malloc(n) AllocatePool(n) > #define calloc(n,s) AllocateZeroPool((n)*(s)) > -#define free(p) FreePool(p) > + > +#define free(p) \ > + do { \ > + VOID *EvalOnce; \ > + \ > + EvalOnce = (VOID *)(p); \ the explicit cast to (VOID *) is an error here. I think I was simply too tired when I wrote this patch last night. The explicit (VOID *) cast will *incorrectly* suppress errors that a direct call to FreePool() or free() would report. Consider the following example: STATIC CONST CHAR8 MyString[] = "Hello World"; CONST VOID *MyPointer; MyPointer = MyString; // valid FreePool (MyPointer); // invalid, but caught by the compiler! The pre-patch macro definition will catch this -- the compiler will report a warning (usually treated as an error) that the pointer conversion (implicit in passing the argument to FreePool(), which takes a (VOID *)) throws away the CONST qualifier. Whereas the modified macro definition will silently suppress that. So, I will go ahead and commit patches 1-3 in this series, with Qin Long's R-b; but I will submit a new version of this patch separately. Thanks, and sorry for the churn. Laszlo > + if (EvalOnce != NULL) { \ > + FreePool (EvalOnce); \ > + } \ > + } while (FALSE) > + > #define realloc(OldPtr,NewSize,OldSize) ReallocatePool(OldSize,NewSize,OldPtr) > #define xmemmove(Dest,Src,Length) CopyMem(Dest,Src,Length) > #define xmemcpy(Dest,Src,Length) CopyMem(Dest,Src,Length) > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.h b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.h index cb791f8c84c6..b970aaa48c0e 100644 --- a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.h +++ b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.h @@ -30,7 +30,17 @@ typedef UINTN size_t; #define malloc(n) AllocatePool(n) #define calloc(n,s) AllocateZeroPool((n)*(s)) -#define free(p) FreePool(p) + +#define free(p) \ + do { \ + VOID *EvalOnce; \ + \ + EvalOnce = (VOID *)(p); \ + if (EvalOnce != NULL) { \ + FreePool (EvalOnce); \ + } \ + } while (FALSE) + #define realloc(OldPtr,NewSize,OldSize) ReallocatePool(OldSize,NewSize,OldPtr) #define xmemmove(Dest,Src,Length) CopyMem(Dest,Src,Length) #define xmemcpy(Dest,Src,Length) CopyMem(Dest,Src,Length)
The ISO C standard says about free(), If ptr is a null pointer, no action occurs. This is not true of the FreePool() interface of the MemoryAllocationLib class: Buffer must have been allocated on a previous call to the pool allocation services of the Memory Allocation Library. [...] If Buffer was not allocated with a pool allocation function in the Memory Allocation Library, then ASSERT(). Therefore we must not forward the argument of free() to FreePool() without checking. Cc: Cecil Sheng <cecil.sheng@hpe.com> Cc: Cinnamon Shia <cinnamon.shia@hpe.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Qiu Shumin <shumin.qiu@intel.com> Cc: Samer El-Haj-Mahmoud <elhaj@hpe.com> Cc: Yao Jiewen <Jiewen.Yao@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: Build-tested only. MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/OnigurumaUefiPort.h | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) -- 1.8.3.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel