From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C222C1A1E2A for ; Tue, 25 Oct 2016 07:20:19 -0700 (PDT) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3E0C767EB7; Tue, 25 Oct 2016 14:20:19 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-71.phx2.redhat.com [10.3.116.71]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9PEKHFh021580; Tue, 25 Oct 2016 10:20:18 -0400 To: Ard Biesheuvel , edk2-devel@ml01.01.org, leif.lindholm@linaro.org References: <1477330907-13733-1-git-send-email-ard.biesheuvel@linaro.org> <1477330907-13733-8-git-send-email-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: Date: Tue, 25 Oct 2016 16:20:17 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1477330907-13733-8-git-send-email-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Tue, 25 Oct 2016 14:20:19 +0000 (UTC) Subject: Re: [PATCH 7/9] EmbeddedPkg/EfiFileLib: eliminate deprecated string function calls X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Oct 2016 14:20:20 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 10/24/16 19:41, Ard Biesheuvel wrote: > Get rid of calls to unsafe string functions. These are deprecated and may > be removed in the future. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c | 42 +++++++++++--------- > 1 file changed, 23 insertions(+), 19 deletions(-) > > diff --git a/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c b/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c > index 4d58c830861c..d3b65aa5a3e0 100644 > --- a/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c > +++ b/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c > @@ -384,9 +384,9 @@ EblFileDevicePath ( > > > if ( *FileName != 0 ) { > - AsciiStrToUnicodeStr (FileName, UnicodeFileName); > + AsciiStrToUnicodeStrS (FileName, UnicodeFileName, MAX_PATHNAME); Seems okay. > } else { > - AsciiStrToUnicodeStr ("\\", UnicodeFileName); > + AsciiStrToUnicodeStrS ("\\", UnicodeFileName, MAX_PATHNAME); > } Ditto. > > Size = StrSize (UnicodeFileName); > @@ -589,7 +589,7 @@ EblFvFileDevicePath ( > &AuthenticationStatus > ); > if (!EFI_ERROR (Status)) { > - UnicodeStrToAsciiStr (Section, AsciiSection); > + UnicodeStrToAsciiStrS (Section, AsciiSection, MAX_PATHNAME); > if (AsciiStriCmp (FileName, AsciiSection) == 0) { > FreePool (Section); > break; Seems fine. > @@ -674,6 +674,7 @@ EfiOpen ( > CHAR8 *CwdPlusPathName; > UINTN Index; > EFI_SECTION_TYPE ModifiedSectionType; > + UINTN AsciiLength; > > EblUpdateDeviceLists (); > > @@ -706,7 +707,8 @@ EfiOpen ( > } > > // We could add a current working directory concept > - CwdPlusPathName = AllocatePool (AsciiStrSize (gCwd) + AsciiStrSize (PathName)); > + AsciiLength = AsciiStrSize (gCwd) + AsciiStrSize (PathName); > + CwdPlusPathName = AllocatePool (AsciiLength); > if (CwdPlusPathName == NULL) { > return NULL; > } > @@ -723,14 +725,14 @@ EfiOpen ( > } > } > } else { > - AsciiStrCpy (CwdPlusPathName, gCwd); > + AsciiStrCpyS (CwdPlusPathName, AsciiLength, gCwd); okay > StrLen = AsciiStrLen (gCwd); > if ((*PathName != '/') && (*PathName != '\\') && (gCwd[StrLen-1] != '/') && (gCwd[StrLen-1] != '\\')) { > - AsciiStrCat (CwdPlusPathName, "\\"); > + AsciiStrCatS (CwdPlusPathName, AsciiLength, "\\"); > } > } okay > > - AsciiStrCat (CwdPlusPathName, PathName); > + AsciiStrCatS (CwdPlusPathName, AsciiLength, PathName); > if (AsciiStrStr (CwdPlusPathName, ":") == NULL) { > // Extra error check to make sure we don't recurse and blow stack > return NULL; Also fine. (I strongly dislike the trickery in the original allocation, that is, summing two full sizes (each including the terminating NUL separately), and then letting one of those NULs account for the pathname separator '\\' that gets inserted in the middle. It happens to be correct, but seriously...) > @@ -745,7 +747,7 @@ EfiOpen ( > } > > File->DeviceName = AllocatePool (StrLen); StrLen was initially assigned like this: StrLen = AsciiStrSize (PathName); (Q: what would you call a variable storing the retval of a function named XxxxSize()? A: XxxxLen, obviously! Never mind that there is a function named XxxxLen() as well; we'll store the retval of *that* in XxxxSize. For clarity.) ... Just to be clear, this is criticism for the original code. > - AsciiStrCpy (File->DeviceName, PathName); > + AsciiStrCpyS (File->DeviceName, StrLen, PathName); okay. > File->DeviceName[FileStart - 1] = '\0'; > File->FileName = &File->DeviceName[FileStart]; > if (File->FileName[0] == '\0') { > @@ -1611,7 +1613,7 @@ ExpandPath ( > { > CHAR8 *NewPath; > CHAR8 *Work, *Start, *End; > - UINTN StrLen; > + UINTN StrLen, AllocLen; > INTN i; > > if (Cwd == NULL || Path == NULL) { > @@ -1625,11 +1627,12 @@ ExpandPath ( > } > > StrLen = AsciiStrSize (Path); > - NewPath = AllocatePool (AsciiStrSize (Cwd) + StrLen + 1); > + AllocLen = AsciiStrSize (Cwd) + StrLen + 1; > + NewPath = AllocatePool (AllocLen); > if (NewPath == NULL) { > return NULL; > } > - AsciiStrCpy (NewPath, Cwd); > + AsciiStrCpyS (NewPath, AllocLen, Cwd); > > End = Path + StrLen; > for (Start = Path ;;) { > @@ -1640,7 +1643,7 @@ ExpandPath ( > } > > // append path prior to .. > - AsciiStrnCat (NewPath, Start, Work - Start); > + AsciiStrnCatS (NewPath, AllocLen, Start, Work - Start); > StrLen = AsciiStrLen (NewPath); > for (i = StrLen; i >= 0; i--) { > if (NewPath[i] == ':') { > @@ -1663,7 +1666,7 @@ ExpandPath ( > } > > // Handle the path that remains after the .. > - AsciiStrnCat (NewPath, Start, End - Start); > + AsciiStrnCatS (NewPath, AllocLen, Start, End - Start); > > return NewPath; > } looks good > @@ -1686,7 +1689,7 @@ EfiSetCwd ( > ) > { > EFI_OPEN_FILE *File; > - UINTN Len; > + UINTN Len, AllocLen; > CHAR8 *Path; > > if (Cwd == NULL) { > @@ -1729,17 +1732,18 @@ EfiSetCwd ( > > // Use the info returned from EfiOpen as it can add in CWD if needed. So Cwd could be > // relative to the current gCwd or not. > - gCwd = AllocatePool (AsciiStrSize (File->DeviceName) + AsciiStrSize (File->FileName) + 10); > + AllocLen = AsciiStrSize (File->DeviceName) + AsciiStrSize (File->FileName) + 10; > + gCwd = AllocatePool (AllocLen); > if (gCwd == NULL) { > return EFI_INVALID_PARAMETER; > } > > - AsciiStrCpy (gCwd, File->DeviceName); > + AsciiStrCpyS (gCwd, AllocLen, File->DeviceName); > if (File->FileName == NULL) { > - AsciiStrCat (gCwd, ":\\"); > + AsciiStrCatS (gCwd, AllocLen, ":\\"); > } else { > - AsciiStrCat (gCwd, ":"); > - AsciiStrCat (gCwd, File->FileName); > + AsciiStrCatS (gCwd, AllocLen, ":"); > + AsciiStrCatS (gCwd, AllocLen, File->FileName); > } > > > Looks correct. (The constant 10 in the addition, for the allocation, represents how carefully the original code was written.) Reviewed-by: Laszlo Ersek I need some brain bleach now.