From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9CDFF1A1E6B for ; Tue, 25 Oct 2016 06:49:45 -0700 (PDT) Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0D19D80084; Tue, 25 Oct 2016 13:49:45 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-71.phx2.redhat.com [10.3.116.71]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9PDnhBp024312; Tue, 25 Oct 2016 09:49:44 -0400 To: Ard Biesheuvel , edk2-devel@ml01.01.org, leif.lindholm@linaro.org References: <1477330907-13733-1-git-send-email-ard.biesheuvel@linaro.org> <1477330907-13733-9-git-send-email-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: <8040c699-b2e6-a5c0-4dd6-b533ae1a389d@redhat.com> Date: Tue, 25 Oct 2016 15:49:43 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1477330907-13733-9-git-send-email-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Tue, 25 Oct 2016 13:49:45 +0000 (UTC) Subject: Re: [PATCH 8/9] EmbeddedPkg/MmcDxe: eliminate deprecated string function calls X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Oct 2016 13:49:45 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 > --- > 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