Re: [PATCH 8/9] EmbeddedPkg/MmcDxe: eliminate deprecated string function calls

[Laszlo Ersek <lersek@redhat.com>]

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