[edk2,8/9] EmbeddedPkg/MmcDxe: eliminate deprecated string function calls

Message ID 1477330907-13733-9-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/Universal/MmcDxe/Diagnostics.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
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, 1:49 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/Universal/MmcDxe/Diagnostics.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c b/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c

> index 783e548d2aed..b489393e27b0 100644

> --- a/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c

> +++ b/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c

> @@ -44,7 +44,7 @@ DiagnosticLog (

>    UINTN len = StrLen (Str);

>    if (len <= mLogRemainChar) {

>      mLogRemainChar -= len;

> -    StrCpy (mLogBuffer, Str);

> +    StrCpyS (mLogBuffer, mLogRemainChar, Str);

>      mLogBuffer += len;

>      return len;

>    } else {

> 


I think this is incorrect: "mLogRemainChar" is decreased before printing.

Also, the original controlling expression is broken: it will permit the
body with (len == mLogRemainChar). The effect thereof, for the original
code, is a buffer overflow, the terminating L'\0' will be written
outside of the buffer. (See the allocation in DiagnosticInitLog().)

This buffer overflow is actually fixed by the patch (by throwing away
the last character and storing an L'\0' instead). However, that in turn
invalidates the returned value (because not all "len" characters have
been written).

So, please move the decrement after the StrCpyS(), and replace the <=
with <.

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

Patch

diff --git a/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c b/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c
index 783e548d2aed..b489393e27b0 100644
--- a/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c
+++ b/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c
@@ -44,7 +44,7 @@  DiagnosticLog (
   UINTN len = StrLen (Str);
   if (len <= mLogRemainChar) {
     mLogRemainChar -= len;
-    StrCpy (mLogBuffer, Str);
+    StrCpyS (mLogBuffer, mLogRemainChar, Str);
     mLogBuffer += len;
     return len;
   } else {