Hi Star, Thank you, I'll update the patch.
Sincerely, Cecil Sheng ISS Firmware Development HPE Servers -----Original Message----- From: Zeng, Star [mailto:[email protected]] Sent: Wednesday, March 16, 2016 5:04 PM To: Sheng, Cecil (HPS SW) <[email protected]>; [email protected] Cc: [email protected]; Eric Dong <[email protected]> Subject: Re: [edk2] [PATCH] MdeModulePkg: Fixed incorrect Regular expression protocol MatchString return value. Minor comments below. On 2016/3/15 10:38, Sheng, Cecil (HPS SW) wrote: > > Loop maintainers. > > > Sincerely, > > Cecil Sheng > ISS Firmware Development > HPE Servers > > > -----Original Message----- > From: Sheng, Cecil (HPS SW) > Sent: Tuesday, March 1, 2016 1:17 PM > To: [email protected] > Cc: El-Haj-Mahmoud, Samer; Sheng, Cecil (HPS SW) > Subject: [PATCH] MdeModulePkg: Fixed incorrect Regular expression protocol > MatchString return value. > > According to UEFI2.6, CapturePtr in the Captures array returned by > MatchString() should be separatedly allocated so that they can be freed by > the caller. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Cecil Sheng <[email protected]> > Reviewed-by: Samer El-Haj-Mahmoud <[email protected]> > --- > .../RegularExpressionDxe/RegularExpressionDxe.c | 32 > ++++++++++++++++++---- > 1 file changed, 27 insertions(+), 5 deletions(-) > > diff --git > a/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.c > b/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.c > index cffbcb8..c6354b6 100644 > --- > a/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe.c > +++ b/MdeModulePkg/Universal/RegularExpressionDxe/RegularExpressionDxe > +++ .c > @@ -2,7 +2,7 @@ > > EFI_REGULAR_EXPRESSION_PROTOCOL Implementation > > - (C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR> > + (C) Copyright 2015-2016 Hewlett Packard Enterprise Development > + LP<BR> > > This program and the accompanying materials are licensed and made > available > under the terms and conditions of the BSD License that accompanies this > @@ -91,6 +91,8 @@ OnigurumaMatch ( > CHAR8 ErrorMessage[ONIG_MAX_ERROR_MESSAGE_LEN]; > UINT32 Index; > OnigUChar *Start; > + EFI_STATUS Status; > + > > // > // Detemine the internal syntax type @@ -102,7 +104,7 @@ > OnigurumaMatch ( > OnigSyntax = ONIG_SYNTAX_PERL; > } else { > DEBUG ((DEBUG_ERROR, "Unsupported regex syntax - using default\n")); > - ASSERT (FALSE); > + return EFI_UNSUPPORTED; > } > > // > @@ -143,6 +145,8 @@ OnigurumaMatch ( > Region, > ONIG_OPTION_NONE > ); > + > + Status = EFI_SUCCESS; [MARK] > if (OnigResult >= 0) { > *Result = TRUE; > } else { > @@ -150,6 +154,9 @@ OnigurumaMatch ( > if (OnigResult != ONIG_MISMATCH) { > onig_error_code_to_str (ErrorMessage, OnigResult); > DEBUG ((DEBUG_ERROR, "Regex match failed: %a\n", > ErrorMessage)); > + onig_region_free (Region, 1); > + onig_free (OnigRegex); > + return EFI_DEVICE_ERROR; > } > } Why not to put Status initialization(see [MARK]) here or just at the beginning of the function as the code block above does not use the Status variable? > > @@ -158,14 +165,29 @@ OnigurumaMatch ( > // > if (*Result && Captures != NULL) { > *CapturesCount = Region->num_regs; > - *Captures = AllocatePool (*CapturesCount * sizeof(**Captures)); > + *Captures = AllocateZeroPool (*CapturesCount * sizeof(**Captures)); > if (*Captures != NULL) { > for (Index = 0; Index < *CapturesCount; ++Index) { > // > // Region beg/end values represent bytes, not characters > // > - (*Captures)[Index].CapturePtr = (CHAR16*)((UINTN)String + > Region->beg[Index]); > (*Captures)[Index].Length = (Region->end[Index] - > Region->beg[Index]) / sizeof(CHAR16); > + (*Captures)[Index].CapturePtr = AllocateCopyPool ( > + ((*Captures)[Index].Length) * > sizeof (CHAR16), > + (CHAR16*)((UINTN)String + > Region->beg[Index]) > + ); > + if ((*Captures)[Index].CapturePtr == NULL) { > + Status = EFI_OUT_OF_RESOURCES; > + break; > + } > + } > + > + if (EFI_ERROR (Status)) { > + for (Index = 0; Index < *CapturesCount; ++Index) { > + if ((*Captures)[Index].CapturePtr != NULL) { > + FreePool ((CHAR16*)(*Captures)[Index].CapturePtr); > + } > + } Should *Captures also need to be freed? Thanks, Star > } > } > } > @@ -173,7 +195,7 @@ OnigurumaMatch ( > onig_region_free (Region, 1); > onig_free (OnigRegex); > > - return EFI_SUCCESS; > + return Status; > } > > /** > -- > 2.6.3.windows.1 > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
