| Message ID | 1477330907-13733-8-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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 <ard.biesheuvel@linaro.org> > --- > 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 <lersek@redhat.com> I need some brain bleach now. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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); } else { - AsciiStrToUnicodeStr ("\\", UnicodeFileName); + AsciiStrToUnicodeStrS ("\\", UnicodeFileName, MAX_PATHNAME); } 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; @@ -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); StrLen = AsciiStrLen (gCwd); if ((*PathName != '/') && (*PathName != '\\') && (gCwd[StrLen-1] != '/') && (gCwd[StrLen-1] != '\\')) { - AsciiStrCat (CwdPlusPathName, "\\"); + AsciiStrCatS (CwdPlusPathName, AsciiLength, "\\"); } } - 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; @@ -745,7 +747,7 @@ EfiOpen ( } File->DeviceName = AllocatePool (StrLen); - AsciiStrCpy (File->DeviceName, PathName); + AsciiStrCpyS (File->DeviceName, StrLen, PathName); 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; } @@ -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); }
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 <ard.biesheuvel@linaro.org> --- EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c | 42 +++++++++++--------- 1 file changed, 23 insertions(+), 19 deletions(-) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel