* [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath()
2018-07-18 20:50 [PATCH 0/6] UefiLib: centralize OpenFileByDevicePath() and fix its bugs Laszlo Ersek
@ 2018-07-18 20:50 ` Laszlo Ersek
2018-07-18 23:10 ` Yao, Jiewen
` (3 more replies)
2018-07-18 20:50 ` [PATCH 2/6] MdeModulePkg/RamDiskDxe: replace OpenFileByDevicePath() with UefiLib API Laszlo Ersek
` (5 subsequent siblings)
6 siblings, 4 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-07-18 20:50 UTC (permalink / raw)
To: edk2-devel-01
Cc: Chao Zhang, Eric Dong, Jaben Carsey, Jiaxin Wu, Jiewen Yao,
Liming Gao, Michael D Kinney, Roman Bacik, Ruiyu Ni, Siyuan Fu,
Star Zeng
The EfiOpenFileByDevicePath() function centralizes functionality from
- MdeModulePkg/Universal/Disk/RamDiskDxe
- NetworkPkg/TlsAuthConfigDxe
- SecurityPkg/VariableAuthenticated/SecureBootConfigDxe
- ShellPkg/Library/UefiShellLib
unifying the implementation and fixing various bugs.
Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jaben Carsey <jaben.carsey@intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Roman Bacik <roman.bacik@broadcom.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
MdePkg/Library/UefiLib/UefiLib.inf | 1 +
MdePkg/Include/Library/UefiLib.h | 86 ++++++++
MdePkg/Library/UefiLib/UefiLib.c | 226 ++++++++++++++++++++
3 files changed, 313 insertions(+)
diff --git a/MdePkg/Library/UefiLib/UefiLib.inf b/MdePkg/Library/UefiLib/UefiLib.inf
index f69f0a43b576..a6c739ef3d6d 100644
--- a/MdePkg/Library/UefiLib/UefiLib.inf
+++ b/MdePkg/Library/UefiLib/UefiLib.inf
@@ -68,6 +68,7 @@ [Protocols]
gEfiSimpleTextOutProtocolGuid ## SOMETIMES_CONSUMES
gEfiGraphicsOutputProtocolGuid ## SOMETIMES_CONSUMES
gEfiHiiFontProtocolGuid ## SOMETIMES_CONSUMES
+ gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES
gEfiUgaDrawProtocolGuid | gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport ## SOMETIMES_CONSUMES # Consumes if gEfiGraphicsOutputProtocolGuid uninstalled
gEfiComponentNameProtocolGuid | NOT gEfiMdePkgTokenSpaceGuid.PcdComponentNameDisable ## SOMETIMES_PRODUCES # User chooses to produce it
gEfiComponentName2ProtocolGuid | NOT gEfiMdePkgTokenSpaceGuid.PcdComponentName2Disable ## SOMETIMES_PRODUCES # User chooses to produce it
diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
index 7c6fde620c74..2468bf2aee80 100644
--- a/MdePkg/Include/Library/UefiLib.h
+++ b/MdePkg/Include/Library/UefiLib.h
@@ -33,6 +33,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include <Protocol/DriverDiagnostics.h>
#include <Protocol/DriverDiagnostics2.h>
#include <Protocol/GraphicsOutput.h>
+#include <Protocol/DevicePath.h>
+#include <Protocol/SimpleFileSystem.h>
#include <Library/BaseLib.h>
@@ -1520,4 +1522,88 @@ EfiLocateProtocolBuffer (
OUT UINTN *NoProtocols,
OUT VOID ***Buffer
);
+
+/**
+ Open or create a file or directory, possibly creating the chain of
+ directories leading up to the directory.
+
+ EfiOpenFileByDevicePath() first locates EFI_SIMPLE_FILE_SYSTEM_PROTOCOL on
+ FilePath, and opens the root directory of that filesystem with
+ EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume().
+
+ On the remaining device path, the longest initial sequence of
+ FILEPATH_DEVICE_PATH nodes is node-wise traversed with
+ EFI_FILE_PROTOCOL.Open(). For the pathname fragment specified by each
+ traversed FILEPATH_DEVICE_PATH node, EfiOpenFileByDevicePath() first masks
+ EFI_FILE_MODE_CREATE out of OpenMode, and passes 0 for Attributes. If
+ EFI_FILE_PROTOCOL.Open() fails, and OpenMode includes EFI_FILE_MODE_CREATE,
+ then the operation is retried with the caller's OpenMode and Attributes
+ unmodified.
+
+ (As a consequence, if OpenMode includes EFI_FILE_MODE_CREATE, and Attributes
+ includes EFI_FILE_DIRECTORY, and each FILEPATH_DEVICE_PATH specifies a single
+ pathname component, then EfiOpenFileByDevicePath() ensures that the specified
+ series of subdirectories exist on return.)
+
+ The EFI_FILE_PROTOCOL identified by the last FILEPATH_DEVICE_PATH node is
+ output to the caller; intermediate EFI_FILE_PROTOCOL instances are closed. If
+ there are no FILEPATH_DEVICE_PATH nodes past the node that identifies the
+ filesystem, then the EFI_FILE_PROTOCOL of the root directory of the
+ filesystem is output to the caller. If a device path node that is different
+ from FILEPATH_DEVICE_PATH is encountered relative to the filesystem, the
+ traversal is stopped with an error, and a NULL EFI_FILE_PROTOCOL is output.
+
+ @param[in,out] FilePath On input, the device path to the file or directory
+ to open or create. On output, FilePath points one
+ past the last node in the original device path that
+ has been successfully processed. FilePath is set on
+ output even if EfiOpenFileByDevicePath() returns an
+ error.
+
+ @param[out] File On error, File is set to NULL. On success, File is
+ set to the EFI_FILE_PROTOCOL of the root directory
+ of the filesystem, if there are no
+ FILEPATH_DEVICE_PATH nodes in FilePath; otherwise,
+ File is set to the EFI_FILE_PROTOCOL identified by
+ the last node in FilePath.
+
+ @param[in] OpenMode The OpenMode parameter to pass to
+ EFI_FILE_PROTOCOL.Open(). For each
+ FILEPATH_DEVICE_PATH node in FilePath,
+ EfiOpenFileByDevicePath() first opens the specified
+ pathname fragment with EFI_FILE_MODE_CREATE masked
+ out of OpenMode and with Attributes set to 0, and
+ only retries the operation with EFI_FILE_MODE_CREATE
+ unmasked and Attributes propagated if the first open
+ attempt fails.
+
+ @param[in] Attributes The Attributes parameter to pass to
+ EFI_FILE_PROTOCOL.Open(), when EFI_FILE_MODE_CREATE
+ is propagated unmasked in OpenMode.
+
+ @retval EFI_SUCCESS The file or directory has been opened or
+ created.
+
+ @retval EFI_INVALID_PARAMETER FilePath is NULL; or File is NULL; or FilePath
+ contains a device path node, past the node
+ that identifies
+ EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, that is not a
+ FILEPATH_DEVICE_PATH node.
+
+ @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
+
+ @return Error codes propagated from the
+ LocateDevicePath() and OpenProtocol() boot
+ services, and from the
+ EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume()
+ and EFI_FILE_PROTOCOL.Open() member functions.
+**/
+EFI_STATUS
+EFIAPI
+EfiOpenFileByDevicePath (
+ IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
+ OUT EFI_FILE_PROTOCOL **File,
+ IN UINT64 OpenMode,
+ IN UINT64 Attributes
+ );
#endif
diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
index 828a54ce7a97..d3e290178cd9 100644
--- a/MdePkg/Library/UefiLib/UefiLib.c
+++ b/MdePkg/Library/UefiLib/UefiLib.c
@@ -1719,3 +1719,229 @@ EfiLocateProtocolBuffer (
return EFI_SUCCESS;
}
+
+/**
+ Open or create a file or directory, possibly creating the chain of
+ directories leading up to the directory.
+
+ EfiOpenFileByDevicePath() first locates EFI_SIMPLE_FILE_SYSTEM_PROTOCOL on
+ FilePath, and opens the root directory of that filesystem with
+ EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume().
+
+ On the remaining device path, the longest initial sequence of
+ FILEPATH_DEVICE_PATH nodes is node-wise traversed with
+ EFI_FILE_PROTOCOL.Open(). For the pathname fragment specified by each
+ traversed FILEPATH_DEVICE_PATH node, EfiOpenFileByDevicePath() first masks
+ EFI_FILE_MODE_CREATE out of OpenMode, and passes 0 for Attributes. If
+ EFI_FILE_PROTOCOL.Open() fails, and OpenMode includes EFI_FILE_MODE_CREATE,
+ then the operation is retried with the caller's OpenMode and Attributes
+ unmodified.
+
+ (As a consequence, if OpenMode includes EFI_FILE_MODE_CREATE, and Attributes
+ includes EFI_FILE_DIRECTORY, and each FILEPATH_DEVICE_PATH specifies a single
+ pathname component, then EfiOpenFileByDevicePath() ensures that the specified
+ series of subdirectories exist on return.)
+
+ The EFI_FILE_PROTOCOL identified by the last FILEPATH_DEVICE_PATH node is
+ output to the caller; intermediate EFI_FILE_PROTOCOL instances are closed. If
+ there are no FILEPATH_DEVICE_PATH nodes past the node that identifies the
+ filesystem, then the EFI_FILE_PROTOCOL of the root directory of the
+ filesystem is output to the caller. If a device path node that is different
+ from FILEPATH_DEVICE_PATH is encountered relative to the filesystem, the
+ traversal is stopped with an error, and a NULL EFI_FILE_PROTOCOL is output.
+
+ @param[in,out] FilePath On input, the device path to the file or directory
+ to open or create. On output, FilePath points one
+ past the last node in the original device path that
+ has been successfully processed. FilePath is set on
+ output even if EfiOpenFileByDevicePath() returns an
+ error.
+
+ @param[out] File On error, File is set to NULL. On success, File is
+ set to the EFI_FILE_PROTOCOL of the root directory
+ of the filesystem, if there are no
+ FILEPATH_DEVICE_PATH nodes in FilePath; otherwise,
+ File is set to the EFI_FILE_PROTOCOL identified by
+ the last node in FilePath.
+
+ @param[in] OpenMode The OpenMode parameter to pass to
+ EFI_FILE_PROTOCOL.Open(). For each
+ FILEPATH_DEVICE_PATH node in FilePath,
+ EfiOpenFileByDevicePath() first opens the specified
+ pathname fragment with EFI_FILE_MODE_CREATE masked
+ out of OpenMode and with Attributes set to 0, and
+ only retries the operation with EFI_FILE_MODE_CREATE
+ unmasked and Attributes propagated if the first open
+ attempt fails.
+
+ @param[in] Attributes The Attributes parameter to pass to
+ EFI_FILE_PROTOCOL.Open(), when EFI_FILE_MODE_CREATE
+ is propagated unmasked in OpenMode.
+
+ @retval EFI_SUCCESS The file or directory has been opened or
+ created.
+
+ @retval EFI_INVALID_PARAMETER FilePath is NULL; or File is NULL; or FilePath
+ contains a device path node, past the node
+ that identifies
+ EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, that is not a
+ FILEPATH_DEVICE_PATH node.
+
+ @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
+
+ @return Error codes propagated from the
+ LocateDevicePath() and OpenProtocol() boot
+ services, and from the
+ EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume()
+ and EFI_FILE_PROTOCOL.Open() member functions.
+**/
+EFI_STATUS
+EFIAPI
+EfiOpenFileByDevicePath (
+ IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
+ OUT EFI_FILE_PROTOCOL **File,
+ IN UINT64 OpenMode,
+ IN UINT64 Attributes
+ )
+{
+ EFI_STATUS Status;
+ EFI_HANDLE FileSystemHandle;
+ EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *FileSystem;
+ EFI_FILE_PROTOCOL *LastFile;
+
+ if (File == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+ *File = NULL;
+
+ if (FilePath == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ //
+ // Look up the filesystem.
+ //
+ Status = gBS->LocateDevicePath (
+ &gEfiSimpleFileSystemProtocolGuid,
+ FilePath,
+ &FileSystemHandle
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ Status = gBS->OpenProtocol (
+ FileSystemHandle,
+ &gEfiSimpleFileSystemProtocolGuid,
+ (VOID **)&FileSystem,
+ gImageHandle,
+ NULL,
+ EFI_OPEN_PROTOCOL_GET_PROTOCOL
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // Open the root directory of the filesystem. After this operation succeeds,
+ // we have to release LastFile on error.
+ //
+ Status = FileSystem->OpenVolume (FileSystem, &LastFile);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // Traverse the device path nodes relative to the filesystem.
+ //
+ while (!IsDevicePathEnd (*FilePath)) {
+ //
+ // Keep local variables that relate to the current device path node tightly
+ // scoped.
+ //
+ FILEPATH_DEVICE_PATH *FilePathNode;
+ CHAR16 *AlignedPathName;
+ CHAR16 *PathName;
+ EFI_FILE_PROTOCOL *NextFile;
+
+ if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
+ DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP) {
+ Status = EFI_INVALID_PARAMETER;
+ goto CloseLastFile;
+ }
+ FilePathNode = (FILEPATH_DEVICE_PATH *)*FilePath;
+
+ //
+ // FilePathNode->PathName may be unaligned, and the UEFI specification
+ // requires pointers that are passed to protocol member functions to be
+ // aligned. Create an aligned copy of the pathname if necessary.
+ //
+ if ((UINTN)FilePathNode->PathName % sizeof *FilePathNode->PathName == 0) {
+ AlignedPathName = NULL;
+ PathName = FilePathNode->PathName;
+ } else {
+ AlignedPathName = AllocateCopyPool (
+ (DevicePathNodeLength (FilePathNode) -
+ SIZE_OF_FILEPATH_DEVICE_PATH),
+ FilePathNode->PathName
+ );
+ if (AlignedPathName == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto CloseLastFile;
+ }
+ PathName = AlignedPathName;
+ }
+
+ //
+ // Open the next pathname fragment with EFI_FILE_MODE_CREATE masked out and
+ // with Attributes set to 0.
+ //
+ Status = LastFile->Open (
+ LastFile,
+ &NextFile,
+ PathName,
+ OpenMode & ~(UINT64)EFI_FILE_MODE_CREATE,
+ 0
+ );
+
+ //
+ // Retry with EFI_FILE_MODE_CREATE and the original Attributes if the first
+ // attempt failed, and the caller specified EFI_FILE_MODE_CREATE.
+ //
+ if (EFI_ERROR (Status) && (OpenMode & EFI_FILE_MODE_CREATE) != 0) {
+ Status = LastFile->Open (
+ LastFile,
+ &NextFile,
+ PathName,
+ OpenMode,
+ Attributes
+ );
+ }
+
+ //
+ // Release any AlignedPathName on both error and success paths; PathName is
+ // no longer needed.
+ //
+ if (AlignedPathName != NULL) {
+ FreePool (AlignedPathName);
+ }
+ if (EFI_ERROR (Status)) {
+ goto CloseLastFile;
+ }
+
+ //
+ // Advance to the next device path node.
+ //
+ LastFile->Close (LastFile);
+ LastFile = NextFile;
+ *FilePath = NextDevicePathNode (FilePathNode);
+ }
+
+ *File = LastFile;
+ return EFI_SUCCESS;
+
+CloseLastFile:
+ LastFile->Close (LastFile);
+
+ ASSERT (EFI_ERROR (Status));
+ return Status;
+}
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath()
2018-07-18 20:50 ` [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath() Laszlo Ersek
@ 2018-07-18 23:10 ` Yao, Jiewen
2018-07-19 10:47 ` Laszlo Ersek
2018-07-24 17:20 ` Laszlo Ersek
` (2 subsequent siblings)
3 siblings, 1 reply; 27+ messages in thread
From: Yao, Jiewen @ 2018-07-18 23:10 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel-01, Zhang, Chao B, Dong, Eric, Carsey, Jaben,
Wu, Jiaxin, Gao, Liming, Kinney, Michael D, Roman Bacik,
Ni, Ruiyu, Fu, Siyuan, Zeng, Star, Yao, Jiewen
Thanks Laszlo.
Would you please add one line comment on the FilePath, to describe if the FilePath is internal input or external input? As such the API consumer can know if caller’s responsibility to verify it or callee’s responsibility.
For example, if the caller gets path from a read write variable, and input it directly, the this API need validate before use.
If the caller already does the verification, then this API can use it directly.
Sanity check is just for the format, not the content.
The question I have is: Where should the sanity check be?
thank you!
Yao, Jiewen
> 在 2018年7月19日，上午4:50，Laszlo Ersek <lersek@redhat.com> 写道：
>
> The EfiOpenFileByDevicePath() function centralizes functionality from
>
> - MdeModulePkg/Universal/Disk/RamDiskDxe
> - NetworkPkg/TlsAuthConfigDxe
> - SecurityPkg/VariableAuthenticated/SecureBootConfigDxe
> - ShellPkg/Library/UefiShellLib
>
> unifying the implementation and fixing various bugs.
>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Roman Bacik <roman.bacik@broadcom.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> MdePkg/Library/UefiLib/UefiLib.inf | 1 +
> MdePkg/Include/Library/UefiLib.h | 86 ++++++++
> MdePkg/Library/UefiLib/UefiLib.c | 226 ++++++++++++++++++++
> 3 files changed, 313 insertions(+)
>
> diff --git a/MdePkg/Library/UefiLib/UefiLib.inf b/MdePkg/Library/UefiLib/UefiLib.inf
> index f69f0a43b576..a6c739ef3d6d 100644
> --- a/MdePkg/Library/UefiLib/UefiLib.inf
> +++ b/MdePkg/Library/UefiLib/UefiLib.inf
> @@ -68,6 +68,7 @@ [Protocols]
> gEfiSimpleTextOutProtocolGuid ## SOMETIMES_CONSUMES
> gEfiGraphicsOutputProtocolGuid ## SOMETIMES_CONSUMES
> gEfiHiiFontProtocolGuid ## SOMETIMES_CONSUMES
> + gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES
> gEfiUgaDrawProtocolGuid | gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport ## SOMETIMES_CONSUMES # Consumes if gEfiGraphicsOutputProtocolGuid uninstalled
> gEfiComponentNameProtocolGuid | NOT gEfiMdePkgTokenSpaceGuid.PcdComponentNameDisable ## SOMETIMES_PRODUCES # User chooses to produce it
> gEfiComponentName2ProtocolGuid | NOT gEfiMdePkgTokenSpaceGuid.PcdComponentName2Disable ## SOMETIMES_PRODUCES # User chooses to produce it
> diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
> index 7c6fde620c74..2468bf2aee80 100644
> --- a/MdePkg/Include/Library/UefiLib.h
> +++ b/MdePkg/Include/Library/UefiLib.h
> @@ -33,6 +33,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> #include <Protocol/DriverDiagnostics.h>
> #include <Protocol/DriverDiagnostics2.h>
> #include <Protocol/GraphicsOutput.h>
> +#include <Protocol/DevicePath.h>
> +#include <Protocol/SimpleFileSystem.h>
>
> #include <Library/BaseLib.h>
>
> @@ -1520,4 +1522,88 @@ EfiLocateProtocolBuffer (
> OUT UINTN *NoProtocols,
> OUT VOID ***Buffer
> );
> +
> +/**
> + Open or create a file or directory, possibly creating the chain of
> + directories leading up to the directory.
> +
> + EfiOpenFileByDevicePath() first locates EFI_SIMPLE_FILE_SYSTEM_PROTOCOL on
> + FilePath, and opens the root directory of that filesystem with
> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume().
> +
> + On the remaining device path, the longest initial sequence of
> + FILEPATH_DEVICE_PATH nodes is node-wise traversed with
> + EFI_FILE_PROTOCOL.Open(). For the pathname fragment specified by each
> + traversed FILEPATH_DEVICE_PATH node, EfiOpenFileByDevicePath() first masks
> + EFI_FILE_MODE_CREATE out of OpenMode, and passes 0 for Attributes. If
> + EFI_FILE_PROTOCOL.Open() fails, and OpenMode includes EFI_FILE_MODE_CREATE,
> + then the operation is retried with the caller's OpenMode and Attributes
> + unmodified.
> +
> + (As a consequence, if OpenMode includes EFI_FILE_MODE_CREATE, and Attributes
> + includes EFI_FILE_DIRECTORY, and each FILEPATH_DEVICE_PATH specifies a single
> + pathname component, then EfiOpenFileByDevicePath() ensures that the specified
> + series of subdirectories exist on return.)
> +
> + The EFI_FILE_PROTOCOL identified by the last FILEPATH_DEVICE_PATH node is
> + output to the caller; intermediate EFI_FILE_PROTOCOL instances are closed. If
> + there are no FILEPATH_DEVICE_PATH nodes past the node that identifies the
> + filesystem, then the EFI_FILE_PROTOCOL of the root directory of the
> + filesystem is output to the caller. If a device path node that is different
> + from FILEPATH_DEVICE_PATH is encountered relative to the filesystem, the
> + traversal is stopped with an error, and a NULL EFI_FILE_PROTOCOL is output.
> +
> + @param[in,out] FilePath On input, the device path to the file or directory
> + to open or create. On output, FilePath points one
> + past the last node in the original device path that
> + has been successfully processed. FilePath is set on
> + output even if EfiOpenFileByDevicePath() returns an
> + error.
> +
> + @param[out] File On error, File is set to NULL. On success, File is
> + set to the EFI_FILE_PROTOCOL of the root directory
> + of the filesystem, if there are no
> + FILEPATH_DEVICE_PATH nodes in FilePath; otherwise,
> + File is set to the EFI_FILE_PROTOCOL identified by
> + the last node in FilePath.
> +
> + @param[in] OpenMode The OpenMode parameter to pass to
> + EFI_FILE_PROTOCOL.Open(). For each
> + FILEPATH_DEVICE_PATH node in FilePath,
> + EfiOpenFileByDevicePath() first opens the specified
> + pathname fragment with EFI_FILE_MODE_CREATE masked
> + out of OpenMode and with Attributes set to 0, and
> + only retries the operation with EFI_FILE_MODE_CREATE
> + unmasked and Attributes propagated if the first open
> + attempt fails.
> +
> + @param[in] Attributes The Attributes parameter to pass to
> + EFI_FILE_PROTOCOL.Open(), when EFI_FILE_MODE_CREATE
> + is propagated unmasked in OpenMode.
> +
> + @retval EFI_SUCCESS The file or directory has been opened or
> + created.
> +
> + @retval EFI_INVALID_PARAMETER FilePath is NULL; or File is NULL; or FilePath
> + contains a device path node, past the node
> + that identifies
> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, that is not a
> + FILEPATH_DEVICE_PATH node.
> +
> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
> +
> + @return Error codes propagated from the
> + LocateDevicePath() and OpenProtocol() boot
> + services, and from the
> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume()
> + and EFI_FILE_PROTOCOL.Open() member functions.
> +**/
> +EFI_STATUS
> +EFIAPI
> +EfiOpenFileByDevicePath (
> + IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
> + OUT EFI_FILE_PROTOCOL **File,
> + IN UINT64 OpenMode,
> + IN UINT64 Attributes
> + );
> #endif
> diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
> index 828a54ce7a97..d3e290178cd9 100644
> --- a/MdePkg/Library/UefiLib/UefiLib.c
> +++ b/MdePkg/Library/UefiLib/UefiLib.c
> @@ -1719,3 +1719,229 @@ EfiLocateProtocolBuffer (
>
> return EFI_SUCCESS;
> }
> +
> +/**
> + Open or create a file or directory, possibly creating the chain of
> + directories leading up to the directory.
> +
> + EfiOpenFileByDevicePath() first locates EFI_SIMPLE_FILE_SYSTEM_PROTOCOL on
> + FilePath, and opens the root directory of that filesystem with
> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume().
> +
> + On the remaining device path, the longest initial sequence of
> + FILEPATH_DEVICE_PATH nodes is node-wise traversed with
> + EFI_FILE_PROTOCOL.Open(). For the pathname fragment specified by each
> + traversed FILEPATH_DEVICE_PATH node, EfiOpenFileByDevicePath() first masks
> + EFI_FILE_MODE_CREATE out of OpenMode, and passes 0 for Attributes. If
> + EFI_FILE_PROTOCOL.Open() fails, and OpenMode includes EFI_FILE_MODE_CREATE,
> + then the operation is retried with the caller's OpenMode and Attributes
> + unmodified.
> +
> + (As a consequence, if OpenMode includes EFI_FILE_MODE_CREATE, and Attributes
> + includes EFI_FILE_DIRECTORY, and each FILEPATH_DEVICE_PATH specifies a single
> + pathname component, then EfiOpenFileByDevicePath() ensures that the specified
> + series of subdirectories exist on return.)
> +
> + The EFI_FILE_PROTOCOL identified by the last FILEPATH_DEVICE_PATH node is
> + output to the caller; intermediate EFI_FILE_PROTOCOL instances are closed. If
> + there are no FILEPATH_DEVICE_PATH nodes past the node that identifies the
> + filesystem, then the EFI_FILE_PROTOCOL of the root directory of the
> + filesystem is output to the caller. If a device path node that is different
> + from FILEPATH_DEVICE_PATH is encountered relative to the filesystem, the
> + traversal is stopped with an error, and a NULL EFI_FILE_PROTOCOL is output.
> +
> + @param[in,out] FilePath On input, the device path to the file or directory
> + to open or create. On output, FilePath points one
> + past the last node in the original device path that
> + has been successfully processed. FilePath is set on
> + output even if EfiOpenFileByDevicePath() returns an
> + error.
> +
> + @param[out] File On error, File is set to NULL. On success, File is
> + set to the EFI_FILE_PROTOCOL of the root directory
> + of the filesystem, if there are no
> + FILEPATH_DEVICE_PATH nodes in FilePath; otherwise,
> + File is set to the EFI_FILE_PROTOCOL identified by
> + the last node in FilePath.
> +
> + @param[in] OpenMode The OpenMode parameter to pass to
> + EFI_FILE_PROTOCOL.Open(). For each
> + FILEPATH_DEVICE_PATH node in FilePath,
> + EfiOpenFileByDevicePath() first opens the specified
> + pathname fragment with EFI_FILE_MODE_CREATE masked
> + out of OpenMode and with Attributes set to 0, and
> + only retries the operation with EFI_FILE_MODE_CREATE
> + unmasked and Attributes propagated if the first open
> + attempt fails.
> +
> + @param[in] Attributes The Attributes parameter to pass to
> + EFI_FILE_PROTOCOL.Open(), when EFI_FILE_MODE_CREATE
> + is propagated unmasked in OpenMode.
> +
> + @retval EFI_SUCCESS The file or directory has been opened or
> + created.
> +
> + @retval EFI_INVALID_PARAMETER FilePath is NULL; or File is NULL; or FilePath
> + contains a device path node, past the node
> + that identifies
> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, that is not a
> + FILEPATH_DEVICE_PATH node.
> +
> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
> +
> + @return Error codes propagated from the
> + LocateDevicePath() and OpenProtocol() boot
> + services, and from the
> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume()
> + and EFI_FILE_PROTOCOL.Open() member functions.
> +**/
> +EFI_STATUS
> +EFIAPI
> +EfiOpenFileByDevicePath (
> + IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
> + OUT EFI_FILE_PROTOCOL **File,
> + IN UINT64 OpenMode,
> + IN UINT64 Attributes
> + )
> +{
> + EFI_STATUS Status;
> + EFI_HANDLE FileSystemHandle;
> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *FileSystem;
> + EFI_FILE_PROTOCOL *LastFile;
> +
> + if (File == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> + *File = NULL;
> +
> + if (FilePath == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + //
> + // Look up the filesystem.
> + //
> + Status = gBS->LocateDevicePath (
> + &gEfiSimpleFileSystemProtocolGuid,
> + FilePath,
> + &FileSystemHandle
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + Status = gBS->OpenProtocol (
> + FileSystemHandle,
> + &gEfiSimpleFileSystemProtocolGuid,
> + (VOID **)&FileSystem,
> + gImageHandle,
> + NULL,
> + EFI_OPEN_PROTOCOL_GET_PROTOCOL
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + //
> + // Open the root directory of the filesystem. After this operation succeeds,
> + // we have to release LastFile on error.
> + //
> + Status = FileSystem->OpenVolume (FileSystem, &LastFile);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + //
> + // Traverse the device path nodes relative to the filesystem.
> + //
> + while (!IsDevicePathEnd (*FilePath)) {
> + //
> + // Keep local variables that relate to the current device path node tightly
> + // scoped.
> + //
> + FILEPATH_DEVICE_PATH *FilePathNode;
> + CHAR16 *AlignedPathName;
> + CHAR16 *PathName;
> + EFI_FILE_PROTOCOL *NextFile;
> +
> + if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
> + DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP) {
> + Status = EFI_INVALID_PARAMETER;
> + goto CloseLastFile;
> + }
> + FilePathNode = (FILEPATH_DEVICE_PATH *)*FilePath;
> +
> + //
> + // FilePathNode->PathName may be unaligned, and the UEFI specification
> + // requires pointers that are passed to protocol member functions to be
> + // aligned. Create an aligned copy of the pathname if necessary.
> + //
> + if ((UINTN)FilePathNode->PathName % sizeof *FilePathNode->PathName == 0) {
> + AlignedPathName = NULL;
> + PathName = FilePathNode->PathName;
> + } else {
> + AlignedPathName = AllocateCopyPool (
> + (DevicePathNodeLength (FilePathNode) -
> + SIZE_OF_FILEPATH_DEVICE_PATH),
> + FilePathNode->PathName
> + );
> + if (AlignedPathName == NULL) {
> + Status = EFI_OUT_OF_RESOURCES;
> + goto CloseLastFile;
> + }
> + PathName = AlignedPathName;
> + }
> +
> + //
> + // Open the next pathname fragment with EFI_FILE_MODE_CREATE masked out and
> + // with Attributes set to 0.
> + //
> + Status = LastFile->Open (
> + LastFile,
> + &NextFile,
> + PathName,
> + OpenMode & ~(UINT64)EFI_FILE_MODE_CREATE,
> + 0
> + );
> +
> + //
> + // Retry with EFI_FILE_MODE_CREATE and the original Attributes if the first
> + // attempt failed, and the caller specified EFI_FILE_MODE_CREATE.
> + //
> + if (EFI_ERROR (Status) && (OpenMode & EFI_FILE_MODE_CREATE) != 0) {
> + Status = LastFile->Open (
> + LastFile,
> + &NextFile,
> + PathName,
> + OpenMode,
> + Attributes
> + );
> + }
> +
> + //
> + // Release any AlignedPathName on both error and success paths; PathName is
> + // no longer needed.
> + //
> + if (AlignedPathName != NULL) {
> + FreePool (AlignedPathName);
> + }
> + if (EFI_ERROR (Status)) {
> + goto CloseLastFile;
> + }
> +
> + //
> + // Advance to the next device path node.
> + //
> + LastFile->Close (LastFile);
> + LastFile = NextFile;
> + *FilePath = NextDevicePathNode (FilePathNode);
> + }
> +
> + *File = LastFile;
> + return EFI_SUCCESS;
> +
> +CloseLastFile:
> + LastFile->Close (LastFile);
> +
> + ASSERT (EFI_ERROR (Status));
> + return Status;
> +}
> --
> 2.14.1.3.gb7cf6e02401b
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath()
2018-07-18 23:10 ` Yao, Jiewen
@ 2018-07-19 10:47 ` Laszlo Ersek
2018-07-19 13:03 ` Yao, Jiewen
0 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2018-07-19 10:47 UTC (permalink / raw)
To: Yao, Jiewen
Cc: edk2-devel-01, Zhang, Chao B, Dong, Eric, Carsey, Jaben,
Wu, Jiaxin, Gao, Liming, Kinney, Michael D, Roman Bacik,
Ni, Ruiyu, Fu, Siyuan, Zeng, Star
On 07/19/18 01:10, Yao, Jiewen wrote:
> Thanks Laszlo.
>
> Would you please add one line comment on the FilePath, to describe if the FilePath is internal input or external input? As such the API consumer can know if caller’s responsibility to verify it or callee’s responsibility.
Good point. The caller is responsible for ensuring the well-formedness
of the device path pointed-to by (*FilePath). The function uses
DevicePathLib APIs such as NextDevicePathNode(), and if e.g. some device
path node headers are invalid (containing bogus lengths, for example),
then the function won't work as expected.
I'll add such a comment.
> For example, if the caller gets path from a read write variable, and input it directly, the this API need validate before use.
> If the caller already does the verification, then this API can use it directly.
Right.
(A separate question would be if edk2 offers facilities for verifying
the well-formedness of any given device path -- it looks like a complex
task, to implement everywhere.)
In this series I'd just like to extract the duplicated code to a common
location, and fix its bugs. I didn't try to change the interface
contract (just to document it more precisely). If we'd like to "armor"
this function against maliciously formatted device paths, IMO that
should be done separately.
So I agree that the comment you suggest should be added.
> Sanity check is just for the format, not the content.
> The question I have is: Where should the sanity check be?
At the moment: in the caller.
Thanks!
Laszlo
>
> thank you!
> Yao, Jiewen
>
>
>> 在 2018年7月19日，上午4:50，Laszlo Ersek <lersek@redhat.com> 写道：
>>
>> The EfiOpenFileByDevicePath() function centralizes functionality from
>>
>> - MdeModulePkg/Universal/Disk/RamDiskDxe
>> - NetworkPkg/TlsAuthConfigDxe
>> - SecurityPkg/VariableAuthenticated/SecureBootConfigDxe
>> - ShellPkg/Library/UefiShellLib
>>
>> unifying the implementation and fixing various bugs.
>>
>> Cc: Chao Zhang <chao.b.zhang@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jaben Carsey <jaben.carsey@intel.com>
>> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Roman Bacik <roman.bacik@broadcom.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Siyuan Fu <siyuan.fu@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> MdePkg/Library/UefiLib/UefiLib.inf | 1 +
>> MdePkg/Include/Library/UefiLib.h | 86 ++++++++
>> MdePkg/Library/UefiLib/UefiLib.c | 226 ++++++++++++++++++++
>> 3 files changed, 313 insertions(+)
>>
>> diff --git a/MdePkg/Library/UefiLib/UefiLib.inf b/MdePkg/Library/UefiLib/UefiLib.inf
>> index f69f0a43b576..a6c739ef3d6d 100644
>> --- a/MdePkg/Library/UefiLib/UefiLib.inf
>> +++ b/MdePkg/Library/UefiLib/UefiLib.inf
>> @@ -68,6 +68,7 @@ [Protocols]
>> gEfiSimpleTextOutProtocolGuid ## SOMETIMES_CONSUMES
>> gEfiGraphicsOutputProtocolGuid ## SOMETIMES_CONSUMES
>> gEfiHiiFontProtocolGuid ## SOMETIMES_CONSUMES
>> + gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES
>> gEfiUgaDrawProtocolGuid | gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport ## SOMETIMES_CONSUMES # Consumes if gEfiGraphicsOutputProtocolGuid uninstalled
>> gEfiComponentNameProtocolGuid | NOT gEfiMdePkgTokenSpaceGuid.PcdComponentNameDisable ## SOMETIMES_PRODUCES # User chooses to produce it
>> gEfiComponentName2ProtocolGuid | NOT gEfiMdePkgTokenSpaceGuid.PcdComponentName2Disable ## SOMETIMES_PRODUCES # User chooses to produce it
>> diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
>> index 7c6fde620c74..2468bf2aee80 100644
>> --- a/MdePkg/Include/Library/UefiLib.h
>> +++ b/MdePkg/Include/Library/UefiLib.h
>> @@ -33,6 +33,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> #include <Protocol/DriverDiagnostics.h>
>> #include <Protocol/DriverDiagnostics2.h>
>> #include <Protocol/GraphicsOutput.h>
>> +#include <Protocol/DevicePath.h>
>> +#include <Protocol/SimpleFileSystem.h>
>>
>> #include <Library/BaseLib.h>
>>
>> @@ -1520,4 +1522,88 @@ EfiLocateProtocolBuffer (
>> OUT UINTN *NoProtocols,
>> OUT VOID ***Buffer
>> );
>> +
>> +/**
>> + Open or create a file or directory, possibly creating the chain of
>> + directories leading up to the directory.
>> +
>> + EfiOpenFileByDevicePath() first locates EFI_SIMPLE_FILE_SYSTEM_PROTOCOL on
>> + FilePath, and opens the root directory of that filesystem with
>> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume().
>> +
>> + On the remaining device path, the longest initial sequence of
>> + FILEPATH_DEVICE_PATH nodes is node-wise traversed with
>> + EFI_FILE_PROTOCOL.Open(). For the pathname fragment specified by each
>> + traversed FILEPATH_DEVICE_PATH node, EfiOpenFileByDevicePath() first masks
>> + EFI_FILE_MODE_CREATE out of OpenMode, and passes 0 for Attributes. If
>> + EFI_FILE_PROTOCOL.Open() fails, and OpenMode includes EFI_FILE_MODE_CREATE,
>> + then the operation is retried with the caller's OpenMode and Attributes
>> + unmodified.
>> +
>> + (As a consequence, if OpenMode includes EFI_FILE_MODE_CREATE, and Attributes
>> + includes EFI_FILE_DIRECTORY, and each FILEPATH_DEVICE_PATH specifies a single
>> + pathname component, then EfiOpenFileByDevicePath() ensures that the specified
>> + series of subdirectories exist on return.)
>> +
>> + The EFI_FILE_PROTOCOL identified by the last FILEPATH_DEVICE_PATH node is
>> + output to the caller; intermediate EFI_FILE_PROTOCOL instances are closed. If
>> + there are no FILEPATH_DEVICE_PATH nodes past the node that identifies the
>> + filesystem, then the EFI_FILE_PROTOCOL of the root directory of the
>> + filesystem is output to the caller. If a device path node that is different
>> + from FILEPATH_DEVICE_PATH is encountered relative to the filesystem, the
>> + traversal is stopped with an error, and a NULL EFI_FILE_PROTOCOL is output.
>> +
>> + @param[in,out] FilePath On input, the device path to the file or directory
>> + to open or create. On output, FilePath points one
>> + past the last node in the original device path that
>> + has been successfully processed. FilePath is set on
>> + output even if EfiOpenFileByDevicePath() returns an
>> + error.
>> +
>> + @param[out] File On error, File is set to NULL. On success, File is
>> + set to the EFI_FILE_PROTOCOL of the root directory
>> + of the filesystem, if there are no
>> + FILEPATH_DEVICE_PATH nodes in FilePath; otherwise,
>> + File is set to the EFI_FILE_PROTOCOL identified by
>> + the last node in FilePath.
>> +
>> + @param[in] OpenMode The OpenMode parameter to pass to
>> + EFI_FILE_PROTOCOL.Open(). For each
>> + FILEPATH_DEVICE_PATH node in FilePath,
>> + EfiOpenFileByDevicePath() first opens the specified
>> + pathname fragment with EFI_FILE_MODE_CREATE masked
>> + out of OpenMode and with Attributes set to 0, and
>> + only retries the operation with EFI_FILE_MODE_CREATE
>> + unmasked and Attributes propagated if the first open
>> + attempt fails.
>> +
>> + @param[in] Attributes The Attributes parameter to pass to
>> + EFI_FILE_PROTOCOL.Open(), when EFI_FILE_MODE_CREATE
>> + is propagated unmasked in OpenMode.
>> +
>> + @retval EFI_SUCCESS The file or directory has been opened or
>> + created.
>> +
>> + @retval EFI_INVALID_PARAMETER FilePath is NULL; or File is NULL; or FilePath
>> + contains a device path node, past the node
>> + that identifies
>> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, that is not a
>> + FILEPATH_DEVICE_PATH node.
>> +
>> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
>> +
>> + @return Error codes propagated from the
>> + LocateDevicePath() and OpenProtocol() boot
>> + services, and from the
>> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume()
>> + and EFI_FILE_PROTOCOL.Open() member functions.
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +EfiOpenFileByDevicePath (
>> + IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
>> + OUT EFI_FILE_PROTOCOL **File,
>> + IN UINT64 OpenMode,
>> + IN UINT64 Attributes
>> + );
>> #endif
>> diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
>> index 828a54ce7a97..d3e290178cd9 100644
>> --- a/MdePkg/Library/UefiLib/UefiLib.c
>> +++ b/MdePkg/Library/UefiLib/UefiLib.c
>> @@ -1719,3 +1719,229 @@ EfiLocateProtocolBuffer (
>>
>> return EFI_SUCCESS;
>> }
>> +
>> +/**
>> + Open or create a file or directory, possibly creating the chain of
>> + directories leading up to the directory.
>> +
>> + EfiOpenFileByDevicePath() first locates EFI_SIMPLE_FILE_SYSTEM_PROTOCOL on
>> + FilePath, and opens the root directory of that filesystem with
>> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume().
>> +
>> + On the remaining device path, the longest initial sequence of
>> + FILEPATH_DEVICE_PATH nodes is node-wise traversed with
>> + EFI_FILE_PROTOCOL.Open(). For the pathname fragment specified by each
>> + traversed FILEPATH_DEVICE_PATH node, EfiOpenFileByDevicePath() first masks
>> + EFI_FILE_MODE_CREATE out of OpenMode, and passes 0 for Attributes. If
>> + EFI_FILE_PROTOCOL.Open() fails, and OpenMode includes EFI_FILE_MODE_CREATE,
>> + then the operation is retried with the caller's OpenMode and Attributes
>> + unmodified.
>> +
>> + (As a consequence, if OpenMode includes EFI_FILE_MODE_CREATE, and Attributes
>> + includes EFI_FILE_DIRECTORY, and each FILEPATH_DEVICE_PATH specifies a single
>> + pathname component, then EfiOpenFileByDevicePath() ensures that the specified
>> + series of subdirectories exist on return.)
>> +
>> + The EFI_FILE_PROTOCOL identified by the last FILEPATH_DEVICE_PATH node is
>> + output to the caller; intermediate EFI_FILE_PROTOCOL instances are closed. If
>> + there are no FILEPATH_DEVICE_PATH nodes past the node that identifies the
>> + filesystem, then the EFI_FILE_PROTOCOL of the root directory of the
>> + filesystem is output to the caller. If a device path node that is different
>> + from FILEPATH_DEVICE_PATH is encountered relative to the filesystem, the
>> + traversal is stopped with an error, and a NULL EFI_FILE_PROTOCOL is output.
>> +
>> + @param[in,out] FilePath On input, the device path to the file or directory
>> + to open or create. On output, FilePath points one
>> + past the last node in the original device path that
>> + has been successfully processed. FilePath is set on
>> + output even if EfiOpenFileByDevicePath() returns an
>> + error.
>> +
>> + @param[out] File On error, File is set to NULL. On success, File is
>> + set to the EFI_FILE_PROTOCOL of the root directory
>> + of the filesystem, if there are no
>> + FILEPATH_DEVICE_PATH nodes in FilePath; otherwise,
>> + File is set to the EFI_FILE_PROTOCOL identified by
>> + the last node in FilePath.
>> +
>> + @param[in] OpenMode The OpenMode parameter to pass to
>> + EFI_FILE_PROTOCOL.Open(). For each
>> + FILEPATH_DEVICE_PATH node in FilePath,
>> + EfiOpenFileByDevicePath() first opens the specified
>> + pathname fragment with EFI_FILE_MODE_CREATE masked
>> + out of OpenMode and with Attributes set to 0, and
>> + only retries the operation with EFI_FILE_MODE_CREATE
>> + unmasked and Attributes propagated if the first open
>> + attempt fails.
>> +
>> + @param[in] Attributes The Attributes parameter to pass to
>> + EFI_FILE_PROTOCOL.Open(), when EFI_FILE_MODE_CREATE
>> + is propagated unmasked in OpenMode.
>> +
>> + @retval EFI_SUCCESS The file or directory has been opened or
>> + created.
>> +
>> + @retval EFI_INVALID_PARAMETER FilePath is NULL; or File is NULL; or FilePath
>> + contains a device path node, past the node
>> + that identifies
>> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, that is not a
>> + FILEPATH_DEVICE_PATH node.
>> +
>> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
>> +
>> + @return Error codes propagated from the
>> + LocateDevicePath() and OpenProtocol() boot
>> + services, and from the
>> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume()
>> + and EFI_FILE_PROTOCOL.Open() member functions.
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +EfiOpenFileByDevicePath (
>> + IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
>> + OUT EFI_FILE_PROTOCOL **File,
>> + IN UINT64 OpenMode,
>> + IN UINT64 Attributes
>> + )
>> +{
>> + EFI_STATUS Status;
>> + EFI_HANDLE FileSystemHandle;
>> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *FileSystem;
>> + EFI_FILE_PROTOCOL *LastFile;
>> +
>> + if (File == NULL) {
>> + return EFI_INVALID_PARAMETER;
>> + }
>> + *File = NULL;
>> +
>> + if (FilePath == NULL) {
>> + return EFI_INVALID_PARAMETER;
>> + }
>> +
>> + //
>> + // Look up the filesystem.
>> + //
>> + Status = gBS->LocateDevicePath (
>> + &gEfiSimpleFileSystemProtocolGuid,
>> + FilePath,
>> + &FileSystemHandle
>> + );
>> + if (EFI_ERROR (Status)) {
>> + return Status;
>> + }
>> + Status = gBS->OpenProtocol (
>> + FileSystemHandle,
>> + &gEfiSimpleFileSystemProtocolGuid,
>> + (VOID **)&FileSystem,
>> + gImageHandle,
>> + NULL,
>> + EFI_OPEN_PROTOCOL_GET_PROTOCOL
>> + );
>> + if (EFI_ERROR (Status)) {
>> + return Status;
>> + }
>> +
>> + //
>> + // Open the root directory of the filesystem. After this operation succeeds,
>> + // we have to release LastFile on error.
>> + //
>> + Status = FileSystem->OpenVolume (FileSystem, &LastFile);
>> + if (EFI_ERROR (Status)) {
>> + return Status;
>> + }
>> +
>> + //
>> + // Traverse the device path nodes relative to the filesystem.
>> + //
>> + while (!IsDevicePathEnd (*FilePath)) {
>> + //
>> + // Keep local variables that relate to the current device path node tightly
>> + // scoped.
>> + //
>> + FILEPATH_DEVICE_PATH *FilePathNode;
>> + CHAR16 *AlignedPathName;
>> + CHAR16 *PathName;
>> + EFI_FILE_PROTOCOL *NextFile;
>> +
>> + if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
>> + DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP) {
>> + Status = EFI_INVALID_PARAMETER;
>> + goto CloseLastFile;
>> + }
>> + FilePathNode = (FILEPATH_DEVICE_PATH *)*FilePath;
>> +
>> + //
>> + // FilePathNode->PathName may be unaligned, and the UEFI specification
>> + // requires pointers that are passed to protocol member functions to be
>> + // aligned. Create an aligned copy of the pathname if necessary.
>> + //
>> + if ((UINTN)FilePathNode->PathName % sizeof *FilePathNode->PathName == 0) {
>> + AlignedPathName = NULL;
>> + PathName = FilePathNode->PathName;
>> + } else {
>> + AlignedPathName = AllocateCopyPool (
>> + (DevicePathNodeLength (FilePathNode) -
>> + SIZE_OF_FILEPATH_DEVICE_PATH),
>> + FilePathNode->PathName
>> + );
>> + if (AlignedPathName == NULL) {
>> + Status = EFI_OUT_OF_RESOURCES;
>> + goto CloseLastFile;
>> + }
>> + PathName = AlignedPathName;
>> + }
>> +
>> + //
>> + // Open the next pathname fragment with EFI_FILE_MODE_CREATE masked out and
>> + // with Attributes set to 0.
>> + //
>> + Status = LastFile->Open (
>> + LastFile,
>> + &NextFile,
>> + PathName,
>> + OpenMode & ~(UINT64)EFI_FILE_MODE_CREATE,
>> + 0
>> + );
>> +
>> + //
>> + // Retry with EFI_FILE_MODE_CREATE and the original Attributes if the first
>> + // attempt failed, and the caller specified EFI_FILE_MODE_CREATE.
>> + //
>> + if (EFI_ERROR (Status) && (OpenMode & EFI_FILE_MODE_CREATE) != 0) {
>> + Status = LastFile->Open (
>> + LastFile,
>> + &NextFile,
>> + PathName,
>> + OpenMode,
>> + Attributes
>> + );
>> + }
>> +
>> + //
>> + // Release any AlignedPathName on both error and success paths; PathName is
>> + // no longer needed.
>> + //
>> + if (AlignedPathName != NULL) {
>> + FreePool (AlignedPathName);
>> + }
>> + if (EFI_ERROR (Status)) {
>> + goto CloseLastFile;
>> + }
>> +
>> + //
>> + // Advance to the next device path node.
>> + //
>> + LastFile->Close (LastFile);
>> + LastFile = NextFile;
>> + *FilePath = NextDevicePathNode (FilePathNode);
>> + }
>> +
>> + *File = LastFile;
>> + return EFI_SUCCESS;
>> +
>> +CloseLastFile:
>> + LastFile->Close (LastFile);
>> +
>> + ASSERT (EFI_ERROR (Status));
>> + return Status;
>> +}
>> --
>> 2.14.1.3.gb7cf6e02401b
>>
>>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath()
2018-07-19 10:47 ` Laszlo Ersek
@ 2018-07-19 13:03 ` Yao, Jiewen
0 siblings, 0 replies; 27+ messages in thread
From: Yao, Jiewen @ 2018-07-19 13:03 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel-01, Zhang, Chao B, Dong, Eric, Carsey, Jaben,
Wu, Jiaxin, Gao, Liming, Kinney, Michael D, Roman Bacik,
Ni, Ruiyu, Fu, Siyuan, Zeng, Star, Yao, Jiewen
Cool. Thanks!
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, July 19, 2018 6:47 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Dong, Eric <eric.dong@intel.com>; Carsey, Jaben
> <jaben.carsey@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Roman Bacik <roman.bacik@broadcom.com>; Ni, Ruiyu <ruiyu.ni@intel.com>;
> Fu, Siyuan <siyuan.fu@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath()
>
> On 07/19/18 01:10, Yao, Jiewen wrote:
> > Thanks Laszlo.
> >
> > Would you please add one line comment on the FilePath, to describe if the
> FilePath is internal input or external input? As such the API consumer can
> know if caller’s responsibility to verify it or callee’s responsibility.
>
> Good point. The caller is responsible for ensuring the well-formedness
> of the device path pointed-to by (*FilePath). The function uses
> DevicePathLib APIs such as NextDevicePathNode(), and if e.g. some device
> path node headers are invalid (containing bogus lengths, for example),
> then the function won't work as expected.
>
> I'll add such a comment.
>
> > For example, if the caller gets path from a read write variable, and input it
> directly, the this API need validate before use.
> > If the caller already does the verification, then this API can use it directly.
>
> Right.
>
> (A separate question would be if edk2 offers facilities for verifying
> the well-formedness of any given device path -- it looks like a complex
> task, to implement everywhere.)
>
> In this series I'd just like to extract the duplicated code to a common
> location, and fix its bugs. I didn't try to change the interface
> contract (just to document it more precisely). If we'd like to "armor"
> this function against maliciously formatted device paths, IMO that
> should be done separately.
>
> So I agree that the comment you suggest should be added.
>
> > Sanity check is just for the format, not the content.
> > The question I have is: Where should the sanity check be?
>
> At the moment: in the caller.
>
> Thanks!
> Laszlo
>
> >
> > thank you!
> > Yao, Jiewen
> >
> >
> >> 在 2018年7月19日，上午4:50，Laszlo Ersek <lersek@redhat.com> 写
> 道：
> >>
> >> The EfiOpenFileByDevicePath() function centralizes functionality from
> >>
> >> - MdeModulePkg/Universal/Disk/RamDiskDxe
> >> - NetworkPkg/TlsAuthConfigDxe
> >> - SecurityPkg/VariableAuthenticated/SecureBootConfigDxe
> >> - ShellPkg/Library/UefiShellLib
> >>
> >> unifying the implementation and fixing various bugs.
> >>
> >> Cc: Chao Zhang <chao.b.zhang@intel.com>
> >> Cc: Eric Dong <eric.dong@intel.com>
> >> Cc: Jaben Carsey <jaben.carsey@intel.com>
> >> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >> Cc: Liming Gao <liming.gao@intel.com>
> >> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >> Cc: Roman Bacik <roman.bacik@broadcom.com>
> >> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >> Cc: Siyuan Fu <siyuan.fu@intel.com>
> >> Cc: Star Zeng <star.zeng@intel.com>
> >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >> ---
> >> MdePkg/Library/UefiLib/UefiLib.inf | 1 +
> >> MdePkg/Include/Library/UefiLib.h | 86 ++++++++
> >> MdePkg/Library/UefiLib/UefiLib.c | 226 ++++++++++++++++++++
> >> 3 files changed, 313 insertions(+)
> >>
> >> diff --git a/MdePkg/Library/UefiLib/UefiLib.inf
> b/MdePkg/Library/UefiLib/UefiLib.inf
> >> index f69f0a43b576..a6c739ef3d6d 100644
> >> --- a/MdePkg/Library/UefiLib/UefiLib.inf
> >> +++ b/MdePkg/Library/UefiLib/UefiLib.inf
> >> @@ -68,6 +68,7 @@ [Protocols]
> >> gEfiSimpleTextOutProtocolGuid ##
> SOMETIMES_CONSUMES
> >> gEfiGraphicsOutputProtocolGuid ##
> SOMETIMES_CONSUMES
> >> gEfiHiiFontProtocolGuid ##
> SOMETIMES_CONSUMES
> >> + gEfiSimpleFileSystemProtocolGuid ##
> SOMETIMES_CONSUMES
> >> gEfiUgaDrawProtocolGuid |
> gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport ##
> SOMETIMES_CONSUMES # Consumes if gEfiGraphicsOutputProtocolGuid
> uninstalled
> >> gEfiComponentNameProtocolGuid | NOT
> gEfiMdePkgTokenSpaceGuid.PcdComponentNameDisable ##
> SOMETIMES_PRODUCES # User chooses to produce it
> >> gEfiComponentName2ProtocolGuid | NOT
> gEfiMdePkgTokenSpaceGuid.PcdComponentName2Disable ##
> SOMETIMES_PRODUCES # User chooses to produce it
> >> diff --git a/MdePkg/Include/Library/UefiLib.h
> b/MdePkg/Include/Library/UefiLib.h
> >> index 7c6fde620c74..2468bf2aee80 100644
> >> --- a/MdePkg/Include/Library/UefiLib.h
> >> +++ b/MdePkg/Include/Library/UefiLib.h
> >> @@ -33,6 +33,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
> >> #include <Protocol/DriverDiagnostics.h>
> >> #include <Protocol/DriverDiagnostics2.h>
> >> #include <Protocol/GraphicsOutput.h>
> >> +#include <Protocol/DevicePath.h>
> >> +#include <Protocol/SimpleFileSystem.h>
> >>
> >> #include <Library/BaseLib.h>
> >>
> >> @@ -1520,4 +1522,88 @@ EfiLocateProtocolBuffer (
> >> OUT UINTN *NoProtocols,
> >> OUT VOID ***Buffer
> >> );
> >> +
> >> +/**
> >> + Open or create a file or directory, possibly creating the chain of
> >> + directories leading up to the directory.
> >> +
> >> + EfiOpenFileByDevicePath() first locates
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL on
> >> + FilePath, and opens the root directory of that filesystem with
> >> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume().
> >> +
> >> + On the remaining device path, the longest initial sequence of
> >> + FILEPATH_DEVICE_PATH nodes is node-wise traversed with
> >> + EFI_FILE_PROTOCOL.Open(). For the pathname fragment specified by
> each
> >> + traversed FILEPATH_DEVICE_PATH node, EfiOpenFileByDevicePath() first
> masks
> >> + EFI_FILE_MODE_CREATE out of OpenMode, and passes 0 for Attributes.
> If
> >> + EFI_FILE_PROTOCOL.Open() fails, and OpenMode includes
> EFI_FILE_MODE_CREATE,
> >> + then the operation is retried with the caller's OpenMode and Attributes
> >> + unmodified.
> >> +
> >> + (As a consequence, if OpenMode includes EFI_FILE_MODE_CREATE, and
> Attributes
> >> + includes EFI_FILE_DIRECTORY, and each FILEPATH_DEVICE_PATH specifies
> a single
> >> + pathname component, then EfiOpenFileByDevicePath() ensures that the
> specified
> >> + series of subdirectories exist on return.)
> >> +
> >> + The EFI_FILE_PROTOCOL identified by the last FILEPATH_DEVICE_PATH
> node is
> >> + output to the caller; intermediate EFI_FILE_PROTOCOL instances are
> closed. If
> >> + there are no FILEPATH_DEVICE_PATH nodes past the node that identifies
> the
> >> + filesystem, then the EFI_FILE_PROTOCOL of the root directory of the
> >> + filesystem is output to the caller. If a device path node that is different
> >> + from FILEPATH_DEVICE_PATH is encountered relative to the filesystem,
> the
> >> + traversal is stopped with an error, and a NULL EFI_FILE_PROTOCOL is
> output.
> >> +
> >> + @param[in,out] FilePath On input, the device path to the file or
> directory
> >> + to open or create. On output, FilePath
> points one
> >> + past the last node in the original device
> path that
> >> + has been successfully processed. FilePath is
> set on
> >> + output even if EfiOpenFileByDevicePath()
> returns an
> >> + error.
> >> +
> >> + @param[out] File On error, File is set to NULL. On success, File
> is
> >> + set to the EFI_FILE_PROTOCOL of the root
> directory
> >> + of the filesystem, if there are no
> >> + FILEPATH_DEVICE_PATH nodes in FilePath;
> otherwise,
> >> + File is set to the EFI_FILE_PROTOCOL
> identified by
> >> + the last node in FilePath.
> >> +
> >> + @param[in] OpenMode The OpenMode parameter to pass to
> >> + EFI_FILE_PROTOCOL.Open(). For each
> >> + FILEPATH_DEVICE_PATH node in FilePath,
> >> + EfiOpenFileByDevicePath() first opens the
> specified
> >> + pathname fragment with
> EFI_FILE_MODE_CREATE masked
> >> + out of OpenMode and with Attributes set
> to 0, and
> >> + only retries the operation with
> EFI_FILE_MODE_CREATE
> >> + unmasked and Attributes propagated if the
> first open
> >> + attempt fails.
> >> +
> >> + @param[in] Attributes The Attributes parameter to pass to
> >> + EFI_FILE_PROTOCOL.Open(), when
> EFI_FILE_MODE_CREATE
> >> + is propagated unmasked in OpenMode.
> >> +
> >> + @retval EFI_SUCCESS The file or directory has been
> opened or
> >> + created.
> >> +
> >> + @retval EFI_INVALID_PARAMETER FilePath is NULL; or File is NULL; or
> FilePath
> >> + contains a device path node, past
> the node
> >> + that identifies
> >> +
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, that is not a
> >> + FILEPATH_DEVICE_PATH node.
> >> +
> >> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
> >> +
> >> + @return Error codes propagated from the
> >> + LocateDevicePath() and
> OpenProtocol() boot
> >> + services, and from the
> >> +
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume()
> >> + and EFI_FILE_PROTOCOL.Open()
> member functions.
> >> +**/
> >> +EFI_STATUS
> >> +EFIAPI
> >> +EfiOpenFileByDevicePath (
> >> + IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
> >> + OUT EFI_FILE_PROTOCOL **File,
> >> + IN UINT64 OpenMode,
> >> + IN UINT64 Attributes
> >> + );
> >> #endif
> >> diff --git a/MdePkg/Library/UefiLib/UefiLib.c
> b/MdePkg/Library/UefiLib/UefiLib.c
> >> index 828a54ce7a97..d3e290178cd9 100644
> >> --- a/MdePkg/Library/UefiLib/UefiLib.c
> >> +++ b/MdePkg/Library/UefiLib/UefiLib.c
> >> @@ -1719,3 +1719,229 @@ EfiLocateProtocolBuffer (
> >>
> >> return EFI_SUCCESS;
> >> }
> >> +
> >> +/**
> >> + Open or create a file or directory, possibly creating the chain of
> >> + directories leading up to the directory.
> >> +
> >> + EfiOpenFileByDevicePath() first locates
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL on
> >> + FilePath, and opens the root directory of that filesystem with
> >> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume().
> >> +
> >> + On the remaining device path, the longest initial sequence of
> >> + FILEPATH_DEVICE_PATH nodes is node-wise traversed with
> >> + EFI_FILE_PROTOCOL.Open(). For the pathname fragment specified by
> each
> >> + traversed FILEPATH_DEVICE_PATH node, EfiOpenFileByDevicePath() first
> masks
> >> + EFI_FILE_MODE_CREATE out of OpenMode, and passes 0 for Attributes.
> If
> >> + EFI_FILE_PROTOCOL.Open() fails, and OpenMode includes
> EFI_FILE_MODE_CREATE,
> >> + then the operation is retried with the caller's OpenMode and Attributes
> >> + unmodified.
> >> +
> >> + (As a consequence, if OpenMode includes EFI_FILE_MODE_CREATE, and
> Attributes
> >> + includes EFI_FILE_DIRECTORY, and each FILEPATH_DEVICE_PATH specifies
> a single
> >> + pathname component, then EfiOpenFileByDevicePath() ensures that the
> specified
> >> + series of subdirectories exist on return.)
> >> +
> >> + The EFI_FILE_PROTOCOL identified by the last FILEPATH_DEVICE_PATH
> node is
> >> + output to the caller; intermediate EFI_FILE_PROTOCOL instances are
> closed. If
> >> + there are no FILEPATH_DEVICE_PATH nodes past the node that identifies
> the
> >> + filesystem, then the EFI_FILE_PROTOCOL of the root directory of the
> >> + filesystem is output to the caller. If a device path node that is different
> >> + from FILEPATH_DEVICE_PATH is encountered relative to the filesystem,
> the
> >> + traversal is stopped with an error, and a NULL EFI_FILE_PROTOCOL is
> output.
> >> +
> >> + @param[in,out] FilePath On input, the device path to the file or
> directory
> >> + to open or create. On output, FilePath
> points one
> >> + past the last node in the original device
> path that
> >> + has been successfully processed. FilePath is
> set on
> >> + output even if EfiOpenFileByDevicePath()
> returns an
> >> + error.
> >> +
> >> + @param[out] File On error, File is set to NULL. On success, File
> is
> >> + set to the EFI_FILE_PROTOCOL of the root
> directory
> >> + of the filesystem, if there are no
> >> + FILEPATH_DEVICE_PATH nodes in FilePath;
> otherwise,
> >> + File is set to the EFI_FILE_PROTOCOL
> identified by
> >> + the last node in FilePath.
> >> +
> >> + @param[in] OpenMode The OpenMode parameter to pass to
> >> + EFI_FILE_PROTOCOL.Open(). For each
> >> + FILEPATH_DEVICE_PATH node in FilePath,
> >> + EfiOpenFileByDevicePath() first opens the
> specified
> >> + pathname fragment with
> EFI_FILE_MODE_CREATE masked
> >> + out of OpenMode and with Attributes set
> to 0, and
> >> + only retries the operation with
> EFI_FILE_MODE_CREATE
> >> + unmasked and Attributes propagated if the
> first open
> >> + attempt fails.
> >> +
> >> + @param[in] Attributes The Attributes parameter to pass to
> >> + EFI_FILE_PROTOCOL.Open(), when
> EFI_FILE_MODE_CREATE
> >> + is propagated unmasked in OpenMode.
> >> +
> >> + @retval EFI_SUCCESS The file or directory has been
> opened or
> >> + created.
> >> +
> >> + @retval EFI_INVALID_PARAMETER FilePath is NULL; or File is NULL; or
> FilePath
> >> + contains a device path node, past
> the node
> >> + that identifies
> >> +
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, that is not a
> >> + FILEPATH_DEVICE_PATH node.
> >> +
> >> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
> >> +
> >> + @return Error codes propagated from the
> >> + LocateDevicePath() and
> OpenProtocol() boot
> >> + services, and from the
> >> +
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume()
> >> + and EFI_FILE_PROTOCOL.Open()
> member functions.
> >> +**/
> >> +EFI_STATUS
> >> +EFIAPI
> >> +EfiOpenFileByDevicePath (
> >> + IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
> >> + OUT EFI_FILE_PROTOCOL **File,
> >> + IN UINT64 OpenMode,
> >> + IN UINT64 Attributes
> >> + )
> >> +{
> >> + EFI_STATUS Status;
> >> + EFI_HANDLE FileSystemHandle;
> >> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *FileSystem;
> >> + EFI_FILE_PROTOCOL *LastFile;
> >> +
> >> + if (File == NULL) {
> >> + return EFI_INVALID_PARAMETER;
> >> + }
> >> + *File = NULL;
> >> +
> >> + if (FilePath == NULL) {
> >> + return EFI_INVALID_PARAMETER;
> >> + }
> >> +
> >> + //
> >> + // Look up the filesystem.
> >> + //
> >> + Status = gBS->LocateDevicePath (
> >> + &gEfiSimpleFileSystemProtocolGuid,
> >> + FilePath,
> >> + &FileSystemHandle
> >> + );
> >> + if (EFI_ERROR (Status)) {
> >> + return Status;
> >> + }
> >> + Status = gBS->OpenProtocol (
> >> + FileSystemHandle,
> >> + &gEfiSimpleFileSystemProtocolGuid,
> >> + (VOID **)&FileSystem,
> >> + gImageHandle,
> >> + NULL,
> >> + EFI_OPEN_PROTOCOL_GET_PROTOCOL
> >> + );
> >> + if (EFI_ERROR (Status)) {
> >> + return Status;
> >> + }
> >> +
> >> + //
> >> + // Open the root directory of the filesystem. After this operation
> succeeds,
> >> + // we have to release LastFile on error.
> >> + //
> >> + Status = FileSystem->OpenVolume (FileSystem, &LastFile);
> >> + if (EFI_ERROR (Status)) {
> >> + return Status;
> >> + }
> >> +
> >> + //
> >> + // Traverse the device path nodes relative to the filesystem.
> >> + //
> >> + while (!IsDevicePathEnd (*FilePath)) {
> >> + //
> >> + // Keep local variables that relate to the current device path node
> tightly
> >> + // scoped.
> >> + //
> >> + FILEPATH_DEVICE_PATH *FilePathNode;
> >> + CHAR16 *AlignedPathName;
> >> + CHAR16 *PathName;
> >> + EFI_FILE_PROTOCOL *NextFile;
> >> +
> >> + if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
> >> + DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP) {
> >> + Status = EFI_INVALID_PARAMETER;
> >> + goto CloseLastFile;
> >> + }
> >> + FilePathNode = (FILEPATH_DEVICE_PATH *)*FilePath;
> >> +
> >> + //
> >> + // FilePathNode->PathName may be unaligned, and the UEFI
> specification
> >> + // requires pointers that are passed to protocol member functions to
> be
> >> + // aligned. Create an aligned copy of the pathname if necessary.
> >> + //
> >> + if ((UINTN)FilePathNode->PathName % sizeof
> *FilePathNode->PathName == 0) {
> >> + AlignedPathName = NULL;
> >> + PathName = FilePathNode->PathName;
> >> + } else {
> >> + AlignedPathName = AllocateCopyPool (
> >> + (DevicePathNodeLength (FilePathNode) -
> >> + SIZE_OF_FILEPATH_DEVICE_PATH),
> >> + FilePathNode->PathName
> >> + );
> >> + if (AlignedPathName == NULL) {
> >> + Status = EFI_OUT_OF_RESOURCES;
> >> + goto CloseLastFile;
> >> + }
> >> + PathName = AlignedPathName;
> >> + }
> >> +
> >> + //
> >> + // Open the next pathname fragment with EFI_FILE_MODE_CREATE
> masked out and
> >> + // with Attributes set to 0.
> >> + //
> >> + Status = LastFile->Open (
> >> + LastFile,
> >> + &NextFile,
> >> + PathName,
> >> + OpenMode &
> ~(UINT64)EFI_FILE_MODE_CREATE,
> >> + 0
> >> + );
> >> +
> >> + //
> >> + // Retry with EFI_FILE_MODE_CREATE and the original Attributes if the
> first
> >> + // attempt failed, and the caller specified EFI_FILE_MODE_CREATE.
> >> + //
> >> + if (EFI_ERROR (Status) && (OpenMode & EFI_FILE_MODE_CREATE) != 0)
> {
> >> + Status = LastFile->Open (
> >> + LastFile,
> >> + &NextFile,
> >> + PathName,
> >> + OpenMode,
> >> + Attributes
> >> + );
> >> + }
> >> +
> >> + //
> >> + // Release any AlignedPathName on both error and success paths;
> PathName is
> >> + // no longer needed.
> >> + //
> >> + if (AlignedPathName != NULL) {
> >> + FreePool (AlignedPathName);
> >> + }
> >> + if (EFI_ERROR (Status)) {
> >> + goto CloseLastFile;
> >> + }
> >> +
> >> + //
> >> + // Advance to the next device path node.
> >> + //
> >> + LastFile->Close (LastFile);
> >> + LastFile = NextFile;
> >> + *FilePath = NextDevicePathNode (FilePathNode);
> >> + }
> >> +
> >> + *File = LastFile;
> >> + return EFI_SUCCESS;
> >> +
> >> +CloseLastFile:
> >> + LastFile->Close (LastFile);
> >> +
> >> + ASSERT (EFI_ERROR (Status));
> >> + return Status;
> >> +}
> >> --
> >> 2.14.1.3.gb7cf6e02401b
> >>
> >>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath()
2018-07-18 20:50 ` [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath() Laszlo Ersek
2018-07-18 23:10 ` Yao, Jiewen
@ 2018-07-24 17:20 ` Laszlo Ersek
2018-07-27 9:15 ` Ni, Ruiyu
2018-07-27 9:28 ` Ni, Ruiyu
3 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-07-24 17:20 UTC (permalink / raw)
To: edk2-devel-01
Cc: Ruiyu Ni, Eric Dong, Liming Gao, Michael D Kinney, Jiaxin Wu,
Jiewen Yao, Star Zeng, Jaben Carsey, Siyuan Fu, Chao Zhang
On 07/18/18 22:50, Laszlo Ersek wrote:
> The EfiOpenFileByDevicePath() function centralizes functionality from
>
> - MdeModulePkg/Universal/Disk/RamDiskDxe
> - NetworkPkg/TlsAuthConfigDxe
> - SecurityPkg/VariableAuthenticated/SecureBootConfigDxe
> - ShellPkg/Library/UefiShellLib
>
> unifying the implementation and fixing various bugs.
Mike, Liming: do you have comments on this patch?
Thanks!
Laszlo
>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Roman Bacik <roman.bacik@broadcom.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> MdePkg/Library/UefiLib/UefiLib.inf | 1 +
> MdePkg/Include/Library/UefiLib.h | 86 ++++++++
> MdePkg/Library/UefiLib/UefiLib.c | 226 ++++++++++++++++++++
> 3 files changed, 313 insertions(+)
>
> diff --git a/MdePkg/Library/UefiLib/UefiLib.inf b/MdePkg/Library/UefiLib/UefiLib.inf
> index f69f0a43b576..a6c739ef3d6d 100644
> --- a/MdePkg/Library/UefiLib/UefiLib.inf
> +++ b/MdePkg/Library/UefiLib/UefiLib.inf
> @@ -68,6 +68,7 @@ [Protocols]
> gEfiSimpleTextOutProtocolGuid ## SOMETIMES_CONSUMES
> gEfiGraphicsOutputProtocolGuid ## SOMETIMES_CONSUMES
> gEfiHiiFontProtocolGuid ## SOMETIMES_CONSUMES
> + gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES
> gEfiUgaDrawProtocolGuid | gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport ## SOMETIMES_CONSUMES # Consumes if gEfiGraphicsOutputProtocolGuid uninstalled
> gEfiComponentNameProtocolGuid | NOT gEfiMdePkgTokenSpaceGuid.PcdComponentNameDisable ## SOMETIMES_PRODUCES # User chooses to produce it
> gEfiComponentName2ProtocolGuid | NOT gEfiMdePkgTokenSpaceGuid.PcdComponentName2Disable ## SOMETIMES_PRODUCES # User chooses to produce it
> diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
> index 7c6fde620c74..2468bf2aee80 100644
> --- a/MdePkg/Include/Library/UefiLib.h
> +++ b/MdePkg/Include/Library/UefiLib.h
> @@ -33,6 +33,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> #include <Protocol/DriverDiagnostics.h>
> #include <Protocol/DriverDiagnostics2.h>
> #include <Protocol/GraphicsOutput.h>
> +#include <Protocol/DevicePath.h>
> +#include <Protocol/SimpleFileSystem.h>
>
> #include <Library/BaseLib.h>
>
> @@ -1520,4 +1522,88 @@ EfiLocateProtocolBuffer (
> OUT UINTN *NoProtocols,
> OUT VOID ***Buffer
> );
> +
> +/**
> + Open or create a file or directory, possibly creating the chain of
> + directories leading up to the directory.
> +
> + EfiOpenFileByDevicePath() first locates EFI_SIMPLE_FILE_SYSTEM_PROTOCOL on
> + FilePath, and opens the root directory of that filesystem with
> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume().
> +
> + On the remaining device path, the longest initial sequence of
> + FILEPATH_DEVICE_PATH nodes is node-wise traversed with
> + EFI_FILE_PROTOCOL.Open(). For the pathname fragment specified by each
> + traversed FILEPATH_DEVICE_PATH node, EfiOpenFileByDevicePath() first masks
> + EFI_FILE_MODE_CREATE out of OpenMode, and passes 0 for Attributes. If
> + EFI_FILE_PROTOCOL.Open() fails, and OpenMode includes EFI_FILE_MODE_CREATE,
> + then the operation is retried with the caller's OpenMode and Attributes
> + unmodified.
> +
> + (As a consequence, if OpenMode includes EFI_FILE_MODE_CREATE, and Attributes
> + includes EFI_FILE_DIRECTORY, and each FILEPATH_DEVICE_PATH specifies a single
> + pathname component, then EfiOpenFileByDevicePath() ensures that the specified
> + series of subdirectories exist on return.)
> +
> + The EFI_FILE_PROTOCOL identified by the last FILEPATH_DEVICE_PATH node is
> + output to the caller; intermediate EFI_FILE_PROTOCOL instances are closed. If
> + there are no FILEPATH_DEVICE_PATH nodes past the node that identifies the
> + filesystem, then the EFI_FILE_PROTOCOL of the root directory of the
> + filesystem is output to the caller. If a device path node that is different
> + from FILEPATH_DEVICE_PATH is encountered relative to the filesystem, the
> + traversal is stopped with an error, and a NULL EFI_FILE_PROTOCOL is output.
> +
> + @param[in,out] FilePath On input, the device path to the file or directory
> + to open or create. On output, FilePath points one
> + past the last node in the original device path that
> + has been successfully processed. FilePath is set on
> + output even if EfiOpenFileByDevicePath() returns an
> + error.
> +
> + @param[out] File On error, File is set to NULL. On success, File is
> + set to the EFI_FILE_PROTOCOL of the root directory
> + of the filesystem, if there are no
> + FILEPATH_DEVICE_PATH nodes in FilePath; otherwise,
> + File is set to the EFI_FILE_PROTOCOL identified by
> + the last node in FilePath.
> +
> + @param[in] OpenMode The OpenMode parameter to pass to
> + EFI_FILE_PROTOCOL.Open(). For each
> + FILEPATH_DEVICE_PATH node in FilePath,
> + EfiOpenFileByDevicePath() first opens the specified
> + pathname fragment with EFI_FILE_MODE_CREATE masked
> + out of OpenMode and with Attributes set to 0, and
> + only retries the operation with EFI_FILE_MODE_CREATE
> + unmasked and Attributes propagated if the first open
> + attempt fails.
> +
> + @param[in] Attributes The Attributes parameter to pass to
> + EFI_FILE_PROTOCOL.Open(), when EFI_FILE_MODE_CREATE
> + is propagated unmasked in OpenMode.
> +
> + @retval EFI_SUCCESS The file or directory has been opened or
> + created.
> +
> + @retval EFI_INVALID_PARAMETER FilePath is NULL; or File is NULL; or FilePath
> + contains a device path node, past the node
> + that identifies
> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, that is not a
> + FILEPATH_DEVICE_PATH node.
> +
> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
> +
> + @return Error codes propagated from the
> + LocateDevicePath() and OpenProtocol() boot
> + services, and from the
> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume()
> + and EFI_FILE_PROTOCOL.Open() member functions.
> +**/
> +EFI_STATUS
> +EFIAPI
> +EfiOpenFileByDevicePath (
> + IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
> + OUT EFI_FILE_PROTOCOL **File,
> + IN UINT64 OpenMode,
> + IN UINT64 Attributes
> + );
> #endif
> diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
> index 828a54ce7a97..d3e290178cd9 100644
> --- a/MdePkg/Library/UefiLib/UefiLib.c
> +++ b/MdePkg/Library/UefiLib/UefiLib.c
> @@ -1719,3 +1719,229 @@ EfiLocateProtocolBuffer (
>
> return EFI_SUCCESS;
> }
> +
> +/**
> + Open or create a file or directory, possibly creating the chain of
> + directories leading up to the directory.
> +
> + EfiOpenFileByDevicePath() first locates EFI_SIMPLE_FILE_SYSTEM_PROTOCOL on
> + FilePath, and opens the root directory of that filesystem with
> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume().
> +
> + On the remaining device path, the longest initial sequence of
> + FILEPATH_DEVICE_PATH nodes is node-wise traversed with
> + EFI_FILE_PROTOCOL.Open(). For the pathname fragment specified by each
> + traversed FILEPATH_DEVICE_PATH node, EfiOpenFileByDevicePath() first masks
> + EFI_FILE_MODE_CREATE out of OpenMode, and passes 0 for Attributes. If
> + EFI_FILE_PROTOCOL.Open() fails, and OpenMode includes EFI_FILE_MODE_CREATE,
> + then the operation is retried with the caller's OpenMode and Attributes
> + unmodified.
> +
> + (As a consequence, if OpenMode includes EFI_FILE_MODE_CREATE, and Attributes
> + includes EFI_FILE_DIRECTORY, and each FILEPATH_DEVICE_PATH specifies a single
> + pathname component, then EfiOpenFileByDevicePath() ensures that the specified
> + series of subdirectories exist on return.)
> +
> + The EFI_FILE_PROTOCOL identified by the last FILEPATH_DEVICE_PATH node is
> + output to the caller; intermediate EFI_FILE_PROTOCOL instances are closed. If
> + there are no FILEPATH_DEVICE_PATH nodes past the node that identifies the
> + filesystem, then the EFI_FILE_PROTOCOL of the root directory of the
> + filesystem is output to the caller. If a device path node that is different
> + from FILEPATH_DEVICE_PATH is encountered relative to the filesystem, the
> + traversal is stopped with an error, and a NULL EFI_FILE_PROTOCOL is output.
> +
> + @param[in,out] FilePath On input, the device path to the file or directory
> + to open or create. On output, FilePath points one
> + past the last node in the original device path that
> + has been successfully processed. FilePath is set on
> + output even if EfiOpenFileByDevicePath() returns an
> + error.
> +
> + @param[out] File On error, File is set to NULL. On success, File is
> + set to the EFI_FILE_PROTOCOL of the root directory
> + of the filesystem, if there are no
> + FILEPATH_DEVICE_PATH nodes in FilePath; otherwise,
> + File is set to the EFI_FILE_PROTOCOL identified by
> + the last node in FilePath.
> +
> + @param[in] OpenMode The OpenMode parameter to pass to
> + EFI_FILE_PROTOCOL.Open(). For each
> + FILEPATH_DEVICE_PATH node in FilePath,
> + EfiOpenFileByDevicePath() first opens the specified
> + pathname fragment with EFI_FILE_MODE_CREATE masked
> + out of OpenMode and with Attributes set to 0, and
> + only retries the operation with EFI_FILE_MODE_CREATE
> + unmasked and Attributes propagated if the first open
> + attempt fails.
> +
> + @param[in] Attributes The Attributes parameter to pass to
> + EFI_FILE_PROTOCOL.Open(), when EFI_FILE_MODE_CREATE
> + is propagated unmasked in OpenMode.
> +
> + @retval EFI_SUCCESS The file or directory has been opened or
> + created.
> +
> + @retval EFI_INVALID_PARAMETER FilePath is NULL; or File is NULL; or FilePath
> + contains a device path node, past the node
> + that identifies
> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, that is not a
> + FILEPATH_DEVICE_PATH node.
> +
> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
> +
> + @return Error codes propagated from the
> + LocateDevicePath() and OpenProtocol() boot
> + services, and from the
> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.OpenVolume()
> + and EFI_FILE_PROTOCOL.Open() member functions.
> +**/
> +EFI_STATUS
> +EFIAPI
> +EfiOpenFileByDevicePath (
> + IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
> + OUT EFI_FILE_PROTOCOL **File,
> + IN UINT64 OpenMode,
> + IN UINT64 Attributes
> + )
> +{
> + EFI_STATUS Status;
> + EFI_HANDLE FileSystemHandle;
> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *FileSystem;
> + EFI_FILE_PROTOCOL *LastFile;
> +
> + if (File == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> + *File = NULL;
> +
> + if (FilePath == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + //
> + // Look up the filesystem.
> + //
> + Status = gBS->LocateDevicePath (
> + &gEfiSimpleFileSystemProtocolGuid,
> + FilePath,
> + &FileSystemHandle
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + Status = gBS->OpenProtocol (
> + FileSystemHandle,
> + &gEfiSimpleFileSystemProtocolGuid,
> + (VOID **)&FileSystem,
> + gImageHandle,
> + NULL,
> + EFI_OPEN_PROTOCOL_GET_PROTOCOL
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + //
> + // Open the root directory of the filesystem. After this operation succeeds,
> + // we have to release LastFile on error.
> + //
> + Status = FileSystem->OpenVolume (FileSystem, &LastFile);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + //
> + // Traverse the device path nodes relative to the filesystem.
> + //
> + while (!IsDevicePathEnd (*FilePath)) {
> + //
> + // Keep local variables that relate to the current device path node tightly
> + // scoped.
> + //
> + FILEPATH_DEVICE_PATH *FilePathNode;
> + CHAR16 *AlignedPathName;
> + CHAR16 *PathName;
> + EFI_FILE_PROTOCOL *NextFile;
> +
> + if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
> + DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP) {
> + Status = EFI_INVALID_PARAMETER;
> + goto CloseLastFile;
> + }
> + FilePathNode = (FILEPATH_DEVICE_PATH *)*FilePath;
> +
> + //
> + // FilePathNode->PathName may be unaligned, and the UEFI specification
> + // requires pointers that are passed to protocol member functions to be
> + // aligned. Create an aligned copy of the pathname if necessary.
> + //
> + if ((UINTN)FilePathNode->PathName % sizeof *FilePathNode->PathName == 0) {
> + AlignedPathName = NULL;
> + PathName = FilePathNode->PathName;
> + } else {
> + AlignedPathName = AllocateCopyPool (
> + (DevicePathNodeLength (FilePathNode) -
> + SIZE_OF_FILEPATH_DEVICE_PATH),
> + FilePathNode->PathName
> + );
> + if (AlignedPathName == NULL) {
> + Status = EFI_OUT_OF_RESOURCES;
> + goto CloseLastFile;
> + }
> + PathName = AlignedPathName;
> + }
> +
> + //
> + // Open the next pathname fragment with EFI_FILE_MODE_CREATE masked out and
> + // with Attributes set to 0.
> + //
> + Status = LastFile->Open (
> + LastFile,
> + &NextFile,
> + PathName,
> + OpenMode & ~(UINT64)EFI_FILE_MODE_CREATE,
> + 0
> + );
> +
> + //
> + // Retry with EFI_FILE_MODE_CREATE and the original Attributes if the first
> + // attempt failed, and the caller specified EFI_FILE_MODE_CREATE.
> + //
> + if (EFI_ERROR (Status) && (OpenMode & EFI_FILE_MODE_CREATE) != 0) {
> + Status = LastFile->Open (
> + LastFile,
> + &NextFile,
> + PathName,
> + OpenMode,
> + Attributes
> + );
> + }
> +
> + //
> + // Release any AlignedPathName on both error and success paths; PathName is
> + // no longer needed.
> + //
> + if (AlignedPathName != NULL) {
> + FreePool (AlignedPathName);
> + }
> + if (EFI_ERROR (Status)) {
> + goto CloseLastFile;
> + }
> +
> + //
> + // Advance to the next device path node.
> + //
> + LastFile->Close (LastFile);
> + LastFile = NextFile;
> + *FilePath = NextDevicePathNode (FilePathNode);
> + }
> +
> + *File = LastFile;
> + return EFI_SUCCESS;
> +
> +CloseLastFile:
> + LastFile->Close (LastFile);
> +
> + ASSERT (EFI_ERROR (Status));
> + return Status;
> +}
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath()
2018-07-18 20:50 ` [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath() Laszlo Ersek
2018-07-18 23:10 ` Yao, Jiewen
2018-07-24 17:20 ` Laszlo Ersek
@ 2018-07-27 9:15 ` Ni, Ruiyu
2018-07-27 9:28 ` Ni, Ruiyu
3 siblings, 0 replies; 27+ messages in thread
From: Ni, Ruiyu @ 2018-07-27 9:15 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Chao Zhang, Eric Dong, Jaben Carsey, Jiaxin Wu, Jiewen Yao,
Liming Gao, Michael D Kinney, Roman Bacik, Siyuan Fu, Star Zeng
On 7/19/2018 4:50 AM, Laszlo Ersek wrote:
> + //
> + // Open the next pathname fragment with EFI_FILE_MODE_CREATE masked out and
> + // with Attributes set to 0.
> + //
> + Status = LastFile->Open (
> + LastFile,
> + &NextFile,
> + PathName,
> + OpenMode & ~(UINT64)EFI_FILE_MODE_CREATE,
> + 0
> + );
> +
> + //
> + // Retry with EFI_FILE_MODE_CREATE and the original Attributes if the first
> + // attempt failed, and the caller specified EFI_FILE_MODE_CREATE.
> + //
> + if (EFI_ERROR (Status) && (OpenMode & EFI_FILE_MODE_CREATE) != 0) {
> + Status = LastFile->Open (
> + LastFile,
> + &NextFile,
> + PathName,
> + OpenMode,
> + Attributes
> + );
> + }
Laszlo,
Why to open the file firstly with CREATE flag off?
Per spec, when CREATE is set but file exists, the file is opened.
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath()
2018-07-18 20:50 ` [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath() Laszlo Ersek
` (2 preceding siblings ...)
2018-07-27 9:15 ` Ni, Ruiyu
@ 2018-07-27 9:28 ` Ni, Ruiyu
2018-07-27 12:06 ` Laszlo Ersek
3 siblings, 1 reply; 27+ messages in thread
From: Ni, Ruiyu @ 2018-07-27 9:28 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Chao Zhang, Eric Dong, Jaben Carsey, Jiaxin Wu, Jiewen Yao,
Liming Gao, Michael D Kinney, Roman Bacik, Siyuan Fu, Star Zeng
On 7/19/2018 4:50 AM, Laszlo Ersek wrote:
> + //
> + // Traverse the device path nodes relative to the filesystem.
> + //
> + while (!IsDevicePathEnd (*FilePath)) {
> + //
> + // Keep local variables that relate to the current device path node tightly
> + // scoped.
> + //
> + FILEPATH_DEVICE_PATH *FilePathNode;
> + CHAR16 *AlignedPathName;
> + CHAR16 *PathName;
> + EFI_FILE_PROTOCOL *NextFile;
1. Not sure if it follows the coding style. I would prefer to move the
definition to the beginning of the function.
> +
> + if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
> + DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP) {
> + Status = EFI_INVALID_PARAMETER;
> + goto CloseLastFile;
> + }
> + FilePathNode = (FILEPATH_DEVICE_PATH *)*FilePath;
> +
> + //
> + // FilePathNode->PathName may be unaligned, and the UEFI specification
> + // requires pointers that are passed to protocol member functions to be
> + // aligned. Create an aligned copy of the pathname if necessary.
> + //
> + if ((UINTN)FilePathNode->PathName % sizeof *FilePathNode->PathName == 0) {
> + AlignedPathName = NULL;
> + PathName = FilePathNode->PathName;
> + } else {
> + AlignedPathName = AllocateCopyPool (
> + (DevicePathNodeLength (FilePathNode) -
> + SIZE_OF_FILEPATH_DEVICE_PATH),
> + FilePathNode->PathName
> + );
> + if (AlignedPathName == NULL) {
> + Status = EFI_OUT_OF_RESOURCES;
> + goto CloseLastFile;
> + }
> + PathName = AlignedPathName;
> + }
> +
> + //
> + // Open the next pathname fragment with EFI_FILE_MODE_CREATE masked out and
> + // with Attributes set to 0.
> + //
> + Status = LastFile->Open (
> + LastFile,
> + &NextFile,
> + PathName,
> + OpenMode & ~(UINT64)EFI_FILE_MODE_CREATE,
> + 0
> + );
2. As I said in previous mail, is it really needed?
Per spec it's not required. Per FAT driver implementation, it's also not
required.
> +
> + //
> + // Retry with EFI_FILE_MODE_CREATE and the original Attributes if the first
> + // attempt failed, and the caller specified EFI_FILE_MODE_CREATE.
> + //
> + if (EFI_ERROR (Status) && (OpenMode & EFI_FILE_MODE_CREATE) != 0) {
> + Status = LastFile->Open (
> + LastFile,
> + &NextFile,
> + PathName,
> + OpenMode,
> + Attributes
> + );
> + }
> +
> + //
> + // Release any AlignedPathName on both error and success paths; PathName is
> + // no longer needed.
> + //
> + if (AlignedPathName != NULL) {
> + FreePool (AlignedPathName);
> + }
> + if (EFI_ERROR (Status)) {
> + goto CloseLastFile;
> + }
> +
> + //
> + // Advance to the next device path node.
> + //
> + LastFile->Close (LastFile);
> + LastFile = NextFile;
> + *FilePath = NextDevicePathNode (FilePathNode);
> + }
> +
> + *File = LastFile;
> + return EFI_SUCCESS;
> +
> +CloseLastFile:
> + LastFile->Close (LastFile);
> +
> + ASSERT (EFI_ERROR (Status));
3. ASSERT_EFI_ERROR (Status);
> + return Status;
> +}
>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath()
2018-07-27 9:28 ` Ni, Ruiyu
@ 2018-07-27 12:06 ` Laszlo Ersek
2018-07-30 1:54 ` Ni, Ruiyu
0 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2018-07-27 12:06 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel-01
Cc: Chao Zhang, Eric Dong, Jaben Carsey, Jiaxin Wu, Jiewen Yao,
Liming Gao, Michael D Kinney, Roman Bacik, Siyuan Fu, Star Zeng
On 07/27/18 11:28, Ni, Ruiyu wrote:
> On 7/19/2018 4:50 AM, Laszlo Ersek wrote:
>
>> + //
>> + // Traverse the device path nodes relative to the filesystem.
>> + //
>> + while (!IsDevicePathEnd (*FilePath)) {
>> + //
>> + // Keep local variables that relate to the current device path
>> node tightly
>> + // scoped.
>> + //
>> + FILEPATH_DEVICE_PATH *FilePathNode;
>> + CHAR16 *AlignedPathName;
>> + CHAR16 *PathName;
>> + EFI_FILE_PROTOCOL *NextFile;
> 1. Not sure if it follows the coding style. I would prefer to move the
> definition to the beginning of the function.
OK, will do.
>
>> +
>> + if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
>> + DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP) {
>> + Status = EFI_INVALID_PARAMETER;
>> + goto CloseLastFile;
>> + }
>> + FilePathNode = (FILEPATH_DEVICE_PATH *)*FilePath;
>> +
>> + //
>> + // FilePathNode->PathName may be unaligned, and the UEFI
>> specification
>> + // requires pointers that are passed to protocol member functions
>> to be
>> + // aligned. Create an aligned copy of the pathname if necessary.
>> + //
>> + if ((UINTN)FilePathNode->PathName % sizeof
>> *FilePathNode->PathName == 0) {
>> + AlignedPathName = NULL;
>> + PathName = FilePathNode->PathName;
>> + } else {
>> + AlignedPathName = AllocateCopyPool (
>> + (DevicePathNodeLength (FilePathNode) -
>> + SIZE_OF_FILEPATH_DEVICE_PATH),
>> + FilePathNode->PathName
>> + );
>> + if (AlignedPathName == NULL) {
>> + Status = EFI_OUT_OF_RESOURCES;
>> + goto CloseLastFile;
>> + }
>> + PathName = AlignedPathName;
>> + }
>> +
>> + //
>> + // Open the next pathname fragment with EFI_FILE_MODE_CREATE
>> masked out and
>> + // with Attributes set to 0.
>> + //
>> + Status = LastFile->Open (
>> + LastFile,
>> + &NextFile,
>> + PathName,
>> + OpenMode & ~(UINT64)EFI_FILE_MODE_CREATE,
>> + 0
>> + );
> 2. As I said in previous mail, is it really needed?
> Per spec it's not required. Per FAT driver implementation, it's also not
> required.
I can do that, but it's out of scope for this series. The behavior that
you point out is not a functionality bug (it is not observably erroneous
behavior), just sub-optimal implementation. This series is about
unifying the implementation and fixing those issues that are actual
bugs. I suggest that we report a separate BZ about this question,
discuss it separately, and then I can send a separate patch (which will
benefit all client code at once).
Does that sound acceptable?
>
>> +
>> + //
>> + // Retry with EFI_FILE_MODE_CREATE and the original Attributes if
>> the first
>> + // attempt failed, and the caller specified EFI_FILE_MODE_CREATE.
>> + //
>> + if (EFI_ERROR (Status) && (OpenMode & EFI_FILE_MODE_CREATE) != 0) {
>> + Status = LastFile->Open (
>> + LastFile,
>> + &NextFile,
>> + PathName,
>> + OpenMode,
>> + Attributes
>> + );
>> + }
>> +
>> + //
>> + // Release any AlignedPathName on both error and success paths;
>> PathName is
>> + // no longer needed.
>> + //
>> + if (AlignedPathName != NULL) {
>> + FreePool (AlignedPathName);
>> + }
>> + if (EFI_ERROR (Status)) {
>> + goto CloseLastFile;
>> + }
>> +
>> + //
>> + // Advance to the next device path node.
>> + //
>> + LastFile->Close (LastFile);
>> + LastFile = NextFile;
>> + *FilePath = NextDevicePathNode (FilePathNode);
>> + }
>> +
>> + *File = LastFile;
>> + return EFI_SUCCESS;
>> +
>> +CloseLastFile:
>> + LastFile->Close (LastFile);
>> +
>> + ASSERT (EFI_ERROR (Status));
> 3. ASSERT_EFI_ERROR (Status);
No, that's not correct; I *really* meant
ASSERT (EFI_ERROR (Status))
Please find the explanation here:
https://lists.01.org/pipermail/edk2-devel/2018-July/027288.html
However, given that both Jaben and you were confused by this, I agree
that I should add a comment before the assert:
//
// We are on the error path; we must have set an error Status for
// returning to the caller.
//
Thanks!
Laszlo
>
>> + return Status;
>> +}
>>
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath()
2018-07-27 12:06 ` Laszlo Ersek
@ 2018-07-30 1:54 ` Ni, Ruiyu
2018-07-30 14:13 ` Laszlo Ersek
0 siblings, 1 reply; 27+ messages in thread
From: Ni, Ruiyu @ 2018-07-30 1:54 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Chao Zhang, Eric Dong, Jaben Carsey, Jiaxin Wu, Jiewen Yao,
Liming Gao, Michael D Kinney, Roman Bacik, Siyuan Fu, Star Zeng
On 7/27/2018 8:06 PM, Laszlo Ersek wrote:
> On 07/27/18 11:28, Ni, Ruiyu wrote:
>> On 7/19/2018 4:50 AM, Laszlo Ersek wrote:
>>
>>> + //
>>> + // Traverse the device path nodes relative to the filesystem.
>>> + //
>>> + while (!IsDevicePathEnd (*FilePath)) {
>>> + //
>>> + // Keep local variables that relate to the current device path
>>> node tightly
>>> + // scoped.
>>> + //
>>> + FILEPATH_DEVICE_PATH *FilePathNode;
>>> + CHAR16 *AlignedPathName;
>>> + CHAR16 *PathName;
>>> + EFI_FILE_PROTOCOL *NextFile;
>> 1. Not sure if it follows the coding style. I would prefer to move the
>> definition to the beginning of the function.
>
> OK, will do.
Thanks!
>
>>
>>> +
>>> + if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
>>> + DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP) {
>>> + Status = EFI_INVALID_PARAMETER;
>>> + goto CloseLastFile;
>>> + }
>>> + FilePathNode = (FILEPATH_DEVICE_PATH *)*FilePath;
>>> +
>>> + //
>>> + // FilePathNode->PathName may be unaligned, and the UEFI
>>> specification
>>> + // requires pointers that are passed to protocol member functions
>>> to be
>>> + // aligned. Create an aligned copy of the pathname if necessary.
>>> + //
>>> + if ((UINTN)FilePathNode->PathName % sizeof
>>> *FilePathNode->PathName == 0) {
>>> + AlignedPathName = NULL;
>>> + PathName = FilePathNode->PathName;
>>> + } else {
>>> + AlignedPathName = AllocateCopyPool (
>>> + (DevicePathNodeLength (FilePathNode) -
>>> + SIZE_OF_FILEPATH_DEVICE_PATH),
>>> + FilePathNode->PathName
>>> + );
>>> + if (AlignedPathName == NULL) {
>>> + Status = EFI_OUT_OF_RESOURCES;
>>> + goto CloseLastFile;
>>> + }
>>> + PathName = AlignedPathName;
>>> + }
>>> +
>>> + //
>>> + // Open the next pathname fragment with EFI_FILE_MODE_CREATE
>>> masked out and
>>> + // with Attributes set to 0.
>>> + //
>>> + Status = LastFile->Open (
>>> + LastFile,
>>> + &NextFile,
>>> + PathName,
>>> + OpenMode & ~(UINT64)EFI_FILE_MODE_CREATE,
>>> + 0
>>> + );
>> 2. As I said in previous mail, is it really needed?
>> Per spec it's not required. Per FAT driver implementation, it's also not
>> required.
>
> I can do that, but it's out of scope for this series. The behavior that
> you point out is not a functionality bug (it is not observably erroneous
> behavior), just sub-optimal implementation. This series is about
> unifying the implementation and fixing those issues that are actual
> bugs. I suggest that we report a separate BZ about this question,
> discuss it separately, and then I can send a separate patch (which will
> benefit all client code at once).
>
> Does that sound acceptable?
I agree.
>
>>
>>> +
>>> + //
>>> + // Retry with EFI_FILE_MODE_CREATE and the original Attributes if
>>> the first
>>> + // attempt failed, and the caller specified EFI_FILE_MODE_CREATE.
>>> + //
>>> + if (EFI_ERROR (Status) && (OpenMode & EFI_FILE_MODE_CREATE) != 0) {
>>> + Status = LastFile->Open (
>>> + LastFile,
>>> + &NextFile,
>>> + PathName,
>>> + OpenMode,
>>> + Attributes
>>> + );
>>> + }
>>> +
>>> + //
>>> + // Release any AlignedPathName on both error and success paths;
>>> PathName is
>>> + // no longer needed.
>>> + //
>>> + if (AlignedPathName != NULL) {
>>> + FreePool (AlignedPathName);
>>> + }
>>> + if (EFI_ERROR (Status)) {
>>> + goto CloseLastFile;
>>> + }
>>> +
>>> + //
>>> + // Advance to the next device path node.
>>> + //
>>> + LastFile->Close (LastFile);
>>> + LastFile = NextFile;
>>> + *FilePath = NextDevicePathNode (FilePathNode);
>>> + }
>>> +
>>> + *File = LastFile;
>>> + return EFI_SUCCESS;
>>> +
>>> +CloseLastFile:
>>> + LastFile->Close (LastFile);
>>> +
>>> + ASSERT (EFI_ERROR (Status));
>> 3. ASSERT_EFI_ERROR (Status);
>
> No, that's not correct; I *really* meant
>
> ASSERT (EFI_ERROR (Status))
>
> Please find the explanation here:
>
> https://lists.01.org/pipermail/edk2-devel/2018-July/027288.html
>
> However, given that both Jaben and you were confused by this, I agree
> that I should add a comment before the assert:
>
> //
> // We are on the error path; we must have set an error Status for
> // returning to the caller.
> //
I just found there is no "!" before "EFI_ERROR".
It's really confusing. I agree a comment before that is better.
Thanks!
With the comment added, Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
>
> Thanks!
> Laszlo
>
>>
>>> + return Status;
>>> +}
>>>
>>
>>
>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath()
2018-07-30 1:54 ` Ni, Ruiyu
@ 2018-07-30 14:13 ` Laszlo Ersek
2018-08-02 4:06 ` Gao, Liming
0 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2018-07-30 14:13 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel-01
Cc: Chao Zhang, Eric Dong, Jaben Carsey, Jiaxin Wu, Jiewen Yao,
Liming Gao, Michael D Kinney, Roman Bacik, Siyuan Fu, Star Zeng
On 07/30/18 03:54, Ni, Ruiyu wrote:
> On 7/27/2018 8:06 PM, Laszlo Ersek wrote:
>> On 07/27/18 11:28, Ni, Ruiyu wrote:
>>> On 7/19/2018 4:50 AM, Laszlo Ersek wrote:
>>>
>>>> + //
>>>> + // Traverse the device path nodes relative to the filesystem.
>>>> + //
>>>> + while (!IsDevicePathEnd (*FilePath)) {
>>>> + //
>>>> + // Keep local variables that relate to the current device path
>>>> node tightly
>>>> + // scoped.
>>>> + //
>>>> + FILEPATH_DEVICE_PATH *FilePathNode;
>>>> + CHAR16 *AlignedPathName;
>>>> + CHAR16 *PathName;
>>>> + EFI_FILE_PROTOCOL *NextFile;
>>> 1. Not sure if it follows the coding style. I would prefer to move the
>>> definition to the beginning of the function.
>>
>> OK, will do.
>
> Thanks!
>
>>
>>>
>>>> +
>>>> + if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
>>>> + DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP) {
>>>> + Status = EFI_INVALID_PARAMETER;
>>>> + goto CloseLastFile;
>>>> + }
>>>> + FilePathNode = (FILEPATH_DEVICE_PATH *)*FilePath;
>>>> +
>>>> + //
>>>> + // FilePathNode->PathName may be unaligned, and the UEFI
>>>> specification
>>>> + // requires pointers that are passed to protocol member functions
>>>> to be
>>>> + // aligned. Create an aligned copy of the pathname if necessary.
>>>> + //
>>>> + if ((UINTN)FilePathNode->PathName % sizeof
>>>> *FilePathNode->PathName == 0) {
>>>> + AlignedPathName = NULL;
>>>> + PathName = FilePathNode->PathName;
>>>> + } else {
>>>> + AlignedPathName = AllocateCopyPool (
>>>> + (DevicePathNodeLength (FilePathNode) -
>>>> + SIZE_OF_FILEPATH_DEVICE_PATH),
>>>> + FilePathNode->PathName
>>>> + );
>>>> + if (AlignedPathName == NULL) {
>>>> + Status = EFI_OUT_OF_RESOURCES;
>>>> + goto CloseLastFile;
>>>> + }
>>>> + PathName = AlignedPathName;
>>>> + }
>>>> +
>>>> + //
>>>> + // Open the next pathname fragment with EFI_FILE_MODE_CREATE
>>>> masked out and
>>>> + // with Attributes set to 0.
>>>> + //
>>>> + Status = LastFile->Open (
>>>> + LastFile,
>>>> + &NextFile,
>>>> + PathName,
>>>> + OpenMode & ~(UINT64)EFI_FILE_MODE_CREATE,
>>>> + 0
>>>> + );
>>> 2. As I said in previous mail, is it really needed?
>>> Per spec it's not required. Per FAT driver implementation, it's also not
>>> required.
>>
>> I can do that, but it's out of scope for this series. The behavior that
>> you point out is not a functionality bug (it is not observably erroneous
>> behavior), just sub-optimal implementation. This series is about
>> unifying the implementation and fixing those issues that are actual
>> bugs. I suggest that we report a separate BZ about this question,
>> discuss it separately, and then I can send a separate patch (which will
>> benefit all client code at once).
>>
>> Does that sound acceptable?
>
> I agree.
>
>>
>>>
>>>> +
>>>> + //
>>>> + // Retry with EFI_FILE_MODE_CREATE and the original Attributes if
>>>> the first
>>>> + // attempt failed, and the caller specified EFI_FILE_MODE_CREATE.
>>>> + //
>>>> + if (EFI_ERROR (Status) && (OpenMode & EFI_FILE_MODE_CREATE) !=
>>>> 0) {
>>>> + Status = LastFile->Open (
>>>> + LastFile,
>>>> + &NextFile,
>>>> + PathName,
>>>> + OpenMode,
>>>> + Attributes
>>>> + );
>>>> + }
>>>> +
>>>> + //
>>>> + // Release any AlignedPathName on both error and success paths;
>>>> PathName is
>>>> + // no longer needed.
>>>> + //
>>>> + if (AlignedPathName != NULL) {
>>>> + FreePool (AlignedPathName);
>>>> + }
>>>> + if (EFI_ERROR (Status)) {
>>>> + goto CloseLastFile;
>>>> + }
>>>> +
>>>> + //
>>>> + // Advance to the next device path node.
>>>> + //
>>>> + LastFile->Close (LastFile);
>>>> + LastFile = NextFile;
>>>> + *FilePath = NextDevicePathNode (FilePathNode);
>>>> + }
>>>> +
>>>> + *File = LastFile;
>>>> + return EFI_SUCCESS;
>>>> +
>>>> +CloseLastFile:
>>>> + LastFile->Close (LastFile);
>>>> +
>>>> + ASSERT (EFI_ERROR (Status));
>>> 3. ASSERT_EFI_ERROR (Status);
>>
>> No, that's not correct; I *really* meant
>>
>> ASSERT (EFI_ERROR (Status))
>>
>> Please find the explanation here:
>>
>> https://lists.01.org/pipermail/edk2-devel/2018-July/027288.html
>>
>> However, given that both Jaben and you were confused by this, I agree
>> that I should add a comment before the assert:
>>
>> //
>> // We are on the error path; we must have set an error Status for
>> // returning to the caller.
>> //
>
> I just found there is no "!" before "EFI_ERROR".
> It's really confusing. I agree a comment before that is better.
> Thanks!
>
> With the comment added, Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
Thanks, Ray -- looks like I've almost got enough feedback for posting
v2; however I haven't received any MdePkg maintainer feedback (from Mike
and/or Liming) yet. Am I to understand your review as a substitute for
theirs, or as an addition to theirs?
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath()
2018-07-30 14:13 ` Laszlo Ersek
@ 2018-08-02 4:06 ` Gao, Liming
2018-08-02 14:45 ` Laszlo Ersek
0 siblings, 1 reply; 27+ messages in thread
From: Gao, Liming @ 2018-08-02 4:06 UTC (permalink / raw)
To: Laszlo Ersek, Ni, Ruiyu, edk2-devel-01
Cc: Zhang, Chao B, Dong, Eric, Carsey, Jaben, Wu, Jiaxin, Yao, Jiewen,
Kinney, Michael D, Roman Bacik, Fu, Siyuan, Zeng, Star
Laszlo:
I have no other comments. IntelFrameworkPkg has another UefiLib library instance FrameworkUefiLib. Could you help update it also?
For MdePkg, you can add Reviewed-by: Liming Gao <liming.gao@intel.com>
Thanks
Liming
>-----Original Message-----
>From: Laszlo Ersek [mailto:lersek@redhat.com]
>Sent: Monday, July 30, 2018 10:14 PM
>To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
>Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Dong, Eric
><eric.dong@intel.com>; Carsey, Jaben <jaben.carsey@intel.com>; Wu, Jiaxin
><jiaxin.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming
><liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
>Roman Bacik <roman.bacik@broadcom.com>; Fu, Siyuan
><siyuan.fu@intel.com>; Zeng, Star <star.zeng@intel.com>
>Subject: Re: [PATCH 1/6] MdePkg/UefiLib: introduce
>EfiOpenFileByDevicePath()
>
>On 07/30/18 03:54, Ni, Ruiyu wrote:
>> On 7/27/2018 8:06 PM, Laszlo Ersek wrote:
>>> On 07/27/18 11:28, Ni, Ruiyu wrote:
>>>> On 7/19/2018 4:50 AM, Laszlo Ersek wrote:
>>>>
>>>>> + //
>>>>> + // Traverse the device path nodes relative to the filesystem.
>>>>> + //
>>>>> + while (!IsDevicePathEnd (*FilePath)) {
>>>>> + //
>>>>> + // Keep local variables that relate to the current device path
>>>>> node tightly
>>>>> + // scoped.
>>>>> + //
>>>>> + FILEPATH_DEVICE_PATH *FilePathNode;
>>>>> + CHAR16 *AlignedPathName;
>>>>> + CHAR16 *PathName;
>>>>> + EFI_FILE_PROTOCOL *NextFile;
>>>> 1. Not sure if it follows the coding style. I would prefer to move the
>>>> definition to the beginning of the function.
>>>
>>> OK, will do.
>>
>> Thanks!
>>
>>>
>>>>
>>>>> +
>>>>> + if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
>>>>> + DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP) {
>>>>> + Status = EFI_INVALID_PARAMETER;
>>>>> + goto CloseLastFile;
>>>>> + }
>>>>> + FilePathNode = (FILEPATH_DEVICE_PATH *)*FilePath;
>>>>> +
>>>>> + //
>>>>> + // FilePathNode->PathName may be unaligned, and the UEFI
>>>>> specification
>>>>> + // requires pointers that are passed to protocol member functions
>>>>> to be
>>>>> + // aligned. Create an aligned copy of the pathname if necessary.
>>>>> + //
>>>>> + if ((UINTN)FilePathNode->PathName % sizeof
>>>>> *FilePathNode->PathName == 0) {
>>>>> + AlignedPathName = NULL;
>>>>> + PathName = FilePathNode->PathName;
>>>>> + } else {
>>>>> + AlignedPathName = AllocateCopyPool (
>>>>> + (DevicePathNodeLength (FilePathNode) -
>>>>> + SIZE_OF_FILEPATH_DEVICE_PATH),
>>>>> + FilePathNode->PathName
>>>>> + );
>>>>> + if (AlignedPathName == NULL) {
>>>>> + Status = EFI_OUT_OF_RESOURCES;
>>>>> + goto CloseLastFile;
>>>>> + }
>>>>> + PathName = AlignedPathName;
>>>>> + }
>>>>> +
>>>>> + //
>>>>> + // Open the next pathname fragment with EFI_FILE_MODE_CREATE
>>>>> masked out and
>>>>> + // with Attributes set to 0.
>>>>> + //
>>>>> + Status = LastFile->Open (
>>>>> + LastFile,
>>>>> + &NextFile,
>>>>> + PathName,
>>>>> + OpenMode & ~(UINT64)EFI_FILE_MODE_CREATE,
>>>>> + 0
>>>>> + );
>>>> 2. As I said in previous mail, is it really needed?
>>>> Per spec it's not required. Per FAT driver implementation, it's also not
>>>> required.
>>>
>>> I can do that, but it's out of scope for this series. The behavior that
>>> you point out is not a functionality bug (it is not observably erroneous
>>> behavior), just sub-optimal implementation. This series is about
>>> unifying the implementation and fixing those issues that are actual
>>> bugs. I suggest that we report a separate BZ about this question,
>>> discuss it separately, and then I can send a separate patch (which will
>>> benefit all client code at once).
>>>
>>> Does that sound acceptable?
>>
>> I agree.
>>
>>>
>>>>
>>>>> +
>>>>> + //
>>>>> + // Retry with EFI_FILE_MODE_CREATE and the original Attributes if
>>>>> the first
>>>>> + // attempt failed, and the caller specified EFI_FILE_MODE_CREATE.
>>>>> + //
>>>>> + if (EFI_ERROR (Status) && (OpenMode &
>EFI_FILE_MODE_CREATE) !=
>>>>> 0) {
>>>>> + Status = LastFile->Open (
>>>>> + LastFile,
>>>>> + &NextFile,
>>>>> + PathName,
>>>>> + OpenMode,
>>>>> + Attributes
>>>>> + );
>>>>> + }
>>>>> +
>>>>> + //
>>>>> + // Release any AlignedPathName on both error and success paths;
>>>>> PathName is
>>>>> + // no longer needed.
>>>>> + //
>>>>> + if (AlignedPathName != NULL) {
>>>>> + FreePool (AlignedPathName);
>>>>> + }
>>>>> + if (EFI_ERROR (Status)) {
>>>>> + goto CloseLastFile;
>>>>> + }
>>>>> +
>>>>> + //
>>>>> + // Advance to the next device path node.
>>>>> + //
>>>>> + LastFile->Close (LastFile);
>>>>> + LastFile = NextFile;
>>>>> + *FilePath = NextDevicePathNode (FilePathNode);
>>>>> + }
>>>>> +
>>>>> + *File = LastFile;
>>>>> + return EFI_SUCCESS;
>>>>> +
>>>>> +CloseLastFile:
>>>>> + LastFile->Close (LastFile);
>>>>> +
>>>>> + ASSERT (EFI_ERROR (Status));
>>>> 3. ASSERT_EFI_ERROR (Status);
>>>
>>> No, that's not correct; I *really* meant
>>>
>>> ASSERT (EFI_ERROR (Status))
>>>
>>> Please find the explanation here:
>>>
>>> https://lists.01.org/pipermail/edk2-devel/2018-July/027288.html
>>>
>>> However, given that both Jaben and you were confused by this, I agree
>>> that I should add a comment before the assert:
>>>
>>> //
>>> // We are on the error path; we must have set an error Status for
>>> // returning to the caller.
>>> //
>>
>> I just found there is no "!" before "EFI_ERROR".
>> It's really confusing. I agree a comment before that is better.
>> Thanks!
>>
>> With the comment added, Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
>
>Thanks, Ray -- looks like I've almost got enough feedback for posting
>v2; however I haven't received any MdePkg maintainer feedback (from Mike
>and/or Liming) yet. Am I to understand your review as a substitute for
>theirs, or as an addition to theirs?
>
>Thanks!
>Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath()
2018-08-02 4:06 ` Gao, Liming
@ 2018-08-02 14:45 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-08-02 14:45 UTC (permalink / raw)
To: Gao, Liming, Ni, Ruiyu, edk2-devel-01
Cc: Dong, Eric, Kinney, Michael D, Wu, Jiaxin, Yao, Jiewen,
Zeng, Star, Carsey, Jaben, Fu, Siyuan, Zhang, Chao B
On 08/02/18 06:06, Gao, Liming wrote:
> Laszlo:
> I have no other comments. IntelFrameworkPkg has another UefiLib library instance FrameworkUefiLib. Could you help update it also?
>
> For MdePkg, you can add Reviewed-by: Liming Gao <liming.gao@intel.com>
Thanks! I have now enough feedback for posting v2.
Cheers
Laszlo
>
> Thanks
> Liming
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, July 30, 2018 10:14 PM
>> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Dong, Eric
>> <eric.dong@intel.com>; Carsey, Jaben <jaben.carsey@intel.com>; Wu, Jiaxin
>> <jiaxin.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
>> Roman Bacik <roman.bacik@broadcom.com>; Fu, Siyuan
>> <siyuan.fu@intel.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: Re: [PATCH 1/6] MdePkg/UefiLib: introduce
>> EfiOpenFileByDevicePath()
>>
>> On 07/30/18 03:54, Ni, Ruiyu wrote:
>>> On 7/27/2018 8:06 PM, Laszlo Ersek wrote:
>>>> On 07/27/18 11:28, Ni, Ruiyu wrote:
>>>>> On 7/19/2018 4:50 AM, Laszlo Ersek wrote:
>>>>>
>>>>>> + //
>>>>>> + // Traverse the device path nodes relative to the filesystem.
>>>>>> + //
>>>>>> + while (!IsDevicePathEnd (*FilePath)) {
>>>>>> + //
>>>>>> + // Keep local variables that relate to the current device path
>>>>>> node tightly
>>>>>> + // scoped.
>>>>>> + //
>>>>>> + FILEPATH_DEVICE_PATH *FilePathNode;
>>>>>> + CHAR16 *AlignedPathName;
>>>>>> + CHAR16 *PathName;
>>>>>> + EFI_FILE_PROTOCOL *NextFile;
>>>>> 1. Not sure if it follows the coding style. I would prefer to move the
>>>>> definition to the beginning of the function.
>>>>
>>>> OK, will do.
>>>
>>> Thanks!
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> + if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
>>>>>> + DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP) {
>>>>>> + Status = EFI_INVALID_PARAMETER;
>>>>>> + goto CloseLastFile;
>>>>>> + }
>>>>>> + FilePathNode = (FILEPATH_DEVICE_PATH *)*FilePath;
>>>>>> +
>>>>>> + //
>>>>>> + // FilePathNode->PathName may be unaligned, and the UEFI
>>>>>> specification
>>>>>> + // requires pointers that are passed to protocol member functions
>>>>>> to be
>>>>>> + // aligned. Create an aligned copy of the pathname if necessary.
>>>>>> + //
>>>>>> + if ((UINTN)FilePathNode->PathName % sizeof
>>>>>> *FilePathNode->PathName == 0) {
>>>>>> + AlignedPathName = NULL;
>>>>>> + PathName = FilePathNode->PathName;
>>>>>> + } else {
>>>>>> + AlignedPathName = AllocateCopyPool (
>>>>>> + (DevicePathNodeLength (FilePathNode) -
>>>>>> + SIZE_OF_FILEPATH_DEVICE_PATH),
>>>>>> + FilePathNode->PathName
>>>>>> + );
>>>>>> + if (AlignedPathName == NULL) {
>>>>>> + Status = EFI_OUT_OF_RESOURCES;
>>>>>> + goto CloseLastFile;
>>>>>> + }
>>>>>> + PathName = AlignedPathName;
>>>>>> + }
>>>>>> +
>>>>>> + //
>>>>>> + // Open the next pathname fragment with EFI_FILE_MODE_CREATE
>>>>>> masked out and
>>>>>> + // with Attributes set to 0.
>>>>>> + //
>>>>>> + Status = LastFile->Open (
>>>>>> + LastFile,
>>>>>> + &NextFile,
>>>>>> + PathName,
>>>>>> + OpenMode & ~(UINT64)EFI_FILE_MODE_CREATE,
>>>>>> + 0
>>>>>> + );
>>>>> 2. As I said in previous mail, is it really needed?
>>>>> Per spec it's not required. Per FAT driver implementation, it's also not
>>>>> required.
>>>>
>>>> I can do that, but it's out of scope for this series. The behavior that
>>>> you point out is not a functionality bug (it is not observably erroneous
>>>> behavior), just sub-optimal implementation. This series is about
>>>> unifying the implementation and fixing those issues that are actual
>>>> bugs. I suggest that we report a separate BZ about this question,
>>>> discuss it separately, and then I can send a separate patch (which will
>>>> benefit all client code at once).
>>>>
>>>> Does that sound acceptable?
>>>
>>> I agree.
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> + //
>>>>>> + // Retry with EFI_FILE_MODE_CREATE and the original Attributes if
>>>>>> the first
>>>>>> + // attempt failed, and the caller specified EFI_FILE_MODE_CREATE.
>>>>>> + //
>>>>>> + if (EFI_ERROR (Status) && (OpenMode &
>> EFI_FILE_MODE_CREATE) !=
>>>>>> 0) {
>>>>>> + Status = LastFile->Open (
>>>>>> + LastFile,
>>>>>> + &NextFile,
>>>>>> + PathName,
>>>>>> + OpenMode,
>>>>>> + Attributes
>>>>>> + );
>>>>>> + }
>>>>>> +
>>>>>> + //
>>>>>> + // Release any AlignedPathName on both error and success paths;
>>>>>> PathName is
>>>>>> + // no longer needed.
>>>>>> + //
>>>>>> + if (AlignedPathName != NULL) {
>>>>>> + FreePool (AlignedPathName);
>>>>>> + }
>>>>>> + if (EFI_ERROR (Status)) {
>>>>>> + goto CloseLastFile;
>>>>>> + }
>>>>>> +
>>>>>> + //
>>>>>> + // Advance to the next device path node.
>>>>>> + //
>>>>>> + LastFile->Close (LastFile);
>>>>>> + LastFile = NextFile;
>>>>>> + *FilePath = NextDevicePathNode (FilePathNode);
>>>>>> + }
>>>>>> +
>>>>>> + *File = LastFile;
>>>>>> + return EFI_SUCCESS;
>>>>>> +
>>>>>> +CloseLastFile:
>>>>>> + LastFile->Close (LastFile);
>>>>>> +
>>>>>> + ASSERT (EFI_ERROR (Status));
>>>>> 3. ASSERT_EFI_ERROR (Status);
>>>>
>>>> No, that's not correct; I *really* meant
>>>>
>>>> ASSERT (EFI_ERROR (Status))
>>>>
>>>> Please find the explanation here:
>>>>
>>>> https://lists.01.org/pipermail/edk2-devel/2018-July/027288.html
>>>>
>>>> However, given that both Jaben and you were confused by this, I agree
>>>> that I should add a comment before the assert:
>>>>
>>>> //
>>>> // We are on the error path; we must have set an error Status for
>>>> // returning to the caller.
>>>> //
>>>
>>> I just found there is no "!" before "EFI_ERROR".
>>> It's really confusing. I agree a comment before that is better.
>>> Thanks!
>>>
>>> With the comment added, Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
>>
>> Thanks, Ray -- looks like I've almost got enough feedback for posting
>> v2; however I haven't received any MdePkg maintainer feedback (from Mike
>> and/or Liming) yet. Am I to understand your review as a substitute for
>> theirs, or as an addition to theirs?
>>
>> Thanks!
>> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 2/6] MdeModulePkg/RamDiskDxe: replace OpenFileByDevicePath() with UefiLib API
2018-07-18 20:50 [PATCH 0/6] UefiLib: centralize OpenFileByDevicePath() and fix its bugs Laszlo Ersek
2018-07-18 20:50 ` [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath() Laszlo Ersek
@ 2018-07-18 20:50 ` Laszlo Ersek
2018-07-19 10:36 ` Zeng, Star
2018-07-18 20:50 ` [PATCH 3/6] NetworkPkg/TlsAuthConfigDxe: " Laszlo Ersek
` (4 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2018-07-18 20:50 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Eric Dong, Jiaxin Wu, Ruiyu Ni, Siyuan Fu, Star Zeng
Replace the OpenFileByDevicePath() function with EfiOpenFileByDevicePath()
from UefiLib, correcting the following issues:
- imprecise comments on OpenFileByDevicePath(),
- code duplication between this module and other modules,
- local variable name "EfiSimpleFileSystemProtocol" starting with "Efi"
prefix,
- bogus "FileHandle = NULL" assignments,
- passing a potentially unaligned "FILEPATH_DEVICE_PATH.PathName" field to
a protocol member function (forbidden by the UEFI spec),
- leaking "Handle1" when the device path type/subtype check fails in the
loop,
- stale SHELL_FILE_HANDLE reference in a comment.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf | 1 -
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h | 39 ------
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c | 140 --------------------
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c | 2 +-
4 files changed, 1 insertion(+), 181 deletions(-)
diff --git a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
index cdd2da690411..da00e4a420e7 100644
--- a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
+++ b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
@@ -75,7 +75,6 @@ [Protocols]
gEfiDevicePathProtocolGuid ## PRODUCES
gEfiBlockIoProtocolGuid ## PRODUCES
gEfiBlockIo2ProtocolGuid ## PRODUCES
- gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES
gEfiAcpiTableProtocolGuid ## SOMETIMES_CONSUMES
gEfiAcpiSdtProtocolGuid ## SOMETIMES_CONSUMES
diff --git a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h
index 077bb77b25bf..08a8ca94c9db 100644
--- a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h
+++ b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h
@@ -605,45 +605,6 @@ FileInfo (
);
-/**
- This function will open a file or directory referenced by DevicePath.
-
- This function opens a file with the open mode according to the file path. The
- Attributes is valid only for EFI_FILE_MODE_CREATE.
-
- @param[in, out] FilePath On input, the device path to the file.
- On output, the remaining device path.
- @param[out] FileHandle Pointer to the file handle.
- @param[in] OpenMode The mode to open the file with.
- @param[in] Attributes The file's file attributes.
-
- @retval EFI_SUCCESS The information was set.
- @retval EFI_INVALID_PARAMETER One of the parameters has an invalid value.
- @retval EFI_UNSUPPORTED Could not open the file path.
- @retval EFI_NOT_FOUND The specified file could not be found on the
- device or the file system could not be found
- on the device.
- @retval EFI_NO_MEDIA The device has no medium.
- @retval EFI_MEDIA_CHANGED The device has a different medium in it or
- the medium is no longer supported.
- @retval EFI_DEVICE_ERROR The device reported an error.
- @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
- @retval EFI_WRITE_PROTECTED The file or medium is write protected.
- @retval EFI_ACCESS_DENIED The file was opened read only.
- @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open
- the file.
- @retval EFI_VOLUME_FULL The volume is full.
-**/
-EFI_STATUS
-EFIAPI
-OpenFileByDevicePath(
- IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
- OUT EFI_FILE_HANDLE *FileHandle,
- IN UINT64 OpenMode,
- IN UINT64 Attributes
- );
-
-
/**
Publish the RAM disk NVDIMM Firmware Interface Table (NFIT) to the ACPI
table.
diff --git a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c
index 2cfd4bbf6ce8..95d676fc3939 100644
--- a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c
+++ b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c
@@ -111,143 +111,3 @@ FileInfo (
return Buffer;
}
-
-
-/**
- This function will open a file or directory referenced by DevicePath.
-
- This function opens a file with the open mode according to the file path. The
- Attributes is valid only for EFI_FILE_MODE_CREATE.
-
- @param[in, out] FilePath On input, the device path to the file.
- On output, the remaining device path.
- @param[out] FileHandle Pointer to the file handle.
- @param[in] OpenMode The mode to open the file with.
- @param[in] Attributes The file's file attributes.
-
- @retval EFI_SUCCESS The information was set.
- @retval EFI_INVALID_PARAMETER One of the parameters has an invalid value.
- @retval EFI_UNSUPPORTED Could not open the file path.
- @retval EFI_NOT_FOUND The specified file could not be found on the
- device or the file system could not be found
- on the device.
- @retval EFI_NO_MEDIA The device has no medium.
- @retval EFI_MEDIA_CHANGED The device has a different medium in it or
- the medium is no longer supported.
- @retval EFI_DEVICE_ERROR The device reported an error.
- @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
- @retval EFI_WRITE_PROTECTED The file or medium is write protected.
- @retval EFI_ACCESS_DENIED The file was opened read only.
- @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open
- the file.
- @retval EFI_VOLUME_FULL The volume is full.
-**/
-EFI_STATUS
-EFIAPI
-OpenFileByDevicePath(
- IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
- OUT EFI_FILE_HANDLE *FileHandle,
- IN UINT64 OpenMode,
- IN UINT64 Attributes
- )
-{
- EFI_STATUS Status;
- EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *EfiSimpleFileSystemProtocol;
- EFI_FILE_PROTOCOL *Handle1;
- EFI_FILE_PROTOCOL *Handle2;
- EFI_HANDLE DeviceHandle;
-
- if ((FilePath == NULL || FileHandle == NULL)) {
- return EFI_INVALID_PARAMETER;
- }
-
- Status = gBS->LocateDevicePath (
- &gEfiSimpleFileSystemProtocolGuid,
- FilePath,
- &DeviceHandle
- );
- if (EFI_ERROR (Status)) {
- return Status;
- }
-
- Status = gBS->OpenProtocol(
- DeviceHandle,
- &gEfiSimpleFileSystemProtocolGuid,
- (VOID**)&EfiSimpleFileSystemProtocol,
- gImageHandle,
- NULL,
- EFI_OPEN_PROTOCOL_GET_PROTOCOL
- );
- if (EFI_ERROR (Status)) {
- return Status;
- }
-
- Status = EfiSimpleFileSystemProtocol->OpenVolume(EfiSimpleFileSystemProtocol, &Handle1);
- if (EFI_ERROR (Status)) {
- FileHandle = NULL;
- return Status;
- }
-
- //
- // go down directories one node at a time.
- //
- while (!IsDevicePathEnd (*FilePath)) {
- //
- // For file system access each node should be a file path component
- //
- if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
- DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP
- ) {
- FileHandle = NULL;
- return (EFI_INVALID_PARAMETER);
- }
- //
- // Open this file path node
- //
- Handle2 = Handle1;
- Handle1 = NULL;
-
- //
- // Try to test opening an existing file
- //
- Status = Handle2->Open (
- Handle2,
- &Handle1,
- ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
- OpenMode &~EFI_FILE_MODE_CREATE,
- 0
- );
-
- //
- // see if the error was that it needs to be created
- //
- if ((EFI_ERROR (Status)) && (OpenMode != (OpenMode &~EFI_FILE_MODE_CREATE))) {
- Status = Handle2->Open (
- Handle2,
- &Handle1,
- ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
- OpenMode,
- Attributes
- );
- }
- //
- // Close the last node
- //
- Handle2->Close (Handle2);
-
- if (EFI_ERROR(Status)) {
- return (Status);
- }
-
- //
- // Get the next node
- //
- *FilePath = NextDevicePathNode (*FilePath);
- }
-
- //
- // This is a weak spot since if the undefined SHELL_FILE_HANDLE format changes this must change also!
- //
- *FileHandle = (VOID*)Handle1;
- return EFI_SUCCESS;
-}
diff --git a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c
index 7ebd397fe68a..62933a2d6201 100644
--- a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c
+++ b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c
@@ -656,7 +656,7 @@ RamDiskCallback (
//
// Open the file.
//
- Status = OpenFileByDevicePath (
+ Status = EfiOpenFileByDevicePath (
&FileDevPath,
&FileHandle,
EFI_FILE_MODE_READ,
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 2/6] MdeModulePkg/RamDiskDxe: replace OpenFileByDevicePath() with UefiLib API
2018-07-18 20:50 ` [PATCH 2/6] MdeModulePkg/RamDiskDxe: replace OpenFileByDevicePath() with UefiLib API Laszlo Ersek
@ 2018-07-19 10:36 ` Zeng, Star
2018-07-19 13:20 ` Laszlo Ersek
0 siblings, 1 reply; 27+ messages in thread
From: Zeng, Star @ 2018-07-19 10:36 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Dong, Eric, Wu, Jiaxin, Ni, Ruiyu, Fu, Siyuan, Zeng, Star
Hi Laszlo,
Have you evaluated whether " #include <Protocol/SimpleFileSystem.h> " can be removed from *.h file or not since gEfiSimpleFileSystemProtocolGuid is removed from *.inf?
Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Thursday, July 19, 2018 4:51 AM
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Dong, Eric <eric.dong@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH 2/6] MdeModulePkg/RamDiskDxe: replace OpenFileByDevicePath() with UefiLib API
Replace the OpenFileByDevicePath() function with EfiOpenFileByDevicePath() from UefiLib, correcting the following issues:
- imprecise comments on OpenFileByDevicePath(),
- code duplication between this module and other modules,
- local variable name "EfiSimpleFileSystemProtocol" starting with "Efi"
prefix,
- bogus "FileHandle = NULL" assignments,
- passing a potentially unaligned "FILEPATH_DEVICE_PATH.PathName" field to
a protocol member function (forbidden by the UEFI spec),
- leaking "Handle1" when the device path type/subtype check fails in the
loop,
- stale SHELL_FILE_HANDLE reference in a comment.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf | 1 -
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h | 39 ------
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c | 140 --------------------
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c | 2 +-
4 files changed, 1 insertion(+), 181 deletions(-)
diff --git a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
index cdd2da690411..da00e4a420e7 100644
--- a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
+++ b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
@@ -75,7 +75,6 @@ [Protocols]
gEfiDevicePathProtocolGuid ## PRODUCES
gEfiBlockIoProtocolGuid ## PRODUCES
gEfiBlockIo2ProtocolGuid ## PRODUCES
- gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES
gEfiAcpiTableProtocolGuid ## SOMETIMES_CONSUMES
gEfiAcpiSdtProtocolGuid ## SOMETIMES_CONSUMES
diff --git a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h
index 077bb77b25bf..08a8ca94c9db 100644
--- a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h
+++ b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h
@@ -605,45 +605,6 @@ FileInfo (
);
-/**
- This function will open a file or directory referenced by DevicePath.
-
- This function opens a file with the open mode according to the file path. The
- Attributes is valid only for EFI_FILE_MODE_CREATE.
-
- @param[in, out] FilePath On input, the device path to the file.
- On output, the remaining device path.
- @param[out] FileHandle Pointer to the file handle.
- @param[in] OpenMode The mode to open the file with.
- @param[in] Attributes The file's file attributes.
-
- @retval EFI_SUCCESS The information was set.
- @retval EFI_INVALID_PARAMETER One of the parameters has an invalid value.
- @retval EFI_UNSUPPORTED Could not open the file path.
- @retval EFI_NOT_FOUND The specified file could not be found on the
- device or the file system could not be found
- on the device.
- @retval EFI_NO_MEDIA The device has no medium.
- @retval EFI_MEDIA_CHANGED The device has a different medium in it or
- the medium is no longer supported.
- @retval EFI_DEVICE_ERROR The device reported an error.
- @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
- @retval EFI_WRITE_PROTECTED The file or medium is write protected.
- @retval EFI_ACCESS_DENIED The file was opened read only.
- @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open
- the file.
- @retval EFI_VOLUME_FULL The volume is full.
-**/
-EFI_STATUS
-EFIAPI
-OpenFileByDevicePath(
- IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
- OUT EFI_FILE_HANDLE *FileHandle,
- IN UINT64 OpenMode,
- IN UINT64 Attributes
- );
-
-
/**
Publish the RAM disk NVDIMM Firmware Interface Table (NFIT) to the ACPI
table.
diff --git a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c
index 2cfd4bbf6ce8..95d676fc3939 100644
--- a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c
+++ b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c
@@ -111,143 +111,3 @@ FileInfo (
return Buffer;
}
-
-
-/**
- This function will open a file or directory referenced by DevicePath.
-
- This function opens a file with the open mode according to the file path. The
- Attributes is valid only for EFI_FILE_MODE_CREATE.
-
- @param[in, out] FilePath On input, the device path to the file.
- On output, the remaining device path.
- @param[out] FileHandle Pointer to the file handle.
- @param[in] OpenMode The mode to open the file with.
- @param[in] Attributes The file's file attributes.
-
- @retval EFI_SUCCESS The information was set.
- @retval EFI_INVALID_PARAMETER One of the parameters has an invalid value.
- @retval EFI_UNSUPPORTED Could not open the file path.
- @retval EFI_NOT_FOUND The specified file could not be found on the
- device or the file system could not be found
- on the device.
- @retval EFI_NO_MEDIA The device has no medium.
- @retval EFI_MEDIA_CHANGED The device has a different medium in it or
- the medium is no longer supported.
- @retval EFI_DEVICE_ERROR The device reported an error.
- @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
- @retval EFI_WRITE_PROTECTED The file or medium is write protected.
- @retval EFI_ACCESS_DENIED The file was opened read only.
- @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open
- the file.
- @retval EFI_VOLUME_FULL The volume is full.
-**/
-EFI_STATUS
-EFIAPI
-OpenFileByDevicePath(
- IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
- OUT EFI_FILE_HANDLE *FileHandle,
- IN UINT64 OpenMode,
- IN UINT64 Attributes
- )
-{
- EFI_STATUS Status;
- EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *EfiSimpleFileSystemProtocol;
- EFI_FILE_PROTOCOL *Handle1;
- EFI_FILE_PROTOCOL *Handle2;
- EFI_HANDLE DeviceHandle;
-
- if ((FilePath == NULL || FileHandle == NULL)) {
- return EFI_INVALID_PARAMETER;
- }
-
- Status = gBS->LocateDevicePath (
- &gEfiSimpleFileSystemProtocolGuid,
- FilePath,
- &DeviceHandle
- );
- if (EFI_ERROR (Status)) {
- return Status;
- }
-
- Status = gBS->OpenProtocol(
- DeviceHandle,
- &gEfiSimpleFileSystemProtocolGuid,
- (VOID**)&EfiSimpleFileSystemProtocol,
- gImageHandle,
- NULL,
- EFI_OPEN_PROTOCOL_GET_PROTOCOL
- );
- if (EFI_ERROR (Status)) {
- return Status;
- }
-
- Status = EfiSimpleFileSystemProtocol->OpenVolume(EfiSimpleFileSystemProtocol, &Handle1);
- if (EFI_ERROR (Status)) {
- FileHandle = NULL;
- return Status;
- }
-
- //
- // go down directories one node at a time.
- //
- while (!IsDevicePathEnd (*FilePath)) {
- //
- // For file system access each node should be a file path component
- //
- if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
- DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP
- ) {
- FileHandle = NULL;
- return (EFI_INVALID_PARAMETER);
- }
- //
- // Open this file path node
- //
- Handle2 = Handle1;
- Handle1 = NULL;
-
- //
- // Try to test opening an existing file
- //
- Status = Handle2->Open (
- Handle2,
- &Handle1,
- ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
- OpenMode &~EFI_FILE_MODE_CREATE,
- 0
- );
-
- //
- // see if the error was that it needs to be created
- //
- if ((EFI_ERROR (Status)) && (OpenMode != (OpenMode &~EFI_FILE_MODE_CREATE))) {
- Status = Handle2->Open (
- Handle2,
- &Handle1,
- ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
- OpenMode,
- Attributes
- );
- }
- //
- // Close the last node
- //
- Handle2->Close (Handle2);
-
- if (EFI_ERROR(Status)) {
- return (Status);
- }
-
- //
- // Get the next node
- //
- *FilePath = NextDevicePathNode (*FilePath);
- }
-
- //
- // This is a weak spot since if the undefined SHELL_FILE_HANDLE format changes this must change also!
- //
- *FileHandle = (VOID*)Handle1;
- return EFI_SUCCESS;
-}
diff --git a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c
index 7ebd397fe68a..62933a2d6201 100644
--- a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c
+++ b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c
@@ -656,7 +656,7 @@ RamDiskCallback (
//
// Open the file.
//
- Status = OpenFileByDevicePath (
+ Status = EfiOpenFileByDevicePath (
&FileDevPath,
&FileHandle,
EFI_FILE_MODE_READ,
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 2/6] MdeModulePkg/RamDiskDxe: replace OpenFileByDevicePath() with UefiLib API
2018-07-19 10:36 ` Zeng, Star
@ 2018-07-19 13:20 ` Laszlo Ersek
2018-07-20 10:22 ` Zeng, Star
0 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2018-07-19 13:20 UTC (permalink / raw)
To: Zeng, Star, edk2-devel-01; +Cc: Dong, Eric, Wu, Jiaxin, Ni, Ruiyu, Fu, Siyuan
Hi Star,
On 07/19/18 12:36, Zeng, Star wrote:
> Hi Laszlo,
>
> Have you evaluated whether " #include <Protocol/SimpleFileSystem.h> " can be removed from *.h file or not since gEfiSimpleFileSystemProtocolGuid is removed from *.inf?
Yes, I did check for that.
I did not remove the #include directive of the SimpleFileSystem protocol
header because the header defines a number of structure types, beyond
declaring "gEfiSimpleFileSystemProtocolGuid", and beyond #defining the
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID.
In particular, it defines the (UEFI-standard) type EFI_FILE_PROTOCOL,
and the (non-standard) type EFI_FILE_HANDLE. RamDiskDxe uses the
EFI_FILE_HANDLE type liberally -- after the application of this patch,
four (4) uses of EFI_FILE_HANDLE remain.
Now, in patch #1, I do #include <Protocol/SimpleFileSystem.h> in
UefiLib.h. However, that is because all APIs declared by a lib class
header should be usable at once, without having to hunt down
dependencies manually. This implies that types used in function
prototypes should be declared automatically for the client C file.
Therefore, the #include directive added to UefiLib.h, in patch #1, is
not a substitute for the existent #include directive in RamDiskDxe.
UefiLib.h needs the #include directive for staying self-contained, and
RamDiskDxe needs the #include directive because it uses the
EFI_FILE_HANDLE type for its own purposes.
(Same for the rest of the modules, and same for the device path protocol
header / GUID, wherever appropriate.)
Thanks,
Laszlo
>
> Thanks,
> Star
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, July 19, 2018 4:51 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Dong, Eric <eric.dong@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 2/6] MdeModulePkg/RamDiskDxe: replace OpenFileByDevicePath() with UefiLib API
>
> Replace the OpenFileByDevicePath() function with EfiOpenFileByDevicePath() from UefiLib, correcting the following issues:
>
> - imprecise comments on OpenFileByDevicePath(),
> - code duplication between this module and other modules,
> - local variable name "EfiSimpleFileSystemProtocol" starting with "Efi"
> prefix,
> - bogus "FileHandle = NULL" assignments,
> - passing a potentially unaligned "FILEPATH_DEVICE_PATH.PathName" field to
> a protocol member function (forbidden by the UEFI spec),
> - leaking "Handle1" when the device path type/subtype check fails in the
> loop,
> - stale SHELL_FILE_HANDLE reference in a comment.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf | 1 -
> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h | 39 ------
> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c | 140 --------------------
> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c | 2 +-
> 4 files changed, 1 insertion(+), 181 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
> index cdd2da690411..da00e4a420e7 100644
> --- a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
> +++ b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
> @@ -75,7 +75,6 @@ [Protocols]
> gEfiDevicePathProtocolGuid ## PRODUCES
> gEfiBlockIoProtocolGuid ## PRODUCES
> gEfiBlockIo2ProtocolGuid ## PRODUCES
> - gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES
> gEfiAcpiTableProtocolGuid ## SOMETIMES_CONSUMES
> gEfiAcpiSdtProtocolGuid ## SOMETIMES_CONSUMES
>
> diff --git a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h
> index 077bb77b25bf..08a8ca94c9db 100644
> --- a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h
> +++ b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h
> @@ -605,45 +605,6 @@ FileInfo (
> );
>
>
> -/**
> - This function will open a file or directory referenced by DevicePath.
> -
> - This function opens a file with the open mode according to the file path. The
> - Attributes is valid only for EFI_FILE_MODE_CREATE.
> -
> - @param[in, out] FilePath On input, the device path to the file.
> - On output, the remaining device path.
> - @param[out] FileHandle Pointer to the file handle.
> - @param[in] OpenMode The mode to open the file with.
> - @param[in] Attributes The file's file attributes.
> -
> - @retval EFI_SUCCESS The information was set.
> - @retval EFI_INVALID_PARAMETER One of the parameters has an invalid value.
> - @retval EFI_UNSUPPORTED Could not open the file path.
> - @retval EFI_NOT_FOUND The specified file could not be found on the
> - device or the file system could not be found
> - on the device.
> - @retval EFI_NO_MEDIA The device has no medium.
> - @retval EFI_MEDIA_CHANGED The device has a different medium in it or
> - the medium is no longer supported.
> - @retval EFI_DEVICE_ERROR The device reported an error.
> - @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
> - @retval EFI_WRITE_PROTECTED The file or medium is write protected.
> - @retval EFI_ACCESS_DENIED The file was opened read only.
> - @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open
> - the file.
> - @retval EFI_VOLUME_FULL The volume is full.
> -**/
> -EFI_STATUS
> -EFIAPI
> -OpenFileByDevicePath(
> - IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
> - OUT EFI_FILE_HANDLE *FileHandle,
> - IN UINT64 OpenMode,
> - IN UINT64 Attributes
> - );
> -
> -
> /**
> Publish the RAM disk NVDIMM Firmware Interface Table (NFIT) to the ACPI
> table.
> diff --git a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c
> index 2cfd4bbf6ce8..95d676fc3939 100644
> --- a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c
> +++ b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c
> @@ -111,143 +111,3 @@ FileInfo (
>
> return Buffer;
> }
> -
> -
> -/**
> - This function will open a file or directory referenced by DevicePath.
> -
> - This function opens a file with the open mode according to the file path. The
> - Attributes is valid only for EFI_FILE_MODE_CREATE.
> -
> - @param[in, out] FilePath On input, the device path to the file.
> - On output, the remaining device path.
> - @param[out] FileHandle Pointer to the file handle.
> - @param[in] OpenMode The mode to open the file with.
> - @param[in] Attributes The file's file attributes.
> -
> - @retval EFI_SUCCESS The information was set.
> - @retval EFI_INVALID_PARAMETER One of the parameters has an invalid value.
> - @retval EFI_UNSUPPORTED Could not open the file path.
> - @retval EFI_NOT_FOUND The specified file could not be found on the
> - device or the file system could not be found
> - on the device.
> - @retval EFI_NO_MEDIA The device has no medium.
> - @retval EFI_MEDIA_CHANGED The device has a different medium in it or
> - the medium is no longer supported.
> - @retval EFI_DEVICE_ERROR The device reported an error.
> - @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
> - @retval EFI_WRITE_PROTECTED The file or medium is write protected.
> - @retval EFI_ACCESS_DENIED The file was opened read only.
> - @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open
> - the file.
> - @retval EFI_VOLUME_FULL The volume is full.
> -**/
> -EFI_STATUS
> -EFIAPI
> -OpenFileByDevicePath(
> - IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
> - OUT EFI_FILE_HANDLE *FileHandle,
> - IN UINT64 OpenMode,
> - IN UINT64 Attributes
> - )
> -{
> - EFI_STATUS Status;
> - EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *EfiSimpleFileSystemProtocol;
> - EFI_FILE_PROTOCOL *Handle1;
> - EFI_FILE_PROTOCOL *Handle2;
> - EFI_HANDLE DeviceHandle;
> -
> - if ((FilePath == NULL || FileHandle == NULL)) {
> - return EFI_INVALID_PARAMETER;
> - }
> -
> - Status = gBS->LocateDevicePath (
> - &gEfiSimpleFileSystemProtocolGuid,
> - FilePath,
> - &DeviceHandle
> - );
> - if (EFI_ERROR (Status)) {
> - return Status;
> - }
> -
> - Status = gBS->OpenProtocol(
> - DeviceHandle,
> - &gEfiSimpleFileSystemProtocolGuid,
> - (VOID**)&EfiSimpleFileSystemProtocol,
> - gImageHandle,
> - NULL,
> - EFI_OPEN_PROTOCOL_GET_PROTOCOL
> - );
> - if (EFI_ERROR (Status)) {
> - return Status;
> - }
> -
> - Status = EfiSimpleFileSystemProtocol->OpenVolume(EfiSimpleFileSystemProtocol, &Handle1);
> - if (EFI_ERROR (Status)) {
> - FileHandle = NULL;
> - return Status;
> - }
> -
> - //
> - // go down directories one node at a time.
> - //
> - while (!IsDevicePathEnd (*FilePath)) {
> - //
> - // For file system access each node should be a file path component
> - //
> - if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
> - DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP
> - ) {
> - FileHandle = NULL;
> - return (EFI_INVALID_PARAMETER);
> - }
> - //
> - // Open this file path node
> - //
> - Handle2 = Handle1;
> - Handle1 = NULL;
> -
> - //
> - // Try to test opening an existing file
> - //
> - Status = Handle2->Open (
> - Handle2,
> - &Handle1,
> - ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> - OpenMode &~EFI_FILE_MODE_CREATE,
> - 0
> - );
> -
> - //
> - // see if the error was that it needs to be created
> - //
> - if ((EFI_ERROR (Status)) && (OpenMode != (OpenMode &~EFI_FILE_MODE_CREATE))) {
> - Status = Handle2->Open (
> - Handle2,
> - &Handle1,
> - ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> - OpenMode,
> - Attributes
> - );
> - }
> - //
> - // Close the last node
> - //
> - Handle2->Close (Handle2);
> -
> - if (EFI_ERROR(Status)) {
> - return (Status);
> - }
> -
> - //
> - // Get the next node
> - //
> - *FilePath = NextDevicePathNode (*FilePath);
> - }
> -
> - //
> - // This is a weak spot since if the undefined SHELL_FILE_HANDLE format changes this must change also!
> - //
> - *FileHandle = (VOID*)Handle1;
> - return EFI_SUCCESS;
> -}
> diff --git a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c
> index 7ebd397fe68a..62933a2d6201 100644
> --- a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c
> +++ b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c
> @@ -656,7 +656,7 @@ RamDiskCallback (
> //
> // Open the file.
> //
> - Status = OpenFileByDevicePath (
> + Status = EfiOpenFileByDevicePath (
> &FileDevPath,
> &FileHandle,
> EFI_FILE_MODE_READ,
> --
> 2.14.1.3.gb7cf6e02401b
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2/6] MdeModulePkg/RamDiskDxe: replace OpenFileByDevicePath() with UefiLib API
2018-07-19 13:20 ` Laszlo Ersek
@ 2018-07-20 10:22 ` Zeng, Star
0 siblings, 0 replies; 27+ messages in thread
From: Zeng, Star @ 2018-07-20 10:22 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Dong, Eric, Wu, Jiaxin, Ni, Ruiyu, Fu, Siyuan, Zeng, Star
That is fine, then I am ok with the patch, Reviewed-by: Star Zeng <star.zeng@intel.com>.
Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Thursday, July 19, 2018 9:21 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Dong, Eric <eric.dong@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
Subject: Re: [PATCH 2/6] MdeModulePkg/RamDiskDxe: replace OpenFileByDevicePath() with UefiLib API
Hi Star,
On 07/19/18 12:36, Zeng, Star wrote:
> Hi Laszlo,
>
> Have you evaluated whether " #include <Protocol/SimpleFileSystem.h> " can be removed from *.h file or not since gEfiSimpleFileSystemProtocolGuid is removed from *.inf?
Yes, I did check for that.
I did not remove the #include directive of the SimpleFileSystem protocol header because the header defines a number of structure types, beyond declaring "gEfiSimpleFileSystemProtocolGuid", and beyond #defining the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID.
In particular, it defines the (UEFI-standard) type EFI_FILE_PROTOCOL, and the (non-standard) type EFI_FILE_HANDLE. RamDiskDxe uses the EFI_FILE_HANDLE type liberally -- after the application of this patch, four (4) uses of EFI_FILE_HANDLE remain.
Now, in patch #1, I do #include <Protocol/SimpleFileSystem.h> in UefiLib.h. However, that is because all APIs declared by a lib class header should be usable at once, without having to hunt down dependencies manually. This implies that types used in function prototypes should be declared automatically for the client C file.
Therefore, the #include directive added to UefiLib.h, in patch #1, is not a substitute for the existent #include directive in RamDiskDxe.
UefiLib.h needs the #include directive for staying self-contained, and RamDiskDxe needs the #include directive because it uses the EFI_FILE_HANDLE type for its own purposes.
(Same for the rest of the modules, and same for the device path protocol header / GUID, wherever appropriate.)
Thanks,
Laszlo
>
> Thanks,
> Star
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, July 19, 2018 4:51 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Dong, Eric <eric.dong@intel.com>; Wu, Jiaxin
> <jiaxin.wu@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan
> <siyuan.fu@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 2/6] MdeModulePkg/RamDiskDxe: replace
> OpenFileByDevicePath() with UefiLib API
>
> Replace the OpenFileByDevicePath() function with EfiOpenFileByDevicePath() from UefiLib, correcting the following issues:
>
> - imprecise comments on OpenFileByDevicePath(),
> - code duplication between this module and other modules,
> - local variable name "EfiSimpleFileSystemProtocol" starting with "Efi"
> prefix,
> - bogus "FileHandle = NULL" assignments,
> - passing a potentially unaligned "FILEPATH_DEVICE_PATH.PathName" field to
> a protocol member function (forbidden by the UEFI spec),
> - leaking "Handle1" when the device path type/subtype check fails in the
> loop,
> - stale SHELL_FILE_HANDLE reference in a comment.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf | 1 -
> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h | 39 ------
> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c | 140 --------------------
> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c | 2 +-
> 4 files changed, 1 insertion(+), 181 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
> b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
> index cdd2da690411..da00e4a420e7 100644
> --- a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
> +++ b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
> @@ -75,7 +75,6 @@ [Protocols]
> gEfiDevicePathProtocolGuid ## PRODUCES
> gEfiBlockIoProtocolGuid ## PRODUCES
> gEfiBlockIo2ProtocolGuid ## PRODUCES
> - gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES
> gEfiAcpiTableProtocolGuid ## SOMETIMES_CONSUMES
> gEfiAcpiSdtProtocolGuid ## SOMETIMES_CONSUMES
>
> diff --git a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h
> b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h
> index 077bb77b25bf..08a8ca94c9db 100644
> --- a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h
> +++ b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h
> @@ -605,45 +605,6 @@ FileInfo (
> );
>
>
> -/**
> - This function will open a file or directory referenced by DevicePath.
> -
> - This function opens a file with the open mode according to the file
> path. The
> - Attributes is valid only for EFI_FILE_MODE_CREATE.
> -
> - @param[in, out] FilePath On input, the device path to the file.
> - On output, the remaining device path.
> - @param[out] FileHandle Pointer to the file handle.
> - @param[in] OpenMode The mode to open the file with.
> - @param[in] Attributes The file's file attributes.
> -
> - @retval EFI_SUCCESS The information was set.
> - @retval EFI_INVALID_PARAMETER One of the parameters has an invalid value.
> - @retval EFI_UNSUPPORTED Could not open the file path.
> - @retval EFI_NOT_FOUND The specified file could not be found on the
> - device or the file system could not be found
> - on the device.
> - @retval EFI_NO_MEDIA The device has no medium.
> - @retval EFI_MEDIA_CHANGED The device has a different medium in it or
> - the medium is no longer supported.
> - @retval EFI_DEVICE_ERROR The device reported an error.
> - @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
> - @retval EFI_WRITE_PROTECTED The file or medium is write protected.
> - @retval EFI_ACCESS_DENIED The file was opened read only.
> - @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open
> - the file.
> - @retval EFI_VOLUME_FULL The volume is full.
> -**/
> -EFI_STATUS
> -EFIAPI
> -OpenFileByDevicePath(
> - IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
> - OUT EFI_FILE_HANDLE *FileHandle,
> - IN UINT64 OpenMode,
> - IN UINT64 Attributes
> - );
> -
> -
> /**
> Publish the RAM disk NVDIMM Firmware Interface Table (NFIT) to the ACPI
> table.
> diff --git
> a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c
> b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c
> index 2cfd4bbf6ce8..95d676fc3939 100644
> --- a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c
> +++ b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c
> @@ -111,143 +111,3 @@ FileInfo (
>
> return Buffer;
> }
> -
> -
> -/**
> - This function will open a file or directory referenced by DevicePath.
> -
> - This function opens a file with the open mode according to the file
> path. The
> - Attributes is valid only for EFI_FILE_MODE_CREATE.
> -
> - @param[in, out] FilePath On input, the device path to the file.
> - On output, the remaining device path.
> - @param[out] FileHandle Pointer to the file handle.
> - @param[in] OpenMode The mode to open the file with.
> - @param[in] Attributes The file's file attributes.
> -
> - @retval EFI_SUCCESS The information was set.
> - @retval EFI_INVALID_PARAMETER One of the parameters has an invalid value.
> - @retval EFI_UNSUPPORTED Could not open the file path.
> - @retval EFI_NOT_FOUND The specified file could not be found on the
> - device or the file system could not be found
> - on the device.
> - @retval EFI_NO_MEDIA The device has no medium.
> - @retval EFI_MEDIA_CHANGED The device has a different medium in it or
> - the medium is no longer supported.
> - @retval EFI_DEVICE_ERROR The device reported an error.
> - @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
> - @retval EFI_WRITE_PROTECTED The file or medium is write protected.
> - @retval EFI_ACCESS_DENIED The file was opened read only.
> - @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open
> - the file.
> - @retval EFI_VOLUME_FULL The volume is full.
> -**/
> -EFI_STATUS
> -EFIAPI
> -OpenFileByDevicePath(
> - IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
> - OUT EFI_FILE_HANDLE *FileHandle,
> - IN UINT64 OpenMode,
> - IN UINT64 Attributes
> - )
> -{
> - EFI_STATUS Status;
> - EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *EfiSimpleFileSystemProtocol;
> - EFI_FILE_PROTOCOL *Handle1;
> - EFI_FILE_PROTOCOL *Handle2;
> - EFI_HANDLE DeviceHandle;
> -
> - if ((FilePath == NULL || FileHandle == NULL)) {
> - return EFI_INVALID_PARAMETER;
> - }
> -
> - Status = gBS->LocateDevicePath (
> - &gEfiSimpleFileSystemProtocolGuid,
> - FilePath,
> - &DeviceHandle
> - );
> - if (EFI_ERROR (Status)) {
> - return Status;
> - }
> -
> - Status = gBS->OpenProtocol(
> - DeviceHandle,
> - &gEfiSimpleFileSystemProtocolGuid,
> - (VOID**)&EfiSimpleFileSystemProtocol,
> - gImageHandle,
> - NULL,
> - EFI_OPEN_PROTOCOL_GET_PROTOCOL
> - );
> - if (EFI_ERROR (Status)) {
> - return Status;
> - }
> -
> - Status =
> EfiSimpleFileSystemProtocol->OpenVolume(EfiSimpleFileSystemProtocol,
> &Handle1);
> - if (EFI_ERROR (Status)) {
> - FileHandle = NULL;
> - return Status;
> - }
> -
> - //
> - // go down directories one node at a time.
> - //
> - while (!IsDevicePathEnd (*FilePath)) {
> - //
> - // For file system access each node should be a file path component
> - //
> - if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
> - DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP
> - ) {
> - FileHandle = NULL;
> - return (EFI_INVALID_PARAMETER);
> - }
> - //
> - // Open this file path node
> - //
> - Handle2 = Handle1;
> - Handle1 = NULL;
> -
> - //
> - // Try to test opening an existing file
> - //
> - Status = Handle2->Open (
> - Handle2,
> - &Handle1,
> - ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> - OpenMode &~EFI_FILE_MODE_CREATE,
> - 0
> - );
> -
> - //
> - // see if the error was that it needs to be created
> - //
> - if ((EFI_ERROR (Status)) && (OpenMode != (OpenMode &~EFI_FILE_MODE_CREATE))) {
> - Status = Handle2->Open (
> - Handle2,
> - &Handle1,
> - ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> - OpenMode,
> - Attributes
> - );
> - }
> - //
> - // Close the last node
> - //
> - Handle2->Close (Handle2);
> -
> - if (EFI_ERROR(Status)) {
> - return (Status);
> - }
> -
> - //
> - // Get the next node
> - //
> - *FilePath = NextDevicePathNode (*FilePath);
> - }
> -
> - //
> - // This is a weak spot since if the undefined SHELL_FILE_HANDLE format changes this must change also!
> - //
> - *FileHandle = (VOID*)Handle1;
> - return EFI_SUCCESS;
> -}
> diff --git a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c
> b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c
> index 7ebd397fe68a..62933a2d6201 100644
> --- a/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c
> +++ b/MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c
> @@ -656,7 +656,7 @@ RamDiskCallback (
> //
> // Open the file.
> //
> - Status = OpenFileByDevicePath (
> + Status = EfiOpenFileByDevicePath (
> &FileDevPath,
> &FileHandle,
> EFI_FILE_MODE_READ,
> --
> 2.14.1.3.gb7cf6e02401b
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 3/6] NetworkPkg/TlsAuthConfigDxe: replace OpenFileByDevicePath() with UefiLib API
2018-07-18 20:50 [PATCH 0/6] UefiLib: centralize OpenFileByDevicePath() and fix its bugs Laszlo Ersek
2018-07-18 20:50 ` [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath() Laszlo Ersek
2018-07-18 20:50 ` [PATCH 2/6] MdeModulePkg/RamDiskDxe: replace OpenFileByDevicePath() with UefiLib API Laszlo Ersek
@ 2018-07-18 20:50 ` Laszlo Ersek
2018-07-24 17:20 ` Laszlo Ersek
2018-07-25 0:30 ` Wu, Jiaxin
2018-07-18 20:50 ` [PATCH 4/6] SecurityPkg/SecureBootConfigDxe: " Laszlo Ersek
` (3 subsequent siblings)
6 siblings, 2 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-07-18 20:50 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jiaxin Wu, Siyuan Fu
Replace the OpenFileByDevicePath() function with EfiOpenFileByDevicePath()
from UefiLib, correcting the following issues:
- imprecise comments on OpenFileByDevicePath(),
- code duplication between this module and other modules,
- local variable name "EfiSimpleFileSystemProtocol" starting with "Efi"
prefix,
- bogus "FileHandle = NULL" assignments,
- passing a potentially unaligned "FILEPATH_DEVICE_PATH.PathName" field to
a protocol member function (forbidden by the UEFI spec),
- leaking "Handle1" when the device path type/subtype check fails in the
loop,
- stale SHELL_FILE_HANDLE reference in a comment.
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf | 1 -
NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c | 141 +-------------------
2 files changed, 1 insertion(+), 141 deletions(-)
diff --git a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
index 3cfcdb9983f1..e5face7b4de5 100644
--- a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
+++ b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
@@ -57,7 +57,6 @@ [LibraryClasses]
[Protocols]
gEfiDevicePathProtocolGuid ## PRODUCES
gEfiHiiConfigAccessProtocolGuid ## PRODUCES
- gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES
[Guids]
gTlsAuthConfigGuid ## PRODUCES ## GUID
diff --git a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c
index 31450b3e4a1b..7259c5e82f61 100644
--- a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c
+++ b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c
@@ -574,145 +574,6 @@ ON_EXIT:
return Status;
}
-/**
- This function will open a file or directory referenced by DevicePath.
-
- This function opens a file with the open mode according to the file path. The
- Attributes is valid only for EFI_FILE_MODE_CREATE.
-
- @param[in, out] FilePath On input, the device path to the file.
- On output, the remaining device path.
- @param[out] FileHandle Pointer to the file handle.
- @param[in] OpenMode The mode to open the file with.
- @param[in] Attributes The file's file attributes.
-
- @retval EFI_SUCCESS The information was set.
- @retval EFI_INVALID_PARAMETER One of the parameters has an invalid value.
- @retval EFI_UNSUPPORTED Could not open the file path.
- @retval EFI_NOT_FOUND The specified file could not be found on the
- device or the file system could not be found on
- the device.
- @retval EFI_NO_MEDIA The device has no medium.
- @retval EFI_MEDIA_CHANGED The device has a different medium in it or the
- medium is no longer supported.
- @retval EFI_DEVICE_ERROR The device reported an error.
- @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
- @retval EFI_WRITE_PROTECTED The file or medium is write protected.
- @retval EFI_ACCESS_DENIED The file was opened read only.
- @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open the
- file.
- @retval EFI_VOLUME_FULL The volume is full.
-**/
-EFI_STATUS
-EFIAPI
-OpenFileByDevicePath (
- IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
- OUT EFI_FILE_HANDLE *FileHandle,
- IN UINT64 OpenMode,
- IN UINT64 Attributes
- )
-{
- EFI_STATUS Status;
- EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *EfiSimpleFileSystemProtocol;
- EFI_FILE_PROTOCOL *Handle1;
- EFI_FILE_PROTOCOL *Handle2;
- EFI_HANDLE DeviceHandle;
-
- if ((FilePath == NULL || FileHandle == NULL)) {
- return EFI_INVALID_PARAMETER;
- }
-
- Status = gBS->LocateDevicePath (
- &gEfiSimpleFileSystemProtocolGuid,
- FilePath,
- &DeviceHandle
- );
- if (EFI_ERROR (Status)) {
- return Status;
- }
-
- Status = gBS->OpenProtocol(
- DeviceHandle,
- &gEfiSimpleFileSystemProtocolGuid,
- (VOID**)&EfiSimpleFileSystemProtocol,
- gImageHandle,
- NULL,
- EFI_OPEN_PROTOCOL_GET_PROTOCOL
- );
- if (EFI_ERROR (Status)) {
- return Status;
- }
-
- Status = EfiSimpleFileSystemProtocol->OpenVolume(EfiSimpleFileSystemProtocol, &Handle1);
- if (EFI_ERROR (Status)) {
- FileHandle = NULL;
- return Status;
- }
-
- //
- // go down directories one node at a time.
- //
- while (!IsDevicePathEnd (*FilePath)) {
- //
- // For file system access each node should be a file path component
- //
- if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
- DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP
- ) {
- FileHandle = NULL;
- return (EFI_INVALID_PARAMETER);
- }
- //
- // Open this file path node
- //
- Handle2 = Handle1;
- Handle1 = NULL;
-
- //
- // Try to test opening an existing file
- //
- Status = Handle2->Open (
- Handle2,
- &Handle1,
- ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
- OpenMode &~EFI_FILE_MODE_CREATE,
- 0
- );
-
- //
- // see if the error was that it needs to be created
- //
- if ((EFI_ERROR (Status)) && (OpenMode != (OpenMode &~EFI_FILE_MODE_CREATE))) {
- Status = Handle2->Open (
- Handle2,
- &Handle1,
- ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
- OpenMode,
- Attributes
- );
- }
- //
- // Close the last node
- //
- Handle2->Close (Handle2);
-
- if (EFI_ERROR(Status)) {
- return (Status);
- }
-
- //
- // Get the next node
- //
- *FilePath = NextDevicePathNode (*FilePath);
- }
-
- //
- // This is a weak spot since if the undefined SHELL_FILE_HANDLE format changes this must change also!
- //
- *FileHandle = (VOID*)Handle1;
- return EFI_SUCCESS;
-}
-
/**
This function converts an input device structure to a Unicode string.
@@ -1039,7 +900,7 @@ UpdatePage(
mTlsAuthPrivateData->FileContext->FileName = FileName;
- OpenFileByDevicePath (
+ EfiOpenFileByDevicePath (
&FilePath,
&mTlsAuthPrivateData->FileContext->FHandle,
EFI_FILE_MODE_READ,
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 3/6] NetworkPkg/TlsAuthConfigDxe: replace OpenFileByDevicePath() with UefiLib API
2018-07-18 20:50 ` [PATCH 3/6] NetworkPkg/TlsAuthConfigDxe: " Laszlo Ersek
@ 2018-07-24 17:20 ` Laszlo Ersek
2018-07-25 0:30 ` Wu, Jiaxin
1 sibling, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-07-24 17:20 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Siyuan Fu, Jiaxin Wu
On 07/18/18 22:50, Laszlo Ersek wrote:
> Replace the OpenFileByDevicePath() function with EfiOpenFileByDevicePath()
> from UefiLib, correcting the following issues:
>
> - imprecise comments on OpenFileByDevicePath(),
> - code duplication between this module and other modules,
> - local variable name "EfiSimpleFileSystemProtocol" starting with "Efi"
> prefix,
> - bogus "FileHandle = NULL" assignments,
> - passing a potentially unaligned "FILEPATH_DEVICE_PATH.PathName" field to
> a protocol member function (forbidden by the UEFI spec),
> - leaking "Handle1" when the device path type/subtype check fails in the
> loop,
> - stale SHELL_FILE_HANDLE reference in a comment.
>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf | 1 -
> NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c | 141 +-------------------
> 2 files changed, 1 insertion(+), 141 deletions(-)
Jiaxin, Siyuan, do you guys have comments on this patch?
Thanks,
Laszlo
>
> diff --git a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
> index 3cfcdb9983f1..e5face7b4de5 100644
> --- a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
> +++ b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
> @@ -57,7 +57,6 @@ [LibraryClasses]
> [Protocols]
> gEfiDevicePathProtocolGuid ## PRODUCES
> gEfiHiiConfigAccessProtocolGuid ## PRODUCES
> - gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES
>
> [Guids]
> gTlsAuthConfigGuid ## PRODUCES ## GUID
> diff --git a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c
> index 31450b3e4a1b..7259c5e82f61 100644
> --- a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c
> +++ b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c
> @@ -574,145 +574,6 @@ ON_EXIT:
> return Status;
> }
>
> -/**
> - This function will open a file or directory referenced by DevicePath.
> -
> - This function opens a file with the open mode according to the file path. The
> - Attributes is valid only for EFI_FILE_MODE_CREATE.
> -
> - @param[in, out] FilePath On input, the device path to the file.
> - On output, the remaining device path.
> - @param[out] FileHandle Pointer to the file handle.
> - @param[in] OpenMode The mode to open the file with.
> - @param[in] Attributes The file's file attributes.
> -
> - @retval EFI_SUCCESS The information was set.
> - @retval EFI_INVALID_PARAMETER One of the parameters has an invalid value.
> - @retval EFI_UNSUPPORTED Could not open the file path.
> - @retval EFI_NOT_FOUND The specified file could not be found on the
> - device or the file system could not be found on
> - the device.
> - @retval EFI_NO_MEDIA The device has no medium.
> - @retval EFI_MEDIA_CHANGED The device has a different medium in it or the
> - medium is no longer supported.
> - @retval EFI_DEVICE_ERROR The device reported an error.
> - @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
> - @retval EFI_WRITE_PROTECTED The file or medium is write protected.
> - @retval EFI_ACCESS_DENIED The file was opened read only.
> - @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open the
> - file.
> - @retval EFI_VOLUME_FULL The volume is full.
> -**/
> -EFI_STATUS
> -EFIAPI
> -OpenFileByDevicePath (
> - IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
> - OUT EFI_FILE_HANDLE *FileHandle,
> - IN UINT64 OpenMode,
> - IN UINT64 Attributes
> - )
> -{
> - EFI_STATUS Status;
> - EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *EfiSimpleFileSystemProtocol;
> - EFI_FILE_PROTOCOL *Handle1;
> - EFI_FILE_PROTOCOL *Handle2;
> - EFI_HANDLE DeviceHandle;
> -
> - if ((FilePath == NULL || FileHandle == NULL)) {
> - return EFI_INVALID_PARAMETER;
> - }
> -
> - Status = gBS->LocateDevicePath (
> - &gEfiSimpleFileSystemProtocolGuid,
> - FilePath,
> - &DeviceHandle
> - );
> - if (EFI_ERROR (Status)) {
> - return Status;
> - }
> -
> - Status = gBS->OpenProtocol(
> - DeviceHandle,
> - &gEfiSimpleFileSystemProtocolGuid,
> - (VOID**)&EfiSimpleFileSystemProtocol,
> - gImageHandle,
> - NULL,
> - EFI_OPEN_PROTOCOL_GET_PROTOCOL
> - );
> - if (EFI_ERROR (Status)) {
> - return Status;
> - }
> -
> - Status = EfiSimpleFileSystemProtocol->OpenVolume(EfiSimpleFileSystemProtocol, &Handle1);
> - if (EFI_ERROR (Status)) {
> - FileHandle = NULL;
> - return Status;
> - }
> -
> - //
> - // go down directories one node at a time.
> - //
> - while (!IsDevicePathEnd (*FilePath)) {
> - //
> - // For file system access each node should be a file path component
> - //
> - if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
> - DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP
> - ) {
> - FileHandle = NULL;
> - return (EFI_INVALID_PARAMETER);
> - }
> - //
> - // Open this file path node
> - //
> - Handle2 = Handle1;
> - Handle1 = NULL;
> -
> - //
> - // Try to test opening an existing file
> - //
> - Status = Handle2->Open (
> - Handle2,
> - &Handle1,
> - ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> - OpenMode &~EFI_FILE_MODE_CREATE,
> - 0
> - );
> -
> - //
> - // see if the error was that it needs to be created
> - //
> - if ((EFI_ERROR (Status)) && (OpenMode != (OpenMode &~EFI_FILE_MODE_CREATE))) {
> - Status = Handle2->Open (
> - Handle2,
> - &Handle1,
> - ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> - OpenMode,
> - Attributes
> - );
> - }
> - //
> - // Close the last node
> - //
> - Handle2->Close (Handle2);
> -
> - if (EFI_ERROR(Status)) {
> - return (Status);
> - }
> -
> - //
> - // Get the next node
> - //
> - *FilePath = NextDevicePathNode (*FilePath);
> - }
> -
> - //
> - // This is a weak spot since if the undefined SHELL_FILE_HANDLE format changes this must change also!
> - //
> - *FileHandle = (VOID*)Handle1;
> - return EFI_SUCCESS;
> -}
> -
> /**
> This function converts an input device structure to a Unicode string.
>
> @@ -1039,7 +900,7 @@ UpdatePage(
>
> mTlsAuthPrivateData->FileContext->FileName = FileName;
>
> - OpenFileByDevicePath (
> + EfiOpenFileByDevicePath (
> &FilePath,
> &mTlsAuthPrivateData->FileContext->FHandle,
> EFI_FILE_MODE_READ,
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 3/6] NetworkPkg/TlsAuthConfigDxe: replace OpenFileByDevicePath() with UefiLib API
2018-07-18 20:50 ` [PATCH 3/6] NetworkPkg/TlsAuthConfigDxe: " Laszlo Ersek
2018-07-24 17:20 ` Laszlo Ersek
@ 2018-07-25 0:30 ` Wu, Jiaxin
1 sibling, 0 replies; 27+ messages in thread
From: Wu, Jiaxin @ 2018-07-25 0:30 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01; +Cc: Fu, Siyuan
Looks good with me.
Reviewed-by: Jiaxin Wu <jiaxin.wu@intel.com>
Thanks,
Jiaxin
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, July 19, 2018 4:51 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: [PATCH 3/6] NetworkPkg/TlsAuthConfigDxe: replace
> OpenFileByDevicePath() with UefiLib API
>
> Replace the OpenFileByDevicePath() function with EfiOpenFileByDevicePath()
> from UefiLib, correcting the following issues:
>
> - imprecise comments on OpenFileByDevicePath(),
> - code duplication between this module and other modules,
> - local variable name "EfiSimpleFileSystemProtocol" starting with "Efi"
> prefix,
> - bogus "FileHandle = NULL" assignments,
> - passing a potentially unaligned "FILEPATH_DEVICE_PATH.PathName" field
> to
> a protocol member function (forbidden by the UEFI spec),
> - leaking "Handle1" when the device path type/subtype check fails in the
> loop,
> - stale SHELL_FILE_HANDLE reference in a comment.
>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf | 1 -
> NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c | 141 +-------------------
> 2 files changed, 1 insertion(+), 141 deletions(-)
>
> diff --git a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
> b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
> index 3cfcdb9983f1..e5face7b4de5 100644
> --- a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
> +++ b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
> @@ -57,7 +57,6 @@ [LibraryClasses]
> [Protocols]
> gEfiDevicePathProtocolGuid ## PRODUCES
> gEfiHiiConfigAccessProtocolGuid ## PRODUCES
> - gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES
>
> [Guids]
> gTlsAuthConfigGuid ## PRODUCES ## GUID
> diff --git a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c
> b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c
> index 31450b3e4a1b..7259c5e82f61 100644
> --- a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c
> +++ b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c
> @@ -574,145 +574,6 @@ ON_EXIT:
> return Status;
> }
>
> -/**
> - This function will open a file or directory referenced by DevicePath.
> -
> - This function opens a file with the open mode according to the file path.
> The
> - Attributes is valid only for EFI_FILE_MODE_CREATE.
> -
> - @param[in, out] FilePath On input, the device path to the file.
> - On output, the remaining device path.
> - @param[out] FileHandle Pointer to the file handle.
> - @param[in] OpenMode The mode to open the file with.
> - @param[in] Attributes The file's file attributes.
> -
> - @retval EFI_SUCCESS The information was set.
> - @retval EFI_INVALID_PARAMETER One of the parameters has an invalid
> value.
> - @retval EFI_UNSUPPORTED Could not open the file path.
> - @retval EFI_NOT_FOUND The specified file could not be found on the
> - device or the file system could not be found on
> - the device.
> - @retval EFI_NO_MEDIA The device has no medium.
> - @retval EFI_MEDIA_CHANGED The device has a different medium in it
> or the
> - medium is no longer supported.
> - @retval EFI_DEVICE_ERROR The device reported an error.
> - @retval EFI_VOLUME_CORRUPTED The file system structures are
> corrupted.
> - @retval EFI_WRITE_PROTECTED The file or medium is write protected.
> - @retval EFI_ACCESS_DENIED The file was opened read only.
> - @retval EFI_OUT_OF_RESOURCES Not enough resources were available
> to open the
> - file.
> - @retval EFI_VOLUME_FULL The volume is full.
> -**/
> -EFI_STATUS
> -EFIAPI
> -OpenFileByDevicePath (
> - IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
> - OUT EFI_FILE_HANDLE *FileHandle,
> - IN UINT64 OpenMode,
> - IN UINT64 Attributes
> - )
> -{
> - EFI_STATUS Status;
> - EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *EfiSimpleFileSystemProtocol;
> - EFI_FILE_PROTOCOL *Handle1;
> - EFI_FILE_PROTOCOL *Handle2;
> - EFI_HANDLE DeviceHandle;
> -
> - if ((FilePath == NULL || FileHandle == NULL)) {
> - return EFI_INVALID_PARAMETER;
> - }
> -
> - Status = gBS->LocateDevicePath (
> - &gEfiSimpleFileSystemProtocolGuid,
> - FilePath,
> - &DeviceHandle
> - );
> - if (EFI_ERROR (Status)) {
> - return Status;
> - }
> -
> - Status = gBS->OpenProtocol(
> - DeviceHandle,
> - &gEfiSimpleFileSystemProtocolGuid,
> - (VOID**)&EfiSimpleFileSystemProtocol,
> - gImageHandle,
> - NULL,
> - EFI_OPEN_PROTOCOL_GET_PROTOCOL
> - );
> - if (EFI_ERROR (Status)) {
> - return Status;
> - }
> -
> - Status = EfiSimpleFileSystemProtocol-
> >OpenVolume(EfiSimpleFileSystemProtocol, &Handle1);
> - if (EFI_ERROR (Status)) {
> - FileHandle = NULL;
> - return Status;
> - }
> -
> - //
> - // go down directories one node at a time.
> - //
> - while (!IsDevicePathEnd (*FilePath)) {
> - //
> - // For file system access each node should be a file path component
> - //
> - if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
> - DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP
> - ) {
> - FileHandle = NULL;
> - return (EFI_INVALID_PARAMETER);
> - }
> - //
> - // Open this file path node
> - //
> - Handle2 = Handle1;
> - Handle1 = NULL;
> -
> - //
> - // Try to test opening an existing file
> - //
> - Status = Handle2->Open (
> - Handle2,
> - &Handle1,
> - ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> - OpenMode &~EFI_FILE_MODE_CREATE,
> - 0
> - );
> -
> - //
> - // see if the error was that it needs to be created
> - //
> - if ((EFI_ERROR (Status)) && (OpenMode != (OpenMode
> &~EFI_FILE_MODE_CREATE))) {
> - Status = Handle2->Open (
> - Handle2,
> - &Handle1,
> - ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
> - OpenMode,
> - Attributes
> - );
> - }
> - //
> - // Close the last node
> - //
> - Handle2->Close (Handle2);
> -
> - if (EFI_ERROR(Status)) {
> - return (Status);
> - }
> -
> - //
> - // Get the next node
> - //
> - *FilePath = NextDevicePathNode (*FilePath);
> - }
> -
> - //
> - // This is a weak spot since if the undefined SHELL_FILE_HANDLE format
> changes this must change also!
> - //
> - *FileHandle = (VOID*)Handle1;
> - return EFI_SUCCESS;
> -}
> -
> /**
> This function converts an input device structure to a Unicode string.
>
> @@ -1039,7 +900,7 @@ UpdatePage(
>
> mTlsAuthPrivateData->FileContext->FileName = FileName;
>
> - OpenFileByDevicePath (
> + EfiOpenFileByDevicePath (
> &FilePath,
> &mTlsAuthPrivateData->FileContext->FHandle,
> EFI_FILE_MODE_READ,
> --
> 2.14.1.3.gb7cf6e02401b
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 4/6] SecurityPkg/SecureBootConfigDxe: replace OpenFileByDevicePath() with UefiLib API
2018-07-18 20:50 [PATCH 0/6] UefiLib: centralize OpenFileByDevicePath() and fix its bugs Laszlo Ersek
` (2 preceding siblings ...)
2018-07-18 20:50 ` [PATCH 3/6] NetworkPkg/TlsAuthConfigDxe: " Laszlo Ersek
@ 2018-07-18 20:50 ` Laszlo Ersek
2018-07-24 5:09 ` Zhang, Chao B
2018-07-18 20:50 ` [PATCH 5/6] ShellPkg/UefiShellLib: drop DeviceHandle param of ShellOpenFileByDevicePath() Laszlo Ersek
` (2 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2018-07-18 20:50 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Chao Zhang, Jiewen Yao, Roman Bacik
Replace the OpenFileByDevicePath() function with EfiOpenFileByDevicePath()
from UefiLib, correcting the following issues:
- imprecise comments on OpenFileByDevicePath(),
- code duplication between this module and other modules,
- local variable name "EfiSimpleFileSystemProtocol" starting with "Efi"
prefix,
- bogus "FileHandle = NULL" assignments,
- leaking "Handle1" when the device path type/subtype check or the
realignment-motivated AllocateCopyPool() fails in the loop,
- stale SHELL_FILE_HANDLE reference in a comment.
Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Roman Bacik <roman.bacik@broadcom.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf | 1 -
SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c | 151 +-------------------
2 files changed, 1 insertion(+), 151 deletions(-)
diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
index 487fc8cda917..caf95ddac7d9 100644
--- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
+++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
@@ -114,7 +114,6 @@ [Guids]
[Protocols]
gEfiHiiConfigAccessProtocolGuid ## PRODUCES
gEfiDevicePathProtocolGuid ## PRODUCES
- gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES
gEfiBlockIoProtocolGuid ## SOMETIMES_CONSUMES
[Depex]
diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
index 2a26c20f394c..312a92d7461a 100644
--- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
+++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
@@ -80,155 +80,6 @@ CleanUpPage (
);
}
-/**
- This function will open a file or directory referenced by DevicePath.
-
- This function opens a file with the open mode according to the file path. The
- Attributes is valid only for EFI_FILE_MODE_CREATE.
-
- @param[in, out] FilePath On input, the device path to the file.
- On output, the remaining device path.
- @param[out] FileHandle Pointer to the file handle.
- @param[in] OpenMode The mode to open the file with.
- @param[in] Attributes The file's file attributes.
-
- @retval EFI_SUCCESS The information was set.
- @retval EFI_INVALID_PARAMETER One of the parameters has an invalid value.
- @retval EFI_UNSUPPORTED Could not open the file path.
- @retval EFI_NOT_FOUND The specified file could not be found on the
- device or the file system could not be found on
- the device.
- @retval EFI_NO_MEDIA The device has no medium.
- @retval EFI_MEDIA_CHANGED The device has a different medium in it or the
- medium is no longer supported.
- @retval EFI_DEVICE_ERROR The device reported an error.
- @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
- @retval EFI_WRITE_PROTECTED The file or medium is write protected.
- @retval EFI_ACCESS_DENIED The file was opened read only.
- @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open the
- file.
- @retval EFI_VOLUME_FULL The volume is full.
-**/
-EFI_STATUS
-EFIAPI
-OpenFileByDevicePath(
- IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
- OUT EFI_FILE_HANDLE *FileHandle,
- IN UINT64 OpenMode,
- IN UINT64 Attributes
- )
-{
- EFI_STATUS Status;
- EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *EfiSimpleFileSystemProtocol;
- EFI_FILE_PROTOCOL *Handle1;
- EFI_FILE_PROTOCOL *Handle2;
- EFI_HANDLE DeviceHandle;
- CHAR16 *PathName;
- UINTN PathLength;
-
- if ((FilePath == NULL || FileHandle == NULL)) {
- return EFI_INVALID_PARAMETER;
- }
-
- Status = gBS->LocateDevicePath (
- &gEfiSimpleFileSystemProtocolGuid,
- FilePath,
- &DeviceHandle
- );
- if (EFI_ERROR (Status)) {
- return Status;
- }
-
- Status = gBS->OpenProtocol(
- DeviceHandle,
- &gEfiSimpleFileSystemProtocolGuid,
- (VOID**)&EfiSimpleFileSystemProtocol,
- gImageHandle,
- NULL,
- EFI_OPEN_PROTOCOL_GET_PROTOCOL
- );
- if (EFI_ERROR (Status)) {
- return Status;
- }
-
- Status = EfiSimpleFileSystemProtocol->OpenVolume(EfiSimpleFileSystemProtocol, &Handle1);
- if (EFI_ERROR (Status)) {
- FileHandle = NULL;
- return Status;
- }
-
- //
- // go down directories one node at a time.
- //
- while (!IsDevicePathEnd (*FilePath)) {
- //
- // For file system access each node should be a file path component
- //
- if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
- DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP
- ) {
- FileHandle = NULL;
- return (EFI_INVALID_PARAMETER);
- }
- //
- // Open this file path node
- //
- Handle2 = Handle1;
- Handle1 = NULL;
- PathLength = DevicePathNodeLength (*FilePath) - sizeof (EFI_DEVICE_PATH_PROTOCOL);
- PathName = AllocateCopyPool (PathLength, ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName);
- if (PathName == NULL) {
- return EFI_OUT_OF_RESOURCES;
- }
-
- //
- // Try to test opening an existing file
- //
- Status = Handle2->Open (
- Handle2,
- &Handle1,
- PathName,
- OpenMode &~EFI_FILE_MODE_CREATE,
- 0
- );
-
- //
- // see if the error was that it needs to be created
- //
- if ((EFI_ERROR (Status)) && (OpenMode != (OpenMode &~EFI_FILE_MODE_CREATE))) {
- Status = Handle2->Open (
- Handle2,
- &Handle1,
- PathName,
- OpenMode,
- Attributes
- );
- }
- //
- // Close the last node
- //
- Handle2->Close (Handle2);
-
- FreePool (PathName);
-
- if (EFI_ERROR(Status)) {
- return (Status);
- }
-
- //
- // Get the next node
- //
- *FilePath = NextDevicePathNode (*FilePath);
- }
-
- //
- // This is a weak spot since if the undefined SHELL_FILE_HANDLE format changes this must change also!
- //
- *FileHandle = (VOID*)Handle1;
- return EFI_SUCCESS;
-}
-
-
/**
Extract filename from device path. The returned buffer is allocated using AllocateCopyPool.
The caller is responsible for freeing the allocated buffer using FreePool(). If return NULL
@@ -312,7 +163,7 @@ UpdatePage(
gSecureBootPrivateData->FileContext->FileName = FileName;
- OpenFileByDevicePath(
+ EfiOpenFileByDevicePath(
&FilePath,
&gSecureBootPrivateData->FileContext->FHandle,
EFI_FILE_MODE_READ,
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 4/6] SecurityPkg/SecureBootConfigDxe: replace OpenFileByDevicePath() with UefiLib API
2018-07-18 20:50 ` [PATCH 4/6] SecurityPkg/SecureBootConfigDxe: " Laszlo Ersek
@ 2018-07-24 5:09 ` Zhang, Chao B
0 siblings, 0 replies; 27+ messages in thread
From: Zhang, Chao B @ 2018-07-24 5:09 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01; +Cc: Yao, Jiewen, Roman Bacik
Reviewed-by： Chao Zhang<chao.b.zhang@intel.com>
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Thursday, July 19, 2018 4:51 AM
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Roman Bacik <roman.bacik@broadcom.com>
Subject: [PATCH 4/6] SecurityPkg/SecureBootConfigDxe: replace OpenFileByDevicePath() with UefiLib API
Replace the OpenFileByDevicePath() function with EfiOpenFileByDevicePath() from UefiLib, correcting the following issues:
- imprecise comments on OpenFileByDevicePath(),
- code duplication between this module and other modules,
- local variable name "EfiSimpleFileSystemProtocol" starting with "Efi"
prefix,
- bogus "FileHandle = NULL" assignments,
- leaking "Handle1" when the device path type/subtype check or the
realignment-motivated AllocateCopyPool() fails in the loop,
- stale SHELL_FILE_HANDLE reference in a comment.
Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Roman Bacik <roman.bacik@broadcom.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf | 1 -
SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c | 151 +-------------------
2 files changed, 1 insertion(+), 151 deletions(-)
diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
index 487fc8cda917..caf95ddac7d9 100644
--- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
+++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCo
+++ nfigDxe.inf
@@ -114,7 +114,6 @@ [Guids]
[Protocols]
gEfiHiiConfigAccessProtocolGuid ## PRODUCES
gEfiDevicePathProtocolGuid ## PRODUCES
- gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES
gEfiBlockIoProtocolGuid ## SOMETIMES_CONSUMES
[Depex]
diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
index 2a26c20f394c..312a92d7461a 100644
--- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
+++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCo
+++ nfigFileExplorer.c
@@ -80,155 +80,6 @@ CleanUpPage (
);
}
-/**
- This function will open a file or directory referenced by DevicePath.
-
- This function opens a file with the open mode according to the file path. The
- Attributes is valid only for EFI_FILE_MODE_CREATE.
-
- @param[in, out] FilePath On input, the device path to the file.
- On output, the remaining device path.
- @param[out] FileHandle Pointer to the file handle.
- @param[in] OpenMode The mode to open the file with.
- @param[in] Attributes The file's file attributes.
-
- @retval EFI_SUCCESS The information was set.
- @retval EFI_INVALID_PARAMETER One of the parameters has an invalid value.
- @retval EFI_UNSUPPORTED Could not open the file path.
- @retval EFI_NOT_FOUND The specified file could not be found on the
- device or the file system could not be found on
- the device.
- @retval EFI_NO_MEDIA The device has no medium.
- @retval EFI_MEDIA_CHANGED The device has a different medium in it or the
- medium is no longer supported.
- @retval EFI_DEVICE_ERROR The device reported an error.
- @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
- @retval EFI_WRITE_PROTECTED The file or medium is write protected.
- @retval EFI_ACCESS_DENIED The file was opened read only.
- @retval EFI_OUT_OF_RESOURCES Not enough resources were available to open the
- file.
- @retval EFI_VOLUME_FULL The volume is full.
-**/
-EFI_STATUS
-EFIAPI
-OpenFileByDevicePath(
- IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
- OUT EFI_FILE_HANDLE *FileHandle,
- IN UINT64 OpenMode,
- IN UINT64 Attributes
- )
-{
- EFI_STATUS Status;
- EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *EfiSimpleFileSystemProtocol;
- EFI_FILE_PROTOCOL *Handle1;
- EFI_FILE_PROTOCOL *Handle2;
- EFI_HANDLE DeviceHandle;
- CHAR16 *PathName;
- UINTN PathLength;
-
- if ((FilePath == NULL || FileHandle == NULL)) {
- return EFI_INVALID_PARAMETER;
- }
-
- Status = gBS->LocateDevicePath (
- &gEfiSimpleFileSystemProtocolGuid,
- FilePath,
- &DeviceHandle
- );
- if (EFI_ERROR (Status)) {
- return Status;
- }
-
- Status = gBS->OpenProtocol(
- DeviceHandle,
- &gEfiSimpleFileSystemProtocolGuid,
- (VOID**)&EfiSimpleFileSystemProtocol,
- gImageHandle,
- NULL,
- EFI_OPEN_PROTOCOL_GET_PROTOCOL
- );
- if (EFI_ERROR (Status)) {
- return Status;
- }
-
- Status = EfiSimpleFileSystemProtocol->OpenVolume(EfiSimpleFileSystemProtocol, &Handle1);
- if (EFI_ERROR (Status)) {
- FileHandle = NULL;
- return Status;
- }
-
- //
- // go down directories one node at a time.
- //
- while (!IsDevicePathEnd (*FilePath)) {
- //
- // For file system access each node should be a file path component
- //
- if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
- DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP
- ) {
- FileHandle = NULL;
- return (EFI_INVALID_PARAMETER);
- }
- //
- // Open this file path node
- //
- Handle2 = Handle1;
- Handle1 = NULL;
- PathLength = DevicePathNodeLength (*FilePath) - sizeof (EFI_DEVICE_PATH_PROTOCOL);
- PathName = AllocateCopyPool (PathLength, ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName);
- if (PathName == NULL) {
- return EFI_OUT_OF_RESOURCES;
- }
-
- //
- // Try to test opening an existing file
- //
- Status = Handle2->Open (
- Handle2,
- &Handle1,
- PathName,
- OpenMode &~EFI_FILE_MODE_CREATE,
- 0
- );
-
- //
- // see if the error was that it needs to be created
- //
- if ((EFI_ERROR (Status)) && (OpenMode != (OpenMode &~EFI_FILE_MODE_CREATE))) {
- Status = Handle2->Open (
- Handle2,
- &Handle1,
- PathName,
- OpenMode,
- Attributes
- );
- }
- //
- // Close the last node
- //
- Handle2->Close (Handle2);
-
- FreePool (PathName);
-
- if (EFI_ERROR(Status)) {
- return (Status);
- }
-
- //
- // Get the next node
- //
- *FilePath = NextDevicePathNode (*FilePath);
- }
-
- //
- // This is a weak spot since if the undefined SHELL_FILE_HANDLE format changes this must change also!
- //
- *FileHandle = (VOID*)Handle1;
- return EFI_SUCCESS;
-}
-
-
/**
Extract filename from device path. The returned buffer is allocated using AllocateCopyPool.
The caller is responsible for freeing the allocated buffer using FreePool(). If return NULL @@ -312,7 +163,7 @@ UpdatePage(
gSecureBootPrivateData->FileContext->FileName = FileName;
- OpenFileByDevicePath(
+ EfiOpenFileByDevicePath(
&FilePath,
&gSecureBootPrivateData->FileContext->FHandle,
EFI_FILE_MODE_READ,
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 5/6] ShellPkg/UefiShellLib: drop DeviceHandle param of ShellOpenFileByDevicePath()
2018-07-18 20:50 [PATCH 0/6] UefiLib: centralize OpenFileByDevicePath() and fix its bugs Laszlo Ersek
` (3 preceding siblings ...)
2018-07-18 20:50 ` [PATCH 4/6] SecurityPkg/SecureBootConfigDxe: " Laszlo Ersek
@ 2018-07-18 20:50 ` Laszlo Ersek
2018-07-18 20:50 ` [PATCH 6/6] ShellPkg/UefiShellLib: rebase ShellOpenFileByDevicePath() to UefiLib API Laszlo Ersek
2018-07-18 21:15 ` [PATCH 0/6] UefiLib: centralize OpenFileByDevicePath() and fix its bugs Carsey, Jaben
6 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-07-18 20:50 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jaben Carsey, Ruiyu Ni
The ShellOpenFileByDevicePath() API promises to set the DeviceHandle
output parameter to the handle of the filesystem identified by the
FilePath input parameter. However, this doesn't actually happen when the
UEFI Shell 2.0 method is used (which is basically "always" nowadays).
Accordingly, the only caller of ShellOpenFileByDevicePath(), namely
ShellOpenFileByName(), defines a (dummy) local DeviceHandle variable just
so it can call ShellOpenFileByDevicePath().
Remove the useless output parameter.
Cc: Jaben Carsey <jaben.carsey@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ShellPkg/Library/UefiShellLib/UefiShellLib.inf | 2 +-
ShellPkg/Include/Library/ShellLib.h | 2 --
ShellPkg/Library/UefiShellLib/UefiShellLib.c | 11 ++++-------
3 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.inf b/ShellPkg/Library/UefiShellLib/UefiShellLib.inf
index 0df632378fe6..38d9a4b81f5f 100644
--- a/ShellPkg/Library/UefiShellLib/UefiShellLib.inf
+++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.inf
@@ -19,7 +19,7 @@ [Defines]
BASE_NAME = UefiShellLib
FILE_GUID = 449D0F00-2148-4a43-9836-F10B3980ECF5
MODULE_TYPE = UEFI_DRIVER
- VERSION_STRING = 1.1
+ VERSION_STRING = 1.2
LIBRARY_CLASS = ShellLib|UEFI_APPLICATION UEFI_DRIVER DXE_RUNTIME_DRIVER DXE_DRIVER
CONSTRUCTOR = ShellLibConstructor
DESTRUCTOR = ShellLibDestructor
diff --git a/ShellPkg/Include/Library/ShellLib.h b/ShellPkg/Include/Library/ShellLib.h
index e360a67ac751..92fddc50f5dd 100644
--- a/ShellPkg/Include/Library/ShellLib.h
+++ b/ShellPkg/Include/Library/ShellLib.h
@@ -89,7 +89,6 @@ ShellSetFileInfo (
@param[in, out] FilePath On input, the device path to the file. On output,
the remaining device path.
- @param[out] DeviceHandle Pointer to the system device handle.
@param[out] FileHandle Pointer to the file handle.
@param[in] OpenMode The mode to open the file with.
@param[in] Attributes The file's file attributes.
@@ -115,7 +114,6 @@ EFI_STATUS
EFIAPI
ShellOpenFileByDevicePath(
IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
- OUT EFI_HANDLE *DeviceHandle,
OUT SHELL_FILE_HANDLE *FileHandle,
IN UINT64 OpenMode,
IN UINT64 Attributes
diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
index 3c24ba1742bf..18c3be4a8bc7 100644
--- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
+++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
@@ -472,7 +472,6 @@ ShellSetFileInfo (
@param FilePath on input the device path to the file. On output
the remaining device path.
- @param DeviceHandle pointer to the system device handle.
@param FileHandle pointer to the file handle.
@param OpenMode the mode to open the file with.
@param Attributes the file's file attributes.
@@ -498,7 +497,6 @@ EFI_STATUS
EFIAPI
ShellOpenFileByDevicePath(
IN OUT EFI_DEVICE_PATH_PROTOCOL **FilePath,
- OUT EFI_HANDLE *DeviceHandle,
OUT SHELL_FILE_HANDLE *FileHandle,
IN UINT64 OpenMode,
IN UINT64 Attributes
@@ -511,8 +509,9 @@ ShellOpenFileByDevicePath(
EFI_FILE_PROTOCOL *Handle2;
CHAR16 *FnafPathName;
UINTN PathLen;
+ EFI_HANDLE DeviceHandle;
- if (FilePath == NULL || FileHandle == NULL || DeviceHandle == NULL) {
+ if (FilePath == NULL || FileHandle == NULL) {
return (EFI_INVALID_PARAMETER);
}
@@ -538,11 +537,11 @@ ShellOpenFileByDevicePath(
//
Status = gBS->LocateDevicePath (&gEfiSimpleFileSystemProtocolGuid,
FilePath,
- DeviceHandle);
+ &DeviceHandle);
if (EFI_ERROR (Status)) {
return Status;
}
- Status = gBS->OpenProtocol(*DeviceHandle,
+ Status = gBS->OpenProtocol(DeviceHandle,
&gEfiSimpleFileSystemProtocolGuid,
(VOID**)&EfiSimpleFileSystemProtocol,
gImageHandle,
@@ -690,7 +689,6 @@ ShellOpenFileByName(
IN UINT64 Attributes
)
{
- EFI_HANDLE DeviceHandle;
EFI_DEVICE_PATH_PROTOCOL *FilePath;
EFI_STATUS Status;
EFI_FILE_INFO *FileInfo;
@@ -774,7 +772,6 @@ ShellOpenFileByName(
FilePath = mEfiShellEnvironment2->NameToPath ((CHAR16*)FileName);
if (FilePath != NULL) {
return (ShellOpenFileByDevicePath(&FilePath,
- &DeviceHandle,
FileHandle,
OpenMode,
Attributes));
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 6/6] ShellPkg/UefiShellLib: rebase ShellOpenFileByDevicePath() to UefiLib API
2018-07-18 20:50 [PATCH 0/6] UefiLib: centralize OpenFileByDevicePath() and fix its bugs Laszlo Ersek
` (4 preceding siblings ...)
2018-07-18 20:50 ` [PATCH 5/6] ShellPkg/UefiShellLib: drop DeviceHandle param of ShellOpenFileByDevicePath() Laszlo Ersek
@ 2018-07-18 20:50 ` Laszlo Ersek
2018-07-18 21:15 ` [PATCH 0/6] UefiLib: centralize OpenFileByDevicePath() and fix its bugs Carsey, Jaben
6 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-07-18 20:50 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jaben Carsey, Ruiyu Ni
Replace the "old shell method" implementation in
ShellOpenFileByDevicePath() with EfiOpenFileByDevicePath() from UefiLib,
correcting the following issues:
- code duplication between this module and other modules,
- local variable name "EfiSimpleFileSystemProtocol" starting with "Efi"
prefix,
- bogus "FileHandle = NULL" assignments,
- leaking "Handle1" when the device path type/subtype check or the
realignment-motivated AllocateCopyPool() fails in the loop.
Cc: Jaben Carsey <jaben.carsey@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ShellPkg/Library/UefiShellLib/UefiShellLib.inf | 1 -
ShellPkg/Library/UefiShellLib/UefiShellLib.c | 113 +-------------------
2 files changed, 3 insertions(+), 111 deletions(-)
diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.inf b/ShellPkg/Library/UefiShellLib/UefiShellLib.inf
index 38d9a4b81f5f..aacddbbf9765 100644
--- a/ShellPkg/Library/UefiShellLib/UefiShellLib.inf
+++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.inf
@@ -51,7 +51,6 @@ [LibraryClasses]
SortLib
[Protocols]
- gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES
gEfiUnicodeCollation2ProtocolGuid ## CONSUMES
# shell 2.0
diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
index 18c3be4a8bc7..f04adbb63ffe 100644
--- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
+++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
@@ -504,12 +504,7 @@ ShellOpenFileByDevicePath(
{
CHAR16 *FileName;
EFI_STATUS Status;
- EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *EfiSimpleFileSystemProtocol;
- EFI_FILE_PROTOCOL *Handle1;
- EFI_FILE_PROTOCOL *Handle2;
- CHAR16 *FnafPathName;
- UINTN PathLen;
- EFI_HANDLE DeviceHandle;
+ EFI_FILE_PROTOCOL *File;
if (FilePath == NULL || FileHandle == NULL) {
return (EFI_INVALID_PARAMETER);
@@ -535,117 +530,15 @@ ShellOpenFileByDevicePath(
//
// use old shell method.
//
- Status = gBS->LocateDevicePath (&gEfiSimpleFileSystemProtocolGuid,
- FilePath,
- &DeviceHandle);
+ Status = EfiOpenFileByDevicePath (FilePath, &File, OpenMode, Attributes);
if (EFI_ERROR (Status)) {
return Status;
}
- Status = gBS->OpenProtocol(DeviceHandle,
- &gEfiSimpleFileSystemProtocolGuid,
- (VOID**)&EfiSimpleFileSystemProtocol,
- gImageHandle,
- NULL,
- EFI_OPEN_PROTOCOL_GET_PROTOCOL);
- if (EFI_ERROR (Status)) {
- return Status;
- }
- Status = EfiSimpleFileSystemProtocol->OpenVolume(EfiSimpleFileSystemProtocol, &Handle1);
- if (EFI_ERROR (Status)) {
- FileHandle = NULL;
- return Status;
- }
-
- //
- // go down directories one node at a time.
- //
- while (!IsDevicePathEnd (*FilePath)) {
- //
- // For file system access each node should be a file path component
- //
- if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
- DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP
- ) {
- FileHandle = NULL;
- return (EFI_INVALID_PARAMETER);
- }
- //
- // Open this file path node
- //
- Handle2 = Handle1;
- Handle1 = NULL;
-
- //
- // File Name Alignment Fix (FNAF)
- // Handle2->Open may be incapable of handling a unaligned CHAR16 data.
- // The structure pointed to by FilePath may be not CHAR16 aligned.
- // This code copies the potentially unaligned PathName data from the
- // FilePath structure to the aligned FnafPathName for use in the
- // calls to Handl2->Open.
- //
-
- //
- // Determine length of PathName, in bytes.
- //
- PathLen = DevicePathNodeLength (*FilePath) - SIZE_OF_FILEPATH_DEVICE_PATH;
-
- //
- // Allocate memory for the aligned copy of the string Extra allocation is to allow for forced alignment
- // Copy bytes from possibly unaligned location to aligned location
- //
- FnafPathName = AllocateCopyPool(PathLen, (UINT8 *)((FILEPATH_DEVICE_PATH*)*FilePath)->PathName);
- if (FnafPathName == NULL) {
- return EFI_OUT_OF_RESOURCES;
- }
-
- //
- // Try to test opening an existing file
- //
- Status = Handle2->Open (
- Handle2,
- &Handle1,
- FnafPathName,
- OpenMode &~EFI_FILE_MODE_CREATE,
- 0
- );
-
- //
- // see if the error was that it needs to be created
- //
- if ((EFI_ERROR (Status)) && (OpenMode != (OpenMode &~EFI_FILE_MODE_CREATE))) {
- Status = Handle2->Open (
- Handle2,
- &Handle1,
- FnafPathName,
- OpenMode,
- Attributes
- );
- }
-
- //
- // Free the alignment buffer
- //
- FreePool(FnafPathName);
-
- //
- // Close the last node
- //
- Handle2->Close (Handle2);
-
- if (EFI_ERROR(Status)) {
- return (Status);
- }
-
- //
- // Get the next node
- //
- *FilePath = NextDevicePathNode (*FilePath);
- }
//
// This is a weak spot since if the undefined SHELL_FILE_HANDLE format changes this must change also!
//
- *FileHandle = (VOID*)Handle1;
+ *FileHandle = (VOID*)File;
return (EFI_SUCCESS);
}
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 0/6] UefiLib: centralize OpenFileByDevicePath() and fix its bugs
2018-07-18 20:50 [PATCH 0/6] UefiLib: centralize OpenFileByDevicePath() and fix its bugs Laszlo Ersek
` (5 preceding siblings ...)
2018-07-18 20:50 ` [PATCH 6/6] ShellPkg/UefiShellLib: rebase ShellOpenFileByDevicePath() to UefiLib API Laszlo Ersek
@ 2018-07-18 21:15 ` Carsey, Jaben
2018-07-19 0:07 ` Ard Biesheuvel
6 siblings, 1 reply; 27+ messages in thread
From: Carsey, Jaben @ 2018-07-18 21:15 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Zhang, Chao B, Dong, Eric, Wu, Jiaxin, Yao, Jiewen, Gao, Liming,
Kinney, Michael D, Roman Bacik, Ni, Ruiyu, Fu, Siyuan, Zeng, Star
Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
One question (do hold up push). Is there a reason to use the former over the latter? I use latter and I see you use the former.
ASSERT(EFI_ERROR (Status));
ASSERT_EFI_ERROR (Status);
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, July 18, 2018 1:51 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Carsey, Jaben <jaben.carsey@intel.com>; Wu, Jiaxin
> <jiaxin.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Roman Bacik <roman.bacik@broadcom.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: [PATCH 0/6] UefiLib: centralize OpenFileByDevicePath() and fix its
> bugs
> Importance: High
>
> Repo: https://github.com/lersek/edk2.git
> Branch: open_file_by_devpath_tiano_1008
>
> This series addresses
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1008>.
>
> In this version of the patch set, EfiOpenFileByDevicePath() is not added
> to the FrameworkUefiLib instance. If FrameworkUefiLib actually needs a
> definition of the function, I suggest that we add it once everybody
> agrees on the implementation.
>
> Tested with:
>
> - MdeModulePkg/RamDiskDxe: created a virtual disk from an ISO file,
> using the HII form; browsed the disk contents from the UEFI shell.
>
> - NetworkPkg/TlsAuthConfigDxe: loaded a CA certificate from a file via
> the HII form, successfully booted via HTTPSv4.
>
> - SecurityPkg/SecureBootConfigDxe: enrolled
> "MicCorKEKCA2011_2011-06-24.crt", "MicCorUEFCA2011_2011-06-27.crt",
> and "MicWinProPCA2011_2011-10-19.crt", using the HII form; verified
> Secure Boot with a Fedora guest.
>
> - ShellPkg/UefiShellLib: couldn't test the "old shell method" code path.
> Help with testing would be appreciated.
>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Roman Bacik <roman.bacik@broadcom.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
>
> Thanks,
> Laszlo
>
> Laszlo Ersek (6):
> MdePkg/UefiLib: introduce EfiOpenFileByDevicePath()
> MdeModulePkg/RamDiskDxe: replace OpenFileByDevicePath() with UefiLib
> API
> NetworkPkg/TlsAuthConfigDxe: replace OpenFileByDevicePath() with
> UefiLib API
> SecurityPkg/SecureBootConfigDxe: replace OpenFileByDevicePath() with
> UefiLib API
> ShellPkg/UefiShellLib: drop DeviceHandle param of
> ShellOpenFileByDevicePath()
> ShellPkg/UefiShellLib: rebase ShellOpenFileByDevicePath() to UefiLib
> API
>
> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
> | 1 -
> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c
> | 140 ------------
> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c
> | 2 +-
> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h
> | 39 ----
> MdePkg/Include/Library/UefiLib.h | 86 ++++++++
> MdePkg/Library/UefiLib/UefiLib.c | 226
> ++++++++++++++++++++
> MdePkg/Library/UefiLib/UefiLib.inf | 1 +
> NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf | 1
> -
> NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c | 141
> +-----------
>
> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfi
> gDxe.inf | 1 -
>
> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfi
> gFileExplorer.c | 151 +------------
> ShellPkg/Include/Library/ShellLib.h | 2 -
> ShellPkg/Library/UefiShellLib/UefiShellLib.c | 118 +-------
> --
> ShellPkg/Library/UefiShellLib/UefiShellLib.inf | 3 +-
> 14 files changed, 321 insertions(+), 591 deletions(-)
>
> --
> 2.14.1.3.gb7cf6e02401b
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 0/6] UefiLib: centralize OpenFileByDevicePath() and fix its bugs
2018-07-18 21:15 ` [PATCH 0/6] UefiLib: centralize OpenFileByDevicePath() and fix its bugs Carsey, Jaben
@ 2018-07-19 0:07 ` Ard Biesheuvel
2018-07-19 10:38 ` Laszlo Ersek
0 siblings, 1 reply; 27+ messages in thread
From: Ard Biesheuvel @ 2018-07-19 0:07 UTC (permalink / raw)
To: Carsey, Jaben
Cc: Laszlo Ersek, edk2-devel-01, Ni, Ruiyu, Dong, Eric, Gao, Liming,
Wu, Jiaxin, Yao, Jiewen, Zeng, Star, Kinney, Michael D,
Fu, Siyuan, Zhang, Chao B
On 19 July 2018 at 06:15, Carsey, Jaben <jaben.carsey@intel.com> wrote:
> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
>
> One question (do hold up push). Is there a reason to use the former over the latter? I use latter and I see you use the former.
>
> ASSERT(EFI_ERROR (Status));
> ASSERT_EFI_ERROR (Status);
>
The former asserts that an error has occurred, the latter does the opposite.
It seems Laszlo is using this to ensure we don't end up in the error
handling path if no error occurred.
--
Ard.
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, July 18, 2018 1:51 PM
>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Dong, Eric
>> <eric.dong@intel.com>; Carsey, Jaben <jaben.carsey@intel.com>; Wu, Jiaxin
>> <jiaxin.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
>> Roman Bacik <roman.bacik@broadcom.com>; Ni, Ruiyu
>> <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Zeng, Star
>> <star.zeng@intel.com>
>> Subject: [PATCH 0/6] UefiLib: centralize OpenFileByDevicePath() and fix its
>> bugs
>> Importance: High
>>
>> Repo: https://github.com/lersek/edk2.git
>> Branch: open_file_by_devpath_tiano_1008
>>
>> This series addresses
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=1008>.
>>
>> In this version of the patch set, EfiOpenFileByDevicePath() is not added
>> to the FrameworkUefiLib instance. If FrameworkUefiLib actually needs a
>> definition of the function, I suggest that we add it once everybody
>> agrees on the implementation.
>>
>> Tested with:
>>
>> - MdeModulePkg/RamDiskDxe: created a virtual disk from an ISO file,
>> using the HII form; browsed the disk contents from the UEFI shell.
>>
>> - NetworkPkg/TlsAuthConfigDxe: loaded a CA certificate from a file via
>> the HII form, successfully booted via HTTPSv4.
>>
>> - SecurityPkg/SecureBootConfigDxe: enrolled
>> "MicCorKEKCA2011_2011-06-24.crt", "MicCorUEFCA2011_2011-06-27.crt",
>> and "MicWinProPCA2011_2011-10-19.crt", using the HII form; verified
>> Secure Boot with a Fedora guest.
>>
>> - ShellPkg/UefiShellLib: couldn't test the "old shell method" code path.
>> Help with testing would be appreciated.
>>
>> Cc: Chao Zhang <chao.b.zhang@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jaben Carsey <jaben.carsey@intel.com>
>> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Roman Bacik <roman.bacik@broadcom.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Siyuan Fu <siyuan.fu@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>>
>> Thanks,
>> Laszlo
>>
>> Laszlo Ersek (6):
>> MdePkg/UefiLib: introduce EfiOpenFileByDevicePath()
>> MdeModulePkg/RamDiskDxe: replace OpenFileByDevicePath() with UefiLib
>> API
>> NetworkPkg/TlsAuthConfigDxe: replace OpenFileByDevicePath() with
>> UefiLib API
>> SecurityPkg/SecureBootConfigDxe: replace OpenFileByDevicePath() with
>> UefiLib API
>> ShellPkg/UefiShellLib: drop DeviceHandle param of
>> ShellOpenFileByDevicePath()
>> ShellPkg/UefiShellLib: rebase ShellOpenFileByDevicePath() to UefiLib
>> API
>>
>> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
>> | 1 -
>> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c
>> | 140 ------------
>> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c
>> | 2 +-
>> MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h
>> | 39 ----
>> MdePkg/Include/Library/UefiLib.h | 86 ++++++++
>> MdePkg/Library/UefiLib/UefiLib.c | 226
>> ++++++++++++++++++++
>> MdePkg/Library/UefiLib/UefiLib.inf | 1 +
>> NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf | 1
>> -
>> NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c | 141
>> +-----------
>>
>> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfi
>> gDxe.inf | 1 -
>>
>> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfi
>> gFileExplorer.c | 151 +------------
>> ShellPkg/Include/Library/ShellLib.h | 2 -
>> ShellPkg/Library/UefiShellLib/UefiShellLib.c | 118 +-------
>> --
>> ShellPkg/Library/UefiShellLib/UefiShellLib.inf | 3 +-
>> 14 files changed, 321 insertions(+), 591 deletions(-)
>>
>> --
>> 2.14.1.3.gb7cf6e02401b
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/6] UefiLib: centralize OpenFileByDevicePath() and fix its bugs
2018-07-19 0:07 ` Ard Biesheuvel
@ 2018-07-19 10:38 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2018-07-19 10:38 UTC (permalink / raw)
To: Ard Biesheuvel, Carsey, Jaben
Cc: edk2-devel-01, Ni, Ruiyu, Dong, Eric, Gao, Liming, Wu, Jiaxin,
Yao, Jiewen, Zeng, Star, Kinney, Michael D, Fu, Siyuan,
Zhang, Chao B
On 07/19/18 02:07, Ard Biesheuvel wrote:
> On 19 July 2018 at 06:15, Carsey, Jaben <jaben.carsey@intel.com> wrote:
>> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
>>
>> One question (do hold up push). Is there a reason to use the former over the latter? I use latter and I see you use the former.
>>
>> ASSERT(EFI_ERROR (Status));
>> ASSERT_EFI_ERROR (Status);
>>
>
> The former asserts that an error has occurred, the latter does the opposite.
Right -- the latter is actually a misnomer; that macro should have
always been called "ASSERT_NO_EFI_ERROR" :)
> It seems Laszlo is using this to ensure we don't end up in the error
> handling path if no error occurred.
Sort of -- the way I put it for myself was, ensure that we don't exit on
the error path without returning an error status to the caller. So it's
less about control flow, and more about setting (or recognizing) error
statuses at all locations that jump to the error path.
But I guess that's just both sides of the same coin. :)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread