Hi Laszlo, Thank you for your comment. I've sent an updated patch for review. https://lists.01.org/pipermail/edk2-devel/2019-January/035806.html
Regards, Neo > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Friday, January 25, 2019 1:07 AM > To: Hsueh, Hong-chihX <hong-chihx.hs...@intel.com>; edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Bi, Dandan > <dandan...@intel.com>; Gao, Liming <liming....@intel.com> > Subject: Re: [edk2] [PATCH] MdePkg/BasePeCoffLib: skip runtime relocation if > relocation info is invalid. > > On 01/25/19 00:18, Neo Hsueh wrote: > > 1.Skip runtime relocation for PE images that provide invalid relocation > > infomation (ex: RelocDir->Size = 0) to fix a hang observed while booting > > Windows. > > 2.Add a magic number check for PE32+ image. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Neo Hsueh <hong-chihx.hs...@intel.com> > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > Cc: Liming Gao <liming....@intel.com> > > Cc: Dandan Bi <dandan...@intel.com> > > --- > > MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 19 ++++++++++++++++--- > > 1 file changed, 16 insertions(+), 3 deletions(-) > > I can't comment on the technical details of the patch, but I have some > comments on the organization of the patch. > > First, the two changes that it implements should be separate patches. > > > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > index 1bd079ad6a..6477ef0759 100644 > > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c > > @@ -1725,11 +1725,18 @@ PeCoffLoaderRelocateImageForRuntime ( > > NumberOfRvaAndSizes = Hdr.Pe32- > >OptionalHeader.NumberOfRvaAndSizes; > > DataDirectory = (EFI_IMAGE_DATA_DIRECTORY *)&(Hdr.Pe32- > >OptionalHeader.DataDirectory[0]); > > } else { > > + if (Hdr.Pe32Plus->OptionalHeader.Magic == > EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) { > > + // > > + // Use PE32+ offset > > + // > > + NumberOfRvaAndSizes = Hdr.Pe32Plus- > >OptionalHeader.NumberOfRvaAndSizes; > > + DataDirectory = (EFI_IMAGE_DATA_DIRECTORY *)&(Hdr.Pe32Plus- > >OptionalHeader.DataDirectory[0]); > > + } else { > > // > > - // Use PE32+ offset > > + // Not a valid PE image so Exit > > // > > - NumberOfRvaAndSizes = Hdr.Pe32Plus- > >OptionalHeader.NumberOfRvaAndSizes; > > - DataDirectory = (EFI_IMAGE_DATA_DIRECTORY *)&(Hdr.Pe32Plus- > >OptionalHeader.DataDirectory[0]); > > + return; > > + } > > } > > > > // > > Second, my understanding is that in edk2, we don't do > > if (Condition1) { > } else { > if (Condition2) { > } else { > } > } > > Instead, we prefer > > if (Condition1) { > } else if (Condition2) { > } else { > } > > As far as I know, this is the only construct where we don't require braces > after > an "else". See: > > https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards- > specification/content/5_source_files/57_c_programming.html#57342-when- > an-else-is-used-it-may-start-on-the-same-line-as-the-close-brace-of-the-if-or- > be-on-the-following-line-and-aligned-with-the-closing-brace > > > Third, the return statement and the comment are not properly indented in the > last branch. > > Thanks > Laszlo > > > @@ -1746,6 +1753,12 @@ PeCoffLoaderRelocateImageForRuntime ( > > > > RelocDir->VirtualAddress + > RelocDir->Size - 1, > > > > 0 > > > > ); > > + if (RelocBase == NULL || RelocBaseEnd == NULL || RelocBaseEnd < > RelocBase) { > > + // > > + // relocation block is not valid, just return > > + // > > + return; > > + } > > } else { > > // > > // Cannot find relocations, cannot continue to relocate the image, > > ASSERT > for this invalid image. > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
