Re: [PATCH v2 4/6] ArmPkg/SemihostFs: eliminate calls to deprecated string functions

[Ryan Harkin <ryan.harkin@linaro.org>]

On 26 October 2016 at 12:55, Laszlo Ersek <lersek@redhat.com> wrote:
> On 10/26/16 10:17, Ard Biesheuvel wrote:
>> Remove calls to deprecated string functions like AsciiStrCpy() and
>> UnicodeStrToAsciiStr()
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c | 20 ++++++++++++--------
>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c b/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c
>> index 6efdad9ebcce..cf94ecd5d56f 100644
>> --- a/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c
>> +++ b/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c
>> @@ -207,11 +207,12 @@ FileOpen (
>>      return EFI_WRITE_PROTECTED;
>>    }
>>
>> -  AsciiFileName = AllocatePool (StrLen (FileName) + 1);
>> +  Length = StrLen (FileName) + 1;
>> +  AsciiFileName = AllocatePool (Length);
>>    if (AsciiFileName == NULL) {
>>      return EFI_OUT_OF_RESOURCES;
>>    }
>> -  UnicodeStrToAsciiStr (FileName, AsciiFileName);
>> +  UnicodeStrToAsciiStrS (FileName, AsciiFileName, Length);
>>
>>    // Opening '/', '\', '.', or the NULL pathname is trying to open the root directory
>>    if ((AsciiStrCmp (AsciiFileName, "\\") == 0) ||
>> @@ -463,7 +464,7 @@ FileDelete (
>>      NameSize = AsciiStrLen (Fcb->FileName);
>>      FileName = AllocatePool (NameSize + 1);
>>
>> -    AsciiStrCpy (FileName, Fcb->FileName);
>> +    AsciiStrCpyS (FileName, NameSize + 1, Fcb->FileName);
>>
>>      // Close the file if it's open.  Disregard return status,
>>      // since it might give an error if the file isn't open.
>> @@ -828,8 +829,10 @@ GetFilesystemInfo (
>>    EFI_FILE_SYSTEM_INFO  *Info;
>>    EFI_STATUS            Status;
>>    UINTN                 ResultSize;
>> +  UINTN                 StringSize;
>>
>> -  ResultSize = SIZE_OF_EFI_FILE_SYSTEM_INFO + StrSize (mSemihostFsLabel);
>> +  StringSize = StrSize (mSemihostFsLabel);
>> +  ResultSize = SIZE_OF_EFI_FILE_SYSTEM_INFO + StringSize;
>>
>>    if (*BufferSize >= ResultSize) {
>>      ZeroMem (Buffer, ResultSize);
>> @@ -843,7 +846,7 @@ GetFilesystemInfo (
>>      Info->FreeSpace  = 0;
>>      Info->BlockSize  = 0;
>>
>> -    StrCpy (Info->VolumeLabel, mSemihostFsLabel);
>> +    CopyMem (Info->VolumeLabel, mSemihostFsLabel, StringSize);
>>    } else {
>>      Status = EFI_BUFFER_TOO_SMALL;
>>    }
>> @@ -903,7 +906,7 @@ FileGetInfo (
>>      ResultSize = StrSize (mSemihostFsLabel);
>>
>>      if (*BufferSize >= ResultSize) {
>> -      StrCpy (Buffer, mSemihostFsLabel);
>> +      CopyMem (Buffer, mSemihostFsLabel, *BufferSize);
>
> This is still wrong; here *BufferSize is the size of the recipient
> buffer, passed in from the caller. As written, the code can overrun the
> *source* buffer. Please use
>
>       CopyMem (Buffer, mSemihostFsLabel, ResultSize);
>
> instead.
>
> With that update:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>

I'll apply this update locally before I test the v2 series.


> Thanks
> Laszlo
>
>>        Status = EFI_SUCCESS;
>>      } else {
>>        Status = EFI_BUFFER_TOO_SMALL;
>> @@ -963,11 +966,12 @@ SetFileInfo (
>>      return EFI_ACCESS_DENIED;
>>    }
>>
>> -  AsciiFileName = AllocatePool (StrLen (Info->FileName) + 1);
>> +  Length = StrLen (Info->FileName) + 1;
>> +  AsciiFileName = AllocatePool (Length);
>>    if (AsciiFileName == NULL) {
>>      return EFI_OUT_OF_RESOURCES;
>>    }
>> -  UnicodeStrToAsciiStr (Info->FileName, AsciiFileName);
>> +  UnicodeStrToAsciiStrS (Info->FileName, AsciiFileName, Length);
>>
>>    FileSizeIsDifferent = (Info->FileSize != Fcb->Info.FileSize);
>>    FileNameIsDifferent = (AsciiStrCmp (AsciiFileName, Fcb->FileName) != 0);
>>
>