diff mbox

[edk2,v2,4/6] ArmPkg/SemihostFs: eliminate calls to deprecated string functions

Message ID 1477469862-10046-5-git-send-email-ard.biesheuvel@linaro.org
State Superseded
Headers show

Commit Message

Ard Biesheuvel Oct. 26, 2016, 8:17 a.m. UTC 
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(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek Oct. 26, 2016, 11:55 a.m. UTC | #1 
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>


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);

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ryan Harkin Oct. 26, 2016, 12:24 p.m. UTC | #2 
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);

>>

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

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);
       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);