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 383151A1E6B for ; Tue, 25 Oct 2016 06:34:25 -0700 (PDT) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (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 81485550B2; Tue, 25 Oct 2016 13:34:24 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-71.phx2.redhat.com [10.3.116.71]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9PDYMf9005338; Tue, 25 Oct 2016 09:34:23 -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-7-git-send-email-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: <6783b428-fe3f-69a2-2e58-faf03ef093d6@redhat.com> Date: Tue, 25 Oct 2016 15:34:22 +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-7-git-send-email-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Tue, 25 Oct 2016 13:34:24 +0000 (UTC) Subject: Re: [PATCH 6/9] EmbeddedPkg/Ebl: 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:34:25 -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/Ebl/Command.c | 2 +- > EmbeddedPkg/Ebl/Dir.c | 4 ++-- > EmbeddedPkg/Ebl/EfiDevice.c | 11 ++++++----- > EmbeddedPkg/Ebl/Main.c | 8 ++++---- > EmbeddedPkg/Ebl/Variable.c | 17 +++++++++++------ > 5 files changed, 24 insertions(+), 18 deletions(-) > > diff --git a/EmbeddedPkg/Ebl/Command.c b/EmbeddedPkg/Ebl/Command.c > index e75c6a2e5c32..4bc1f4df0ca0 100644 > --- a/EmbeddedPkg/Ebl/Command.c > +++ b/EmbeddedPkg/Ebl/Command.c > @@ -614,7 +614,7 @@ OutputData ( > UINTN Spaces = 0; > CHAR8 Blanks[80]; > > - AsciiStrCpy (Blanks, mBlanks); > + AsciiStrCpyS (Blanks, sizeof Blanks, mBlanks); The original code is so poor that it makes me weep. (Not knowing who wrote this, and I don't want to look it up.) The mBlanks array has 43 (forty three) space characters in it. Please drop the mBlanks array, and use a SetMem() call here. Or... meh, I don't care. It's fine as it is. > for (EndAddress = Address + Length; Address < EndAddress; Offset += Line) { > AsciiPrint ("%08x: ", Offset); > for (Line = 0; (Line < 0x10) && (Address < EndAddress);) { > diff --git a/EmbeddedPkg/Ebl/Dir.c b/EmbeddedPkg/Ebl/Dir.c > index 36095b633019..8dd9d48ff6ac 100644 > --- a/EmbeddedPkg/Ebl/Dir.c > +++ b/EmbeddedPkg/Ebl/Dir.c > @@ -116,7 +116,7 @@ EblDirCmd ( > UnicodeFileName[0] = '\0'; > MatchSubString = &UnicodeFileName[0]; > if (Argc > 2) { > - AsciiStrToUnicodeStr (Argv[2], UnicodeFileName); > + AsciiStrToUnicodeStrS (Argv[2], UnicodeFileName, MAX_CMD_LINE); > if (UnicodeFileName[0] == '*') { > // Handle *Name substring matching > MatchSubString = &UnicodeFileName[1]; Looks okay. > @@ -231,7 +231,7 @@ EblDirCmd ( > MatchSubString = NULL; > UnicodeFileName[0] = '\0'; > if (Argc > 2) { > - AsciiStrToUnicodeStr (Argv[2], UnicodeFileName); > + AsciiStrToUnicodeStrS (Argv[2], UnicodeFileName, MAX_CMD_LINE); > if (UnicodeFileName[0] == '*') { > MatchSubString = &UnicodeFileName[1]; > } Ditto. > diff --git a/EmbeddedPkg/Ebl/EfiDevice.c b/EmbeddedPkg/Ebl/EfiDevice.c > index ec9c331b7004..f6969e7b2b05 100644 > --- a/EmbeddedPkg/Ebl/EfiDevice.c > +++ b/EmbeddedPkg/Ebl/EfiDevice.c > @@ -343,7 +343,7 @@ EblStartCmd ( > > ImageInfo->LoadOptionsSize = (UINT32)AsciiStrSize (Argv[2]); > ImageInfo->LoadOptions = AllocatePool (ImageInfo->LoadOptionsSize); > - AsciiStrCpy (ImageInfo->LoadOptions, Argv[2]); > + AsciiStrCpyS (ImageInfo->LoadOptions, ImageInfo->LoadOptionsSize, Argv[2]); > } > > // Transfer control to the EFI image we loaded with LoadImage() AllocateCopyPool() would be much better, but this will do. > @@ -741,7 +741,7 @@ EblFileCopyCmd ( > UINTN Size; > UINTN Offset; > UINTN Chunk = FILE_COPY_CHUNK; > - UINTN FileNameLen; > + UINTN FileNameLen, DestFileNameLen; > CHAR8* DestFileName; > CHAR8* SrcFileName; > CHAR8* SrcPtr; > @@ -786,9 +786,10 @@ EblFileCopyCmd ( > } > > // Construct the destination filepath > - DestFileName = (CHAR8*)AllocatePool (FileNameLen + AsciiStrLen (SrcFileName) + 1); > - AsciiStrCpy (DestFileName, Argv[2]); > - AsciiStrCat (DestFileName, SrcFileName); > + DestFileNameLen = FileNameLen + AsciiStrLen (SrcFileName) + 1; > + DestFileName = (CHAR8*)AllocatePool (DestFileNameLen); > + AsciiStrCpyS (DestFileName, DestFileNameLen, Argv[2]); > + AsciiStrCatS (DestFileName, DestFileNameLen, SrcFileName); > } > > Source = EfiOpen(Argv[1], EFI_FILE_MODE_READ, 0); AsciiSPrint would be (and would have been, originally) superior, but this will do. > diff --git a/EmbeddedPkg/Ebl/Main.c b/EmbeddedPkg/Ebl/Main.c > index 18b2878f69a1..4f04ae4afd33 100644 > --- a/EmbeddedPkg/Ebl/Main.c > +++ b/EmbeddedPkg/Ebl/Main.c > @@ -88,7 +88,7 @@ SetCmdHistory ( > } > > // Copy the new command line into the ring buffer > - AsciiStrnCpy(&mCmdHistory[mCmdHistoryStart][0], Cmd, MAX_CMD_LINE); > + AsciiStrnCpyS (&mCmdHistory[mCmdHistoryStart][0], MAX_CMD_LINE, Cmd, MAX_CMD_LINE); > } > > // Reset the command history for the next up arrow press > @@ -432,7 +432,7 @@ GetCmd ( > } > AsciiPrint (History); > Index = AsciiStrLen (History); > - AsciiStrnCpy (Cmd, History, CmdMaxSize); > + AsciiStrnCpyS (Cmd, CmdMaxSize, History, CmdMaxSize); > } else { > Cmd[Index++] = Char; > if (FixedPcdGetBool(PcdEmbeddedShellCharacterEcho) == TRUE) { > @@ -644,14 +644,14 @@ EdkBootLoaderEntry ( > > Status = gRT->GetVariable(CommandLineVariableName, &VendorGuid, NULL, &CommandLineVariableSize, CommandLineVariable); > if (!EFI_ERROR(Status)) { > - UnicodeStrToAsciiStr(CommandLineVariable, CmdLine); > + UnicodeStrToAsciiStrS (CommandLineVariable, CmdLine, CommandLineVariableSize); > } This is wrong, the last argument should be MAX_CMD_LINE. CommandLineVariableSize is the size (incl. the terminating NUL) of the UCS2 string read from the variable. The last argument of UnicodeStrToAsciiStrS() is DestMax, "The maximum number of Destination scii char, including terminating null char.". That is, MAX_CMD_LINE. > > FreePool(CommandLineVariable); > } > > if (EFI_ERROR(Status)) { > - AsciiStrCpy (CmdLine, (CHAR8 *)PcdGetPtr (PcdEmbeddedAutomaticBootCommand)); > + AsciiStrCpyS (CmdLine, MAX_CMD_LINE, (CHAR8 *)PcdGetPtr (PcdEmbeddedAutomaticBootCommand)); > } > > for (;;) { This looks good. > diff --git a/EmbeddedPkg/Ebl/Variable.c b/EmbeddedPkg/Ebl/Variable.c > index f440c48f16dd..b94531300d07 100644 > --- a/EmbeddedPkg/Ebl/Variable.c > +++ b/EmbeddedPkg/Ebl/Variable.c > @@ -29,6 +29,7 @@ EblGetCmd ( > VOID* Value; > CHAR8* AsciiVariableName = NULL; > CHAR16* VariableName; > + UINTN VariableNameLen; > UINT32 Index; > > if (Argc == 1) { > @@ -48,8 +49,9 @@ EblGetCmd ( > AsciiPrint("Variable name is missing.\n"); > return Status; > } else { > - VariableName = AllocatePool((AsciiStrLen (AsciiVariableName) + 1) * sizeof (CHAR16)); > - AsciiStrToUnicodeStr (AsciiVariableName,VariableName); > + VariableNameLen = (AsciiStrLen (AsciiVariableName) + 1) * sizeof (CHAR16); > + VariableName = AllocatePool(VariableNameLen); > + AsciiStrToUnicodeStrS (AsciiVariableName, VariableName, VariableNameLen); > } > > // Try to get the variable size. This is wrong. VariableNameLen is the size in bytes of a CHAR16 array (incl. the terminating L'\0'). The last argument of AsciiStrToUnicodeStrS(), is DestMax: "The maximum number of Destination Unicode char, including terminating null char." > @@ -93,6 +95,7 @@ EblSetCmd ( > CHAR8* AsciiValue; > UINT32 AsciiValueLength; > CHAR16* VariableName; > + UINTN VariableNameLen; > UINT32 Index; > UINT32 EscapedQuotes = 0; > BOOLEAN Volatile = FALSE; > @@ -125,8 +128,9 @@ EblSetCmd ( > // > > // Convert VariableName into Unicode > - VariableName = AllocatePool((AsciiStrLen (AsciiVariableSetting) + 1) * sizeof (CHAR16)); > - AsciiStrToUnicodeStr (AsciiVariableSetting,VariableName); > + VariableNameLen = (AsciiStrLen (AsciiVariableSetting) + 1) * sizeof (CHAR16); > + VariableName = AllocatePool(VariableNameLen); > + AsciiStrToUnicodeStrS (AsciiVariableSetting, VariableName, VariableNameLen); > > Status = gRT->SetVariable ( > VariableName, Same issue here. > @@ -170,8 +174,9 @@ EblSetCmd ( > } > > // Convert VariableName into Unicode > - VariableName = AllocatePool((AsciiStrLen (AsciiVariableName) + 1) * sizeof (CHAR16)); > - AsciiStrToUnicodeStr (AsciiVariableName,VariableName); > + VariableNameLen = (AsciiStrLen (AsciiVariableName) + 1) * sizeof (CHAR16); > + VariableName = AllocatePool(VariableNameLen); > + AsciiStrToUnicodeStrS (AsciiVariableName, VariableName, VariableNameLen); > > Status = gRT->SetVariable ( > VariableName, > And here. Thanks Laszlo