[edk2,5/9] EmbeddedPkg/AndroidFastboot: eliminate deprecated string function calls

Message ID 1477330907-13733-6-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel Oct. 24, 2016, 5:41 p.m.
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/Application/AndroidFastboot/AndroidBootImg.c     | 3 ++-
 EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c | 4 ++--
 2 files changed, 4 insertions(+), 3 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. 25, 2016, 12:40 p.m. | #1
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/Application/AndroidFastboot/AndroidBootImg.c     | 3 ++-

>  EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c | 4 ++--

>  2 files changed, 4 insertions(+), 3 deletions(-)

> 

> diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c

> index bbca90fc08a2..f3e770bcc980 100644

> --- a/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c

> +++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c

> @@ -84,7 +84,8 @@ ParseAndroidBootImg (

>                   + ALIGN_VALUE (Header->KernelSize, Header->PageSize));

>    }

>  

> -  AsciiStrnCpy (KernelArgs, Header->KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE);

> +  AsciiStrnCpyS (KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,

> +    BOOTIMG_KERNEL_ARGS_SIZE);

>  

>    return EFI_SUCCESS;

>  }


This loses the zero padding, but I guess that's okay. Is fine otherwise.

> diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c

> index 9ddc34f57cf4..960218b25241 100644

> --- a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c

> +++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c

> @@ -127,7 +127,7 @@ HandleDownload (

>    if (mDataBuffer == NULL) {

>      SEND_LITERAL ("FAILNot enough memory");

>    } else {

> -    AsciiStrnCpy (Response + 4, NumBytesString, 8);

> +    AsciiStrnCpyS (Response + 4, mNumDataBytes, NumBytesString, 8);

>      mTransport->Send (sizeof(Response), Response, &mFatalSendErrorEvent);

>  

>      mState = ExpectDataState;


I don't think this is right. Here we're trying to copy no more than 8
characters from NumBytesString into Response, after the "DATA" prefix.
mNumDataBytes is the decimal value of NumBytesString, and it's unrelated
to this formatting.

What we could do is

  AsciiStrnCpyS (Response + 4, sizeof Response - 4, NumBytesString, 8)

in order to remain "surgical". However, that's not right again, because
AsciiStrnCpyS() *always* NUL-terminates, and here we only have

  CHAR8       Response[12] = "DATA";

i.e., no room for values above 0x0FFF_FFFF. Another issue is that the
zero padding of AsciiStrnCpy() would be lost, and we actually send the
zero padding to the wire.

So, the real fix is, in my opinion:

* resize Response to 4 + 8 + 1 == 13 bytes,

* format it like this:
  ZeroMem (Response, sizeof Response);
  AsciiSPrint (Response, sizeof Response, "DATA%x",
    (UINT32)mNumDataBytes);

* send it like this:
  mTransport->Send (sizeof Response - 1, Response,
    &mFatalSendErrorEvent);


> @@ -257,7 +257,7 @@ AcceptCmd (

>    }

>  

>    // Commands aren't null-terminated. Let's get a null-terminated version.

> -  AsciiStrnCpy (Command, Data, Size);

> +  AsciiStrnCpyS (Command, sizeof Command, Data, Size);

>    Command[Size] = '\0';

>  

>    // Parse command

> 


This looks good, but the explicit NUL-termination can be dropped, as
AsciiStrnCpyS() enforces that internally.

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

Patch

diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
index bbca90fc08a2..f3e770bcc980 100644
--- a/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
+++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
@@ -84,7 +84,8 @@  ParseAndroidBootImg (
                  + ALIGN_VALUE (Header->KernelSize, Header->PageSize));
   }
 
-  AsciiStrnCpy (KernelArgs, Header->KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE);
+  AsciiStrnCpyS (KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,
+    BOOTIMG_KERNEL_ARGS_SIZE);
 
   return EFI_SUCCESS;
 }
diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
index 9ddc34f57cf4..960218b25241 100644
--- a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
+++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c
@@ -127,7 +127,7 @@  HandleDownload (
   if (mDataBuffer == NULL) {
     SEND_LITERAL ("FAILNot enough memory");
   } else {
-    AsciiStrnCpy (Response + 4, NumBytesString, 8);
+    AsciiStrnCpyS (Response + 4, mNumDataBytes, NumBytesString, 8);
     mTransport->Send (sizeof(Response), Response, &mFatalSendErrorEvent);
 
     mState = ExpectDataState;
@@ -257,7 +257,7 @@  AcceptCmd (
   }
 
   // Commands aren't null-terminated. Let's get a null-terminated version.
-  AsciiStrnCpy (Command, Data, Size);
+  AsciiStrnCpyS (Command, sizeof Command, Data, Size);
   Command[Size] = '\0';
 
   // Parse command