* [PATCH 1/9] EmbeddedPkg/AndroidFastbootTransportTcpDxe: remove broken hostname handling
2016-10-24 17:41 [PATCH 0/9] EmbeddedPkg: eliminate calls to deprecated functions Ard Biesheuvel
@ 2016-10-24 17:41 ` Ard Biesheuvel
2016-10-24 17:41 ` [PATCH 2/9] EmbeddedPkg: remove unused PrePiHobListPointerLib Ard Biesheuvel
` (7 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 17:41 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
The fastboot TCP connection setup routine retrieves a hostname from a
UEFI variable 'hostname' that is scoped under a GUID gEfiHostnameVariableGuid
whose definition is missing from the code. Since the hostname is only printed
and then discarded, let's just drop the whole thing.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcp.c | 23 --------------------
EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcpDxe.inf | 3 ---
2 files changed, 26 deletions(-)
diff --git a/EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcp.c b/EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcp.c
index 8ec78aeb534e..7c008ac9722e 100644
--- a/EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcp.c
+++ b/EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcp.c
@@ -27,8 +27,6 @@
#include <Library/UefiDriverEntryPoint.h>
#include <Library/UefiRuntimeServicesTableLib.h>
-#include <Guid/Hostname.h>
-
#define IP4_ADDR_TO_STRING(IpAddr, IpAddrString) UnicodeSPrint ( \
IpAddrString, \
16 * 2, \
@@ -307,9 +305,6 @@ TcpFastbootTransportStart (
EFI_HANDLE *HandleBuffer;
EFI_IP4_MODE_DATA Ip4ModeData;
UINTN NumHandles;
- UINTN HostnameSize = 256;
- CHAR8 Hostname[256];
- CHAR16 HostnameUnicode[256] = L"<no hostname>";
CHAR16 IpAddrString[16];
UINTN Index;
@@ -442,28 +437,10 @@ TcpFastbootTransportStart (
//
IP4_ADDR_TO_STRING (Ip4ModeData.ConfigData.StationAddress, IpAddrString);
- // Look up hostname
- Status = gRT->GetVariable (
- L"Hostname",
- &gEfiHostnameVariableGuid,
- NULL,
- &HostnameSize,
- &Hostname
- );
- if (!EFI_ERROR (Status) && HostnameSize != 0) {
- AsciiStrToUnicodeStr (Hostname, HostnameUnicode);
- }
-
- // Hostname variable is not null-terminated.
- Hostname[HostnameSize] = L'\0';
-
mTextOut->OutputString (mTextOut, L"TCP Fastboot transport configured.");
mTextOut->OutputString (mTextOut, L"\r\nIP address: ");
mTextOut->OutputString (mTextOut ,IpAddrString);
mTextOut->OutputString (mTextOut, L"\r\n");
- mTextOut->OutputString (mTextOut, L"\r\nhostname: ");
- mTextOut->OutputString (mTextOut, HostnameUnicode);
- mTextOut->OutputString (mTextOut, L"\r\n");
//
// Start listening for a connection
diff --git a/EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcpDxe.inf b/EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcpDxe.inf
index 4d777934e5a4..89ff556b40ca 100644
--- a/EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcpDxe.inf
+++ b/EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcpDxe.inf
@@ -47,8 +47,5 @@ [Packages]
MdeModulePkg/MdeModulePkg.dec
EmbeddedPkg/EmbeddedPkg.dec
-[Guids]
- gEfiHostnameVariableGuid
-
[FixedPcd]
gEmbeddedTokenSpaceGuid.PcdAndroidFastbootTcpPort
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 2/9] EmbeddedPkg: remove unused PrePiHobListPointerLib
2016-10-24 17:41 [PATCH 0/9] EmbeddedPkg: eliminate calls to deprecated functions Ard Biesheuvel
2016-10-24 17:41 ` [PATCH 1/9] EmbeddedPkg/AndroidFastbootTransportTcpDxe: remove broken hostname handling Ard Biesheuvel
@ 2016-10-24 17:41 ` Ard Biesheuvel
2016-10-24 17:41 ` [PATCH 3/9] EmbeddedPkg: add missing modules Ard Biesheuvel
` (6 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 17:41 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
Remove this unused version: all existing platforms use the one under
ArmPlatformPkg instead.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
EmbeddedPkg/EmbeddedPkg.dec | 1 -
EmbeddedPkg/EmbeddedPkg.dsc | 1 -
EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointer.c | 69 --------------------
EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf | 38 -----------
Omap35xxPkg/Omap35xxPkg.dsc | 1 -
5 files changed, 110 deletions(-)
diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index 775d863c5f64..2c2cf41103c2 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -97,7 +97,6 @@ [PcdsFixedAtBuild.common]
gEmbeddedTokenSpaceGuid.PcdEmbeddedMemVariableStoreSize|0x10000|UINT32|0x00000009
gEmbeddedTokenSpaceGuid.PcdEmbeddedPrompt|"Ebl"|VOID*|0x00000034
- gEmbeddedTokenSpaceGuid.PcdPrePiHobBase|131072|UINT32|0x00000040
gEmbeddedTokenSpaceGuid.PcdPrePiStackBase|0|UINT32|0x0000000b
gEmbeddedTokenSpaceGuid.PcdPrePiStackSize|131072|UINT32|0x0000000c
diff --git a/EmbeddedPkg/EmbeddedPkg.dsc b/EmbeddedPkg/EmbeddedPkg.dsc
index cd1f6df06b24..5224bcefa9c7 100644
--- a/EmbeddedPkg/EmbeddedPkg.dsc
+++ b/EmbeddedPkg/EmbeddedPkg.dsc
@@ -184,7 +184,6 @@ [PcdsFixedAtBuild.common]
gEmbeddedTokenSpaceGuid.PcdEmbeddedDefaultTextColor|0x07
gEmbeddedTokenSpaceGuid.PcdEmbeddedMemVariableStoreSize|0x10000
- gEmbeddedTokenSpaceGuid.PcdPrePiHobBase|0
gEmbeddedTokenSpaceGuid.PcdPrePiStackBase|0
gEmbeddedTokenSpaceGuid.PcdPrePiStackSize|0
diff --git a/EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointer.c b/EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointer.c
deleted file mode 100644
index 9065b1132fda..000000000000
--- a/EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointer.c
+++ /dev/null
@@ -1,69 +0,0 @@
-/** @file
-*
-* Copyright (c) 2011, ARM Limited. All rights reserved.
-*
-* This program and the accompanying materials
-* are licensed and made available under the terms and conditions of the BSD License
-* which accompanies this distribution. The full text of the license may be found at
-* http://opensource.org/licenses/bsd-license.php
-*
-* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-*
-**/
-
-#include <PiPei.h>
-#include <Library/PrePiHobListPointerLib.h>
-#include <Library/DebugLib.h>
-#include <Library/PcdLib.h>
-
-//
-// Have to use build system to set the original value in case we are running
-// from FLASH and globals don't work. So if you do a GetHobList() and gHobList
-// and gHobList is NULL the PCD default values are used.
-//
-VOID *gHobList = NULL;
-
-
-/**
- Returns the pointer to the HOB list.
-
- This function returns the pointer to first HOB in the list.
-
- @return The pointer to the HOB list.
-
-**/
-VOID *
-EFIAPI
-PrePeiGetHobList (
- VOID
- )
-{
- if (gHobList == NULL) {
- return (VOID *)*(UINTN*)PcdGet32 (PcdPrePiHobBase);
- } else {
- return gHobList;
- }
-}
-
-
-
-/**
- Updates the pointer to the HOB list.
-
- @param HobList Hob list pointer to store
-
-**/
-EFI_STATUS
-EFIAPI
-PrePeiSetHobList (
- IN VOID *HobList
- )
-{
- gHobList = HobList;
-
- //
- // If this code is running from ROM this could fail
- //
- return (gHobList == HobList) ? EFI_SUCCESS: EFI_UNSUPPORTED;
-}
diff --git a/EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf b/EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
deleted file mode 100644
index bfd3c209a13b..000000000000
--- a/EmbeddedPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
+++ /dev/null
@@ -1,38 +0,0 @@
-#/** @file
-#
-# Copyright (c) 2011, ARM Limited. All rights reserved.
-#
-# This program and the accompanying materials
-# are licensed and made available under the terms and conditions of the BSD License
-# which accompanies this distribution. The full text of the license may be found at
-# http://opensource.org/licenses/bsd-license.php
-#
-# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-#
-#**/
-
-[Defines]
- INF_VERSION = 0x00010005
- BASE_NAME = PrePiHobListPointerLib
- FILE_GUID = 7a6a7fc0-5ee0-11e0-b47f-0002a5d5c51b
- MODULE_TYPE = BASE
- VERSION_STRING = 1.0
- LIBRARY_CLASS = PrePiHobListPointerLib
-
-#
-# VALID_ARCHITECTURES = IA32 X64 IPF EBC (EBC is for build only)
-#
-
-[Sources]
- PrePiHobListPointer.c
-
-[Packages]
- MdePkg/MdePkg.dec
- EmbeddedPkg/EmbeddedPkg.dec
-
-[LibraryClasses]
- #DebugLib
-
-[FixedPcd.common]
- gEmbeddedTokenSpaceGuid.PcdPrePiHobBase
diff --git a/Omap35xxPkg/Omap35xxPkg.dsc b/Omap35xxPkg/Omap35xxPkg.dsc
index 11259191e1aa..2b339253ff9c 100644
--- a/Omap35xxPkg/Omap35xxPkg.dsc
+++ b/Omap35xxPkg/Omap35xxPkg.dsc
@@ -145,7 +145,6 @@ [PcdsFixedAtBuild.common]
gEmbeddedTokenSpaceGuid.PcdPrePiBfvSize|0
gEmbeddedTokenSpaceGuid.PcdFlashFvMainBase|0
gEmbeddedTokenSpaceGuid.PcdFlashFvMainSize|0
- gEmbeddedTokenSpaceGuid.PcdPrePiHobBase|0x80001000
gEmbeddedTokenSpaceGuid.PcdPrePiStackBase|0x87FE0000 # stack at top of memory
gEmbeddedTokenSpaceGuid.PcdPrePiStackSize|0x20000 # 128K stack
gArmTokenSpaceGuid.PcdCpuVectorBaseAddress|0x80000000
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 3/9] EmbeddedPkg: add missing modules
2016-10-24 17:41 [PATCH 0/9] EmbeddedPkg: eliminate calls to deprecated functions Ard Biesheuvel
2016-10-24 17:41 ` [PATCH 1/9] EmbeddedPkg/AndroidFastbootTransportTcpDxe: remove broken hostname handling Ard Biesheuvel
2016-10-24 17:41 ` [PATCH 2/9] EmbeddedPkg: remove unused PrePiHobListPointerLib Ard Biesheuvel
@ 2016-10-24 17:41 ` Ard Biesheuvel
2016-10-24 17:41 ` [PATCH 4/9] EmbeddedPkg/GdbDebugAgent: fix VOID* cast of incorrect size Ard Biesheuvel
` (5 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 17:41 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
Add modules that live under EmbeddedPkg but were missing from the
[Components] section of the package .dsc file
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
EmbeddedPkg/EmbeddedPkg.dsc | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/EmbeddedPkg/EmbeddedPkg.dsc b/EmbeddedPkg/EmbeddedPkg.dsc
index 5224bcefa9c7..cc95564f31af 100644
--- a/EmbeddedPkg/EmbeddedPkg.dsc
+++ b/EmbeddedPkg/EmbeddedPkg.dsc
@@ -280,5 +280,14 @@ [Components.common]
EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.inf
EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132Dxe.inf
+ EmbeddedPkg/Library/AcpiLib/AcpiLib.inf
+ EmbeddedPkg/Library/DebugAgentTimerLibNull/DebugAgentTimerLibNull.inf
+ EmbeddedPkg/Library/DxeHobPeCoffLib/DxeHobPeCoffLib.inf
+ EmbeddedPkg/Library/EblNetworkLib/EblNetworkLib.inf
+ EmbeddedPkg/Library/FdtLib/FdtLib.inf
+ EmbeddedPkg/Library/GdbDebugAgent/GdbDebugAgent.inf
+ EmbeddedPkg/Library/PrePiHobLib/PrePiHobLib.inf
+ EmbeddedPkg/Library/PrePiMemoryAllocationLib/PrePiMemoryAllocationLib.inf
+
[Components.IA32, Components.X64, Components.IPF, Components.ARM]
EmbeddedPkg/GdbStub/GdbStub.inf
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 4/9] EmbeddedPkg/GdbDebugAgent: fix VOID* cast of incorrect size
2016-10-24 17:41 [PATCH 0/9] EmbeddedPkg: eliminate calls to deprecated functions Ard Biesheuvel
` (2 preceding siblings ...)
2016-10-24 17:41 ` [PATCH 3/9] EmbeddedPkg: add missing modules Ard Biesheuvel
@ 2016-10-24 17:41 ` Ard Biesheuvel
2016-10-25 12:16 ` Laszlo Ersek
2016-10-24 17:41 ` [PATCH 5/9] EmbeddedPkg/AndroidFastboot: eliminate deprecated string function calls Ard Biesheuvel
` (4 subsequent siblings)
8 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 17:41 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
The result of PcdGet64() can only be cast to VOID* on 64-bit platforms,
so add an intermediate UINTN cast to make this code build again on 32 bit.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c b/EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c
index 8226c80ab466..a43ff95d03a3 100644
--- a/EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c
+++ b/EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c
@@ -657,7 +657,7 @@ InitializeDebugAgent (
*(UINTN *) (((UINT8 *)VectorBase) + Offset) = (UINTN)AsmCommonExceptionEntry;
// Flush Caches since we updated executable stuff
- InvalidateInstructionCacheRange ((VOID *)PcdGet64(PcdCpuVectorBaseAddress), Length);
+ InvalidateInstructionCacheRange ((VOID *)(UINTN)PcdGet64(PcdCpuVectorBaseAddress), Length);
// setup a timer so gdb can break in via ctrl-c
DebugAgentTimerIntialize ();
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 4/9] EmbeddedPkg/GdbDebugAgent: fix VOID* cast of incorrect size
2016-10-24 17:41 ` [PATCH 4/9] EmbeddedPkg/GdbDebugAgent: fix VOID* cast of incorrect size Ard Biesheuvel
@ 2016-10-25 12:16 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2016-10-25 12:16 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel, leif.lindholm
On 10/24/16 19:41, Ard Biesheuvel wrote:
> The result of PcdGet64() can only be cast to VOID* on 64-bit platforms,
> so add an intermediate UINTN cast to make this code build again on 32 bit.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c b/EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c
> index 8226c80ab466..a43ff95d03a3 100644
> --- a/EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c
> +++ b/EmbeddedPkg/Library/GdbDebugAgent/Arm/Processor.c
> @@ -657,7 +657,7 @@ InitializeDebugAgent (
> *(UINTN *) (((UINT8 *)VectorBase) + Offset) = (UINTN)AsmCommonExceptionEntry;
>
> // Flush Caches since we updated executable stuff
> - InvalidateInstructionCacheRange ((VOID *)PcdGet64(PcdCpuVectorBaseAddress), Length);
> + InvalidateInstructionCacheRange ((VOID *)(UINTN)PcdGet64(PcdCpuVectorBaseAddress), Length);
>
> // setup a timer so gdb can break in via ctrl-c
> DebugAgentTimerIntialize ();
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 5/9] EmbeddedPkg/AndroidFastboot: eliminate deprecated string function calls
2016-10-24 17:41 [PATCH 0/9] EmbeddedPkg: eliminate calls to deprecated functions Ard Biesheuvel
` (3 preceding siblings ...)
2016-10-24 17:41 ` [PATCH 4/9] EmbeddedPkg/GdbDebugAgent: fix VOID* cast of incorrect size Ard Biesheuvel
@ 2016-10-24 17:41 ` Ard Biesheuvel
2016-10-25 12:40 ` Laszlo Ersek
2016-10-24 17:41 ` [PATCH 6/9] EmbeddedPkg/Ebl: " Ard Biesheuvel
` (3 subsequent siblings)
8 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 17:41 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
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;
}
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
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 5/9] EmbeddedPkg/AndroidFastboot: eliminate deprecated string function calls
2016-10-24 17:41 ` [PATCH 5/9] EmbeddedPkg/AndroidFastboot: eliminate deprecated string function calls Ard Biesheuvel
@ 2016-10-25 12:40 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2016-10-25 12:40 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel, leif.lindholm
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 6/9] EmbeddedPkg/Ebl: eliminate deprecated string function calls
2016-10-24 17:41 [PATCH 0/9] EmbeddedPkg: eliminate calls to deprecated functions Ard Biesheuvel
` (4 preceding siblings ...)
2016-10-24 17:41 ` [PATCH 5/9] EmbeddedPkg/AndroidFastboot: eliminate deprecated string function calls Ard Biesheuvel
@ 2016-10-24 17:41 ` Ard Biesheuvel
2016-10-25 13:34 ` Laszlo Ersek
2016-10-24 17:41 ` [PATCH 7/9] EmbeddedPkg/EfiFileLib: " Ard Biesheuvel
` (2 subsequent siblings)
8 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 17:41 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
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/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);
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];
@@ -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];
}
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()
@@ -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);
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);
}
FreePool(CommandLineVariable);
}
if (EFI_ERROR(Status)) {
- AsciiStrCpy (CmdLine, (CHAR8 *)PcdGetPtr (PcdEmbeddedAutomaticBootCommand));
+ AsciiStrCpyS (CmdLine, MAX_CMD_LINE, (CHAR8 *)PcdGetPtr (PcdEmbeddedAutomaticBootCommand));
}
for (;;) {
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.
@@ -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,
@@ -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,
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 6/9] EmbeddedPkg/Ebl: eliminate deprecated string function calls
2016-10-24 17:41 ` [PATCH 6/9] EmbeddedPkg/Ebl: " Ard Biesheuvel
@ 2016-10-25 13:34 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2016-10-25 13:34 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel, leif.lindholm
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/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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 7/9] EmbeddedPkg/EfiFileLib: eliminate deprecated string function calls
2016-10-24 17:41 [PATCH 0/9] EmbeddedPkg: eliminate calls to deprecated functions Ard Biesheuvel
` (5 preceding siblings ...)
2016-10-24 17:41 ` [PATCH 6/9] EmbeddedPkg/Ebl: " Ard Biesheuvel
@ 2016-10-24 17:41 ` Ard Biesheuvel
2016-10-25 14:20 ` Laszlo Ersek
2016-10-24 17:41 ` [PATCH 8/9] EmbeddedPkg/MmcDxe: " Ard Biesheuvel
2016-10-24 17:41 ` [PATCH 9/9] EmbeddedPkg: enable -DDISABLE_NEW_DEPRECATED_INTERFACES Ard Biesheuvel
8 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 17:41 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
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/Library/EfiFileLib/EfiFileLib.c | 42 +++++++++++---------
1 file changed, 23 insertions(+), 19 deletions(-)
diff --git a/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c b/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c
index 4d58c830861c..d3b65aa5a3e0 100644
--- a/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c
+++ b/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c
@@ -384,9 +384,9 @@ EblFileDevicePath (
if ( *FileName != 0 ) {
- AsciiStrToUnicodeStr (FileName, UnicodeFileName);
+ AsciiStrToUnicodeStrS (FileName, UnicodeFileName, MAX_PATHNAME);
} else {
- AsciiStrToUnicodeStr ("\\", UnicodeFileName);
+ AsciiStrToUnicodeStrS ("\\", UnicodeFileName, MAX_PATHNAME);
}
Size = StrSize (UnicodeFileName);
@@ -589,7 +589,7 @@ EblFvFileDevicePath (
&AuthenticationStatus
);
if (!EFI_ERROR (Status)) {
- UnicodeStrToAsciiStr (Section, AsciiSection);
+ UnicodeStrToAsciiStrS (Section, AsciiSection, MAX_PATHNAME);
if (AsciiStriCmp (FileName, AsciiSection) == 0) {
FreePool (Section);
break;
@@ -674,6 +674,7 @@ EfiOpen (
CHAR8 *CwdPlusPathName;
UINTN Index;
EFI_SECTION_TYPE ModifiedSectionType;
+ UINTN AsciiLength;
EblUpdateDeviceLists ();
@@ -706,7 +707,8 @@ EfiOpen (
}
// We could add a current working directory concept
- CwdPlusPathName = AllocatePool (AsciiStrSize (gCwd) + AsciiStrSize (PathName));
+ AsciiLength = AsciiStrSize (gCwd) + AsciiStrSize (PathName);
+ CwdPlusPathName = AllocatePool (AsciiLength);
if (CwdPlusPathName == NULL) {
return NULL;
}
@@ -723,14 +725,14 @@ EfiOpen (
}
}
} else {
- AsciiStrCpy (CwdPlusPathName, gCwd);
+ AsciiStrCpyS (CwdPlusPathName, AsciiLength, gCwd);
StrLen = AsciiStrLen (gCwd);
if ((*PathName != '/') && (*PathName != '\\') && (gCwd[StrLen-1] != '/') && (gCwd[StrLen-1] != '\\')) {
- AsciiStrCat (CwdPlusPathName, "\\");
+ AsciiStrCatS (CwdPlusPathName, AsciiLength, "\\");
}
}
- AsciiStrCat (CwdPlusPathName, PathName);
+ AsciiStrCatS (CwdPlusPathName, AsciiLength, PathName);
if (AsciiStrStr (CwdPlusPathName, ":") == NULL) {
// Extra error check to make sure we don't recurse and blow stack
return NULL;
@@ -745,7 +747,7 @@ EfiOpen (
}
File->DeviceName = AllocatePool (StrLen);
- AsciiStrCpy (File->DeviceName, PathName);
+ AsciiStrCpyS (File->DeviceName, StrLen, PathName);
File->DeviceName[FileStart - 1] = '\0';
File->FileName = &File->DeviceName[FileStart];
if (File->FileName[0] == '\0') {
@@ -1611,7 +1613,7 @@ ExpandPath (
{
CHAR8 *NewPath;
CHAR8 *Work, *Start, *End;
- UINTN StrLen;
+ UINTN StrLen, AllocLen;
INTN i;
if (Cwd == NULL || Path == NULL) {
@@ -1625,11 +1627,12 @@ ExpandPath (
}
StrLen = AsciiStrSize (Path);
- NewPath = AllocatePool (AsciiStrSize (Cwd) + StrLen + 1);
+ AllocLen = AsciiStrSize (Cwd) + StrLen + 1;
+ NewPath = AllocatePool (AllocLen);
if (NewPath == NULL) {
return NULL;
}
- AsciiStrCpy (NewPath, Cwd);
+ AsciiStrCpyS (NewPath, AllocLen, Cwd);
End = Path + StrLen;
for (Start = Path ;;) {
@@ -1640,7 +1643,7 @@ ExpandPath (
}
// append path prior to ..
- AsciiStrnCat (NewPath, Start, Work - Start);
+ AsciiStrnCatS (NewPath, AllocLen, Start, Work - Start);
StrLen = AsciiStrLen (NewPath);
for (i = StrLen; i >= 0; i--) {
if (NewPath[i] == ':') {
@@ -1663,7 +1666,7 @@ ExpandPath (
}
// Handle the path that remains after the ..
- AsciiStrnCat (NewPath, Start, End - Start);
+ AsciiStrnCatS (NewPath, AllocLen, Start, End - Start);
return NewPath;
}
@@ -1686,7 +1689,7 @@ EfiSetCwd (
)
{
EFI_OPEN_FILE *File;
- UINTN Len;
+ UINTN Len, AllocLen;
CHAR8 *Path;
if (Cwd == NULL) {
@@ -1729,17 +1732,18 @@ EfiSetCwd (
// Use the info returned from EfiOpen as it can add in CWD if needed. So Cwd could be
// relative to the current gCwd or not.
- gCwd = AllocatePool (AsciiStrSize (File->DeviceName) + AsciiStrSize (File->FileName) + 10);
+ AllocLen = AsciiStrSize (File->DeviceName) + AsciiStrSize (File->FileName) + 10;
+ gCwd = AllocatePool (AllocLen);
if (gCwd == NULL) {
return EFI_INVALID_PARAMETER;
}
- AsciiStrCpy (gCwd, File->DeviceName);
+ AsciiStrCpyS (gCwd, AllocLen, File->DeviceName);
if (File->FileName == NULL) {
- AsciiStrCat (gCwd, ":\\");
+ AsciiStrCatS (gCwd, AllocLen, ":\\");
} else {
- AsciiStrCat (gCwd, ":");
- AsciiStrCat (gCwd, File->FileName);
+ AsciiStrCatS (gCwd, AllocLen, ":");
+ AsciiStrCatS (gCwd, AllocLen, File->FileName);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 7/9] EmbeddedPkg/EfiFileLib: eliminate deprecated string function calls
2016-10-24 17:41 ` [PATCH 7/9] EmbeddedPkg/EfiFileLib: " Ard Biesheuvel
@ 2016-10-25 14:20 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2016-10-25 14:20 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel, leif.lindholm
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/Library/EfiFileLib/EfiFileLib.c | 42 +++++++++++---------
> 1 file changed, 23 insertions(+), 19 deletions(-)
>
> diff --git a/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c b/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c
> index 4d58c830861c..d3b65aa5a3e0 100644
> --- a/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c
> +++ b/EmbeddedPkg/Library/EfiFileLib/EfiFileLib.c
> @@ -384,9 +384,9 @@ EblFileDevicePath (
>
>
> if ( *FileName != 0 ) {
> - AsciiStrToUnicodeStr (FileName, UnicodeFileName);
> + AsciiStrToUnicodeStrS (FileName, UnicodeFileName, MAX_PATHNAME);
Seems okay.
> } else {
> - AsciiStrToUnicodeStr ("\\", UnicodeFileName);
> + AsciiStrToUnicodeStrS ("\\", UnicodeFileName, MAX_PATHNAME);
> }
Ditto.
>
> Size = StrSize (UnicodeFileName);
> @@ -589,7 +589,7 @@ EblFvFileDevicePath (
> &AuthenticationStatus
> );
> if (!EFI_ERROR (Status)) {
> - UnicodeStrToAsciiStr (Section, AsciiSection);
> + UnicodeStrToAsciiStrS (Section, AsciiSection, MAX_PATHNAME);
> if (AsciiStriCmp (FileName, AsciiSection) == 0) {
> FreePool (Section);
> break;
Seems fine.
> @@ -674,6 +674,7 @@ EfiOpen (
> CHAR8 *CwdPlusPathName;
> UINTN Index;
> EFI_SECTION_TYPE ModifiedSectionType;
> + UINTN AsciiLength;
>
> EblUpdateDeviceLists ();
>
> @@ -706,7 +707,8 @@ EfiOpen (
> }
>
> // We could add a current working directory concept
> - CwdPlusPathName = AllocatePool (AsciiStrSize (gCwd) + AsciiStrSize (PathName));
> + AsciiLength = AsciiStrSize (gCwd) + AsciiStrSize (PathName);
> + CwdPlusPathName = AllocatePool (AsciiLength);
> if (CwdPlusPathName == NULL) {
> return NULL;
> }
> @@ -723,14 +725,14 @@ EfiOpen (
> }
> }
> } else {
> - AsciiStrCpy (CwdPlusPathName, gCwd);
> + AsciiStrCpyS (CwdPlusPathName, AsciiLength, gCwd);
okay
> StrLen = AsciiStrLen (gCwd);
> if ((*PathName != '/') && (*PathName != '\\') && (gCwd[StrLen-1] != '/') && (gCwd[StrLen-1] != '\\')) {
> - AsciiStrCat (CwdPlusPathName, "\\");
> + AsciiStrCatS (CwdPlusPathName, AsciiLength, "\\");
> }
> }
okay
>
> - AsciiStrCat (CwdPlusPathName, PathName);
> + AsciiStrCatS (CwdPlusPathName, AsciiLength, PathName);
> if (AsciiStrStr (CwdPlusPathName, ":") == NULL) {
> // Extra error check to make sure we don't recurse and blow stack
> return NULL;
Also fine.
(I strongly dislike the trickery in the original allocation, that is,
summing two full sizes (each including the terminating NUL separately),
and then letting one of those NULs account for the pathname separator
'\\' that gets inserted in the middle. It happens to be correct, but
seriously...)
> @@ -745,7 +747,7 @@ EfiOpen (
> }
>
> File->DeviceName = AllocatePool (StrLen);
StrLen was initially assigned like this:
StrLen = AsciiStrSize (PathName);
(Q: what would you call a variable storing the retval of a function
named XxxxSize()? A: XxxxLen, obviously!
Never mind that there is a function named XxxxLen() as well; we'll store
the retval of *that* in XxxxSize. For clarity.)
... Just to be clear, this is criticism for the original code.
> - AsciiStrCpy (File->DeviceName, PathName);
> + AsciiStrCpyS (File->DeviceName, StrLen, PathName);
okay.
> File->DeviceName[FileStart - 1] = '\0';
> File->FileName = &File->DeviceName[FileStart];
> if (File->FileName[0] == '\0') {
> @@ -1611,7 +1613,7 @@ ExpandPath (
> {
> CHAR8 *NewPath;
> CHAR8 *Work, *Start, *End;
> - UINTN StrLen;
> + UINTN StrLen, AllocLen;
> INTN i;
>
> if (Cwd == NULL || Path == NULL) {
> @@ -1625,11 +1627,12 @@ ExpandPath (
> }
>
> StrLen = AsciiStrSize (Path);
> - NewPath = AllocatePool (AsciiStrSize (Cwd) + StrLen + 1);
> + AllocLen = AsciiStrSize (Cwd) + StrLen + 1;
> + NewPath = AllocatePool (AllocLen);
> if (NewPath == NULL) {
> return NULL;
> }
> - AsciiStrCpy (NewPath, Cwd);
> + AsciiStrCpyS (NewPath, AllocLen, Cwd);
>
> End = Path + StrLen;
> for (Start = Path ;;) {
> @@ -1640,7 +1643,7 @@ ExpandPath (
> }
>
> // append path prior to ..
> - AsciiStrnCat (NewPath, Start, Work - Start);
> + AsciiStrnCatS (NewPath, AllocLen, Start, Work - Start);
> StrLen = AsciiStrLen (NewPath);
> for (i = StrLen; i >= 0; i--) {
> if (NewPath[i] == ':') {
> @@ -1663,7 +1666,7 @@ ExpandPath (
> }
>
> // Handle the path that remains after the ..
> - AsciiStrnCat (NewPath, Start, End - Start);
> + AsciiStrnCatS (NewPath, AllocLen, Start, End - Start);
>
> return NewPath;
> }
looks good
> @@ -1686,7 +1689,7 @@ EfiSetCwd (
> )
> {
> EFI_OPEN_FILE *File;
> - UINTN Len;
> + UINTN Len, AllocLen;
> CHAR8 *Path;
>
> if (Cwd == NULL) {
> @@ -1729,17 +1732,18 @@ EfiSetCwd (
>
> // Use the info returned from EfiOpen as it can add in CWD if needed. So Cwd could be
> // relative to the current gCwd or not.
> - gCwd = AllocatePool (AsciiStrSize (File->DeviceName) + AsciiStrSize (File->FileName) + 10);
> + AllocLen = AsciiStrSize (File->DeviceName) + AsciiStrSize (File->FileName) + 10;
> + gCwd = AllocatePool (AllocLen);
> if (gCwd == NULL) {
> return EFI_INVALID_PARAMETER;
> }
>
> - AsciiStrCpy (gCwd, File->DeviceName);
> + AsciiStrCpyS (gCwd, AllocLen, File->DeviceName);
> if (File->FileName == NULL) {
> - AsciiStrCat (gCwd, ":\\");
> + AsciiStrCatS (gCwd, AllocLen, ":\\");
> } else {
> - AsciiStrCat (gCwd, ":");
> - AsciiStrCat (gCwd, File->FileName);
> + AsciiStrCatS (gCwd, AllocLen, ":");
> + AsciiStrCatS (gCwd, AllocLen, File->FileName);
> }
>
>
>
Looks correct.
(The constant 10 in the addition, for the allocation, represents how
carefully the original code was written.)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
I need some brain bleach now.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 8/9] EmbeddedPkg/MmcDxe: eliminate deprecated string function calls
2016-10-24 17:41 [PATCH 0/9] EmbeddedPkg: eliminate calls to deprecated functions Ard Biesheuvel
` (6 preceding siblings ...)
2016-10-24 17:41 ` [PATCH 7/9] EmbeddedPkg/EfiFileLib: " Ard Biesheuvel
@ 2016-10-24 17:41 ` Ard Biesheuvel
2016-10-25 13:49 ` Laszlo Ersek
2016-10-24 17:41 ` [PATCH 9/9] EmbeddedPkg: enable -DDISABLE_NEW_DEPRECATED_INTERFACES Ard Biesheuvel
8 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 17:41 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
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 {
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 8/9] EmbeddedPkg/MmcDxe: eliminate deprecated string function calls
2016-10-24 17:41 ` [PATCH 8/9] EmbeddedPkg/MmcDxe: " Ard Biesheuvel
@ 2016-10-25 13:49 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2016-10-25 13:49 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel, leif.lindholm
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 9/9] EmbeddedPkg: enable -DDISABLE_NEW_DEPRECATED_INTERFACES
2016-10-24 17:41 [PATCH 0/9] EmbeddedPkg: eliminate calls to deprecated functions Ard Biesheuvel
` (7 preceding siblings ...)
2016-10-24 17:41 ` [PATCH 8/9] EmbeddedPkg/MmcDxe: " Ard Biesheuvel
@ 2016-10-24 17:41 ` Ard Biesheuvel
8 siblings, 0 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 17:41 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
Define DISABLE_NEW_DEPRECATED_INTERFACES on the compiler command line by
default, to prevent deprecated interfaces from being used in core EDK2
code.
Bug: https://bugzilla.tianocore.org/show_bug.cgi?id=164
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
EmbeddedPkg/EmbeddedPkg.dsc | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/EmbeddedPkg/EmbeddedPkg.dsc b/EmbeddedPkg/EmbeddedPkg.dsc
index cc95564f31af..eb7af800f0b2 100644
--- a/EmbeddedPkg/EmbeddedPkg.dsc
+++ b/EmbeddedPkg/EmbeddedPkg.dsc
@@ -4,6 +4,7 @@
#
# Copyright (c) 2007 - 2015, Intel Corporation. All rights reserved.<BR>
# Copyright (c) 2012-2015, ARM Ltd. All rights reserved.<BR>
+# Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
#
# This program and the accompanying materials
# are licensed and made available under the terms and conditions of the BSD License
@@ -227,7 +228,7 @@ [PcdsFixedAtBuild.IPF]
[BuildOptions]
RVCT:*_*_ARM_PLATFORM_FLAGS == --cpu=7-A.security
-
+ *_*_*_CC_FLAGS = -DDISABLE_NEW_DEPRECATED_INTERFACES
################################################################################
#
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread