Ard, Thanks for the fix.
Regards, Jian > -----Original Message----- > From: Ard Biesheuvel [mailto:[email protected]] > Sent: Sunday, December 31, 2017 11:57 PM > To: Wang, Jian J <[email protected]> > Cc: [email protected]; Kinney, Michael D <[email protected]>; > Yao, Jiewen <[email protected]>; Zeng, Star <[email protected]>; Gao, > Liming <[email protected]> > Subject: Re: [edk2] [PATCH v2 1/2] MdePkg/BasePrintLib: Fix error in Precision > position calculation > > On 28 December 2017 at 02:38, Jian J Wang <[email protected]> wrote: > >> v2: > >> a. Correct incorrect description in commit log > >> b. Fix another similar issue in the same function > > > > Due to a potential hole in the stop condition of loop, the two continuous > > access to ArgumentString (index, index+1) inside the loop might cause the > > string ending character ('\0') and the byte after it to be read. > > > > Cc: Michael D Kinney <[email protected]> > > Cc: Liming Gao <[email protected]> > > Cc: Jiewen Yao <[email protected]> > > Cc: Star Zeng <[email protected]> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Jian J Wang <[email protected]> > > --- > > MdePkg/Library/BasePrintLib/PrintLibInternal.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.c > b/MdePkg/Library/BasePrintLib/PrintLibInternal.c > > index 28d946472f..fc57255068 100644 > > --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c > > +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c > > @@ -1107,7 +1107,10 @@ BasePrintLibSPrintMarker ( > > // Compute the number of characters in ArgumentString and store it in > Count > > // ArgumentString is either null-terminated, or it contains Precision > characters > > // > > - for (Count = 0; Count < Precision || ((Flags & PRECISION) == 0); > > Count++) { > > + for (Count = 0; > > + ArgumentString[Count * BytesPerArgumentCharacter] != '\0' && > > + (Count < Precision || ((Flags & PRECISION) == 0)); > > + Count++) { > > ArgumentCharacter = ((ArgumentString[Count * > BytesPerArgumentCharacter] & 0xff) | ((ArgumentString[Count * > BytesPerArgumentCharacter + 1]) << 8)) & ArgumentMask; > > if (ArgumentCharacter == 0) { > > break; > > @@ -1164,7 +1167,7 @@ BasePrintLibSPrintMarker ( > > // > > // Copy the string into the output buffer performing the required type > conversions > > // > > - while (Index < Count) { > > + while (Index < Count && (*ArgumentString) != '\0') { > > ArgumentCharacter = ((*ArgumentString & 0xff) | > (((UINT8)*(ArgumentString + 1)) << 8)) & ArgumentMask; > > > > LengthToReturn += (1 * BytesPerOutputCharacter); > > This patch breaks UEFI menu rendering: the following > > /------------------------------------------------------------------------------\ > | Device Manager > | > \------------------------------------------------------------------------------/ > > > is rendered as > > /\ > | Device Manager > | > \/.0 2.00 GHz > > (the spurious digits are SMBIOS data from the home screen) > > The problem appears to be that the CHAR16 value of BOXDRAW_HORIZONTAL > equals 0x2500, which means that testing ArgumentString[] != '\0' > (which tests the low byte only) will yield FALSE and terminate the > loop prematurely. > > The following patch fixes things for me. > > diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.c > b/MdePkg/Library/BasePrintLib/PrintLibInternal.c > index fc5725506848..3f4be932f369 100644 > --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c > +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c > @@ -1108,7 +1108,9 @@ BasePrintLibSPrintMarker ( > // ArgumentString is either null-terminated, or it contains > Precision characters > // > for (Count = 0; > - ArgumentString[Count * BytesPerArgumentCharacter] != '\0' && > + (ArgumentString[Count * BytesPerArgumentCharacter] != '\0' || > + (BytesPerArgumentCharacter > 1 && > + ArgumentString[Count * BytesPerArgumentCharacter + 1] > != '\0')) && > (Count < Precision || ((Flags & PRECISION) == 0)); > Count++) { > ArgumentCharacter = ((ArgumentString[Count * > BytesPerArgumentCharacter] & 0xff) | ((ArgumentString[Count * > BytesPerArgumentCharacter + 1]) << 8)) & ArgumentMask; > @@ -1167,7 +1169,9 @@ BasePrintLibSPrintMarker ( > // > // Copy the string into the output buffer performing the required > type conversions > // > - while (Index < Count && (*ArgumentString) != '\0') { > + while (Index < Count && > + (ArgumentString[0] != '\0' || > + (BytesPerArgumentCharacter > 1 && ArgumentString[1] != '\0'))) { > ArgumentCharacter = ((*ArgumentString & 0xff) | > (((UINT8)*(ArgumentString + 1)) << 8)) & ArgumentMask; > > LengthToReturn += (1 * BytesPerOutputCharacter); _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
