Message ID | 1477419424-22235-3-git-send-email-ard.biesheuvel@linaro.org |
---|---|
State | Superseded |
Headers | show |
On 10/25/16 20:17, Ard Biesheuvel wrote: > Get rid of functions that are no longer available when defining > DISABLE_NEW_DEPRECATED_INTERFACES > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c | 8 +++----- > ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c | 3 ++- > ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c | 12 +++++------- > 3 files changed, 10 insertions(+), 13 deletions(-) > > diff --git a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c > index 450a707f183c..2736d3e0d0bf 100644 > --- a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c > +++ b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c > @@ -304,7 +304,6 @@ SetFileName ( > IN CONST CHAR16 *FileName > ) > { > - CHAR16 TruncFileName[MAX_NAME_LENGTH]; > CHAR8 AsciiFileName[MAX_NAME_LENGTH]; > BOOTMON_FS_FILE *SameFile; > > @@ -314,9 +313,7 @@ SetFileName ( > FileName++; > } > > - StrnCpy (TruncFileName, FileName, MAX_NAME_LENGTH - 1); > - TruncFileName[MAX_NAME_LENGTH - 1] = 0; > - UnicodeStrToAsciiStr (TruncFileName, AsciiFileName); > + UnicodeStrToAsciiStrS (FileName, AsciiFileName, MAX_NAME_LENGTH); > > if (BootMonGetFileFromAsciiFileName ( > File->Instance, Good. > @@ -327,7 +324,8 @@ SetFileName ( > return EFI_ACCESS_DENIED; > } else { > // OK, change the filename. > - AsciiStrToUnicodeStr (AsciiFileName, File->Info->FileName); > + AsciiStrToUnicodeStrS (AsciiFileName, File->Info->FileName, > + (File->Info->Size - sizeof *File->Info) / sizeof (CHAR16)); > return EFI_SUCCESS; > } > } I think this is incorrect. The division is fine, but the dividend is off by one CHAR16: the last member of EFI_FILE_INFO (that is, of *File->Info) is /// /// The Null-terminated name of the file. /// CHAR16 FileName[1]; If you subtract the entire EFI_FILE_INFO structure, then you remove the first character from the file name as well. Please add (sizeof (CHAR16)) to the dividend; or else, use File->Info->Size - OFFSET_OF (EFI_FILE_INFO, FileName) as the dividend. Hey, wait a minute: look at the macro SIZE_OF_EFI_FILE_INFO in "MdePkg/Include/Guid/FileInfo.h": /// /// The FileName field of the EFI_FILE_INFO data structure is variable /// length. Whenever code needs to know the size of the EFI_FILE_INFO /// data structure, it needs to be the size of the data structure /// without the FileName field. The following macro computes this size /// correctly no matter how big the FileName array is declared. This is /// required to make the EFI_FILE_INFO data structure ANSI compilant. /// #define SIZE_OF_EFI_FILE_INFO OFFSET_OF (EFI_FILE_INFO, FileName) So, for take-no-hostages pedantry, you should make the dividend File->Info->Size - SIZE_OF_EFI_FILE_INFO > diff --git a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c > index 3d71760fef99..a1150856f6ba 100644 > --- a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c > +++ b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c > @@ -98,7 +98,8 @@ BootMonGetFileFromAsciiFileName ( > { > FileEntry = BOOTMON_FS_FILE_FROM_LINK_THIS (Entry); > if (FileEntry->Info != NULL) { > - UnicodeStrToAsciiStr (FileEntry->Info->FileName, OpenFileAsciiFileName); > + UnicodeStrToAsciiStrS (FileEntry->Info->FileName, OpenFileAsciiFileName, > + MAX_NAME_LENGTH); > AsciiFileNameToCompare = OpenFileAsciiFileName; > } else { > AsciiFileNameToCompare = FileEntry->HwDescription.Footer.Filename; okay > diff --git a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c > index af2fe514f044..4927d987eccf 100644 > --- a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c > +++ b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c > @@ -101,7 +101,8 @@ WriteFileDescription ( > Description->Attributes = 1; > Description->BlockStart = FileStart / BlockSize; > Description->BlockEnd = Description->BlockStart + (FileSize / BlockSize); > - AsciiStrCpy (Description->Footer.Filename, FileName); > + AsciiStrCpyS (Description->Footer.Filename, > + sizeof Description->Footer.Filename, FileName); > > #ifdef MDE_CPU_ARM > Description->Footer.Offset = HW_IMAGE_FOOTER_OFFSET; okay > @@ -294,7 +295,7 @@ BootMonFsFlushFile ( > DiskIo = Instance->DiskIo; > BlockSize = Media->BlockSize; > > - UnicodeStrToAsciiStr (Info->FileName, AsciiFileName); > + UnicodeStrToAsciiStrS (Info->FileName, AsciiFileName, MAX_NAME_LENGTH); > > // If the file doesn't exist then find a space for it > if (File->HwDescription.RegionCount == 0) { okay > @@ -626,10 +627,7 @@ BootMonFsOpenFile ( > Status = EFI_OUT_OF_RESOURCES; > goto Error; > } > - UnicodeStrToAsciiStr (Path, AsciiFileName); > - if (AsciiStrSize (AsciiFileName) > MAX_NAME_LENGTH) { > - AsciiFileName[MAX_NAME_LENGTH - 1] = '\0'; > - } > + UnicodeStrToAsciiStrS (Path, AsciiFileName, MAX_NAME_LENGTH); > > if ((AsciiFileName[0] == '\0') || > (AsciiFileName[0] == '.' ) ) { This change is incorrect. Consider the case when StrLen (Path) == 1, for example -- you won't have MAX_NAME_LENGTH (32) characters in the dynamically allocated AsciiFileName array. I realize that no buffer overflow could happen in reality -- that's because the original code is already safe here, and the receiving ASCII buffer has been sized for the UCS2 input -- but DestMax=MAX_NAME_LENGTH is untrue, generally speaking. I suggest to introduce AsciiFileNameSize = StrLen (Path) + 1; if (AsciiFileNameSize > MAX_NAME_LENGTH) { AsciiFileNameSize = MAX_NAME_LENGTH; } and then use AsciiFileNameSize in both the allocation and the UnicodeStrToAsciiStrS() call. > @@ -688,7 +686,7 @@ BootMonFsOpenFile ( > > Info->FileSize = BootMonFsGetImageLength (File); > Info->PhysicalSize = BootMonFsGetPhysicalSize (File); > - AsciiStrToUnicodeStr (AsciiFileName, Info->FileName); > + AsciiStrToUnicodeStrS (AsciiFileName, Info->FileName, MAX_NAME_LENGTH); > > File->Info = Info; > Info = NULL; > Info is allocated with: Info = AllocateZeroPool ( SIZE_OF_EFI_FILE_INFO + (sizeof (CHAR16) * MAX_NAME_LENGTH)); -- see SIZE_OF_EFI_FILE_INFO above --, so this hunk is correct. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c index 450a707f183c..2736d3e0d0bf 100644 --- a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c +++ b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c @@ -304,7 +304,6 @@ SetFileName ( IN CONST CHAR16 *FileName ) { - CHAR16 TruncFileName[MAX_NAME_LENGTH]; CHAR8 AsciiFileName[MAX_NAME_LENGTH]; BOOTMON_FS_FILE *SameFile; @@ -314,9 +313,7 @@ SetFileName ( FileName++; } - StrnCpy (TruncFileName, FileName, MAX_NAME_LENGTH - 1); - TruncFileName[MAX_NAME_LENGTH - 1] = 0; - UnicodeStrToAsciiStr (TruncFileName, AsciiFileName); + UnicodeStrToAsciiStrS (FileName, AsciiFileName, MAX_NAME_LENGTH); if (BootMonGetFileFromAsciiFileName ( File->Instance, @@ -327,7 +324,8 @@ SetFileName ( return EFI_ACCESS_DENIED; } else { // OK, change the filename. - AsciiStrToUnicodeStr (AsciiFileName, File->Info->FileName); + AsciiStrToUnicodeStrS (AsciiFileName, File->Info->FileName, + (File->Info->Size - sizeof *File->Info) / sizeof (CHAR16)); return EFI_SUCCESS; } } diff --git a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c index 3d71760fef99..a1150856f6ba 100644 --- a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c +++ b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c @@ -98,7 +98,8 @@ BootMonGetFileFromAsciiFileName ( { FileEntry = BOOTMON_FS_FILE_FROM_LINK_THIS (Entry); if (FileEntry->Info != NULL) { - UnicodeStrToAsciiStr (FileEntry->Info->FileName, OpenFileAsciiFileName); + UnicodeStrToAsciiStrS (FileEntry->Info->FileName, OpenFileAsciiFileName, + MAX_NAME_LENGTH); AsciiFileNameToCompare = OpenFileAsciiFileName; } else { AsciiFileNameToCompare = FileEntry->HwDescription.Footer.Filename; diff --git a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c index af2fe514f044..4927d987eccf 100644 --- a/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c +++ b/ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c @@ -101,7 +101,8 @@ WriteFileDescription ( Description->Attributes = 1; Description->BlockStart = FileStart / BlockSize; Description->BlockEnd = Description->BlockStart + (FileSize / BlockSize); - AsciiStrCpy (Description->Footer.Filename, FileName); + AsciiStrCpyS (Description->Footer.Filename, + sizeof Description->Footer.Filename, FileName); #ifdef MDE_CPU_ARM Description->Footer.Offset = HW_IMAGE_FOOTER_OFFSET; @@ -294,7 +295,7 @@ BootMonFsFlushFile ( DiskIo = Instance->DiskIo; BlockSize = Media->BlockSize; - UnicodeStrToAsciiStr (Info->FileName, AsciiFileName); + UnicodeStrToAsciiStrS (Info->FileName, AsciiFileName, MAX_NAME_LENGTH); // If the file doesn't exist then find a space for it if (File->HwDescription.RegionCount == 0) { @@ -626,10 +627,7 @@ BootMonFsOpenFile ( Status = EFI_OUT_OF_RESOURCES; goto Error; } - UnicodeStrToAsciiStr (Path, AsciiFileName); - if (AsciiStrSize (AsciiFileName) > MAX_NAME_LENGTH) { - AsciiFileName[MAX_NAME_LENGTH - 1] = '\0'; - } + UnicodeStrToAsciiStrS (Path, AsciiFileName, MAX_NAME_LENGTH); if ((AsciiFileName[0] == '\0') || (AsciiFileName[0] == '.' ) ) { @@ -688,7 +686,7 @@ BootMonFsOpenFile ( Info->FileSize = BootMonFsGetImageLength (File); Info->PhysicalSize = BootMonFsGetPhysicalSize (File); - AsciiStrToUnicodeStr (AsciiFileName, Info->FileName); + AsciiStrToUnicodeStrS (AsciiFileName, Info->FileName, MAX_NAME_LENGTH); File->Info = Info; Info = NULL;
Get rid of functions that are no longer available when defining DISABLE_NEW_DEPRECATED_INTERFACES Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsDir.c | 8 +++----- ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsEntryPoint.c | 3 ++- ArmPlatformPkg/FileSystem/BootMonFs/BootMonFsOpenClose.c | 12 +++++------- 3 files changed, 10 insertions(+), 13 deletions(-) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel