* [PATCH V2 0/3] Update comments and code for GetNextVariableName to follow UEFI 2.7
@ 2017-06-23 8:08 Star Zeng
2017-06-23 8:08 ` [PATCH V2 1/3] MdePkg: Update comments " Star Zeng
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Star Zeng @ 2017-06-23 8:08 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng
V2: Add changes for EmuVariable and FsVariable.
"The size must be large enough to fit input string supplied in
VariableName buffer" is added in the description for VariableNameSize.
And two cases of EFI_INVALID_PARAMETER are added.
1. The input values of VariableName and VendorGuid are not a name and
GUID of an existing variable.
2. Null-terminator is not found in the first VariableNameSize bytes of
the input VariableName buffer.
Star Zeng (3):
MdePkg: Update comments for GetNextVariableName to follow UEFI 2.7
MdeModulePkg Variable: Update GetNextVariableName to follow UEFI 2.7
DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
DuetPkg/FSVariable/FSVariable.c | 21 +++++++++++++++++-
.../Universal/Variable/EmuRuntimeDxe/EmuVariable.c | 25 +++++++++++++++++++++-
.../Universal/Variable/RuntimeDxe/Variable.c | 19 ++++++++++++++++
MdePkg/Include/Uefi/UefiSpec.h | 7 +++++-
4 files changed, 69 insertions(+), 3 deletions(-)
--
2.7.0.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V2 1/3] MdePkg: Update comments for GetNextVariableName to follow UEFI 2.7
2017-06-23 8:08 [PATCH V2 0/3] Update comments and code for GetNextVariableName to follow UEFI 2.7 Star Zeng
@ 2017-06-23 8:08 ` Star Zeng
2017-06-23 8:08 ` [PATCH V2 2/3] MdeModulePkg Variable: Update " Star Zeng
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: Star Zeng @ 2017-06-23 8:08 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Liming Gao
"The size must be large enough to fit input string supplied in
VariableName buffer" is added in the description for VariableNameSize.
And two cases of EFI_INVALID_PARAMETER are added.
1. The input values of VariableName and VendorGuid are not a name and
GUID of an existing variable.
2. Null-terminator is not found in the first VariableNameSize bytes of
the input VariableName buffer.
This patch is to update comments for GetNextVariableName to follow them.
Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng <star.zeng@intel.com>
---
MdePkg/Include/Uefi/UefiSpec.h | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/MdePkg/Include/Uefi/UefiSpec.h b/MdePkg/Include/Uefi/UefiSpec.h
index eb662a35503c..0b84f4829749 100644
--- a/MdePkg/Include/Uefi/UefiSpec.h
+++ b/MdePkg/Include/Uefi/UefiSpec.h
@@ -661,7 +661,8 @@ EFI_STATUS
/**
Enumerates the current variable names.
- @param[in, out] VariableNameSize The size of the VariableName buffer.
+ @param[in, out] VariableNameSize The size of the VariableName buffer. The size must be large
+ enough to fit input string supplied in VariableName buffer.
@param[in, out] VariableName On input, supplies the last VariableName that was returned
by GetNextVariableName(). On output, returns the Nullterminated
string of the current variable.
@@ -675,6 +676,10 @@ EFI_STATUS
@retval EFI_INVALID_PARAMETER VariableNameSize is NULL.
@retval EFI_INVALID_PARAMETER VariableName is NULL.
@retval EFI_INVALID_PARAMETER VendorGuid is NULL.
+ @retval EFI_INVALID_PARAMETER The input values of VariableName and VendorGuid are not a name and
+ GUID of an existing variable.
+ @retval EFI_INVALID_PARAMETER Null-terminator is not found in the first VariableNameSize bytes of
+ the input VariableName buffer.
@retval EFI_DEVICE_ERROR The variable could not be retrieved due to a hardware error.
**/
--
2.7.0.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH V2 2/3] MdeModulePkg Variable: Update GetNextVariableName to follow UEFI 2.7
2017-06-23 8:08 [PATCH V2 0/3] Update comments and code for GetNextVariableName to follow UEFI 2.7 Star Zeng
2017-06-23 8:08 ` [PATCH V2 1/3] MdePkg: Update comments " Star Zeng
@ 2017-06-23 8:08 ` Star Zeng
2017-06-23 8:08 ` [PATCH V2 3/3] DuetPkg FsVariable: " Star Zeng
2017-06-23 8:10 ` [PATCH V2 0/3] Update comments and code for " Gao, Liming
3 siblings, 0 replies; 15+ messages in thread
From: Star Zeng @ 2017-06-23 8:08 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Liming Gao
"The size must be large enough to fit input string supplied in
VariableName buffer" is added in the description for VariableNameSize.
And two cases of EFI_INVALID_PARAMETER are added.
1. The input values of VariableName and VendorGuid are not a name and
GUID of an existing variable.
2. Null-terminator is not found in the first VariableNameSize bytes of
the input VariableName buffer.
This patch is to update code to follow them.
Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng <star.zeng@intel.com>
---
.../Universal/Variable/EmuRuntimeDxe/EmuVariable.c | 25 +++++++++++++++++++++-
.../Universal/Variable/RuntimeDxe/Variable.c | 19 ++++++++++++++++
2 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Universal/Variable/EmuRuntimeDxe/EmuVariable.c b/MdeModulePkg/Universal/Variable/EmuRuntimeDxe/EmuVariable.c
index 27ea1496a044..6211ec52a439 100644
--- a/MdeModulePkg/Universal/Variable/EmuRuntimeDxe/EmuVariable.c
+++ b/MdeModulePkg/Universal/Variable/EmuRuntimeDxe/EmuVariable.c
@@ -3,7 +3,7 @@
Emulation Variable services operate on the runtime volatile memory.
The nonvolatile variable space doesn't exist.
-Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
which accompanies this distribution. The full text of the license may be found at
@@ -1245,6 +1245,10 @@ Done:
@retval EFI_BUFFER_TOO_SMALL VariableNameSize is too small for the result.
VariableNameSize has been updated with the size needed to complete the request.
@retval EFI_INVALID_PARAMETER VariableNameSize or VariableName or VendorGuid is NULL.
+ @retval EFI_INVALID_PARAMETER The input values of VariableName and VendorGuid are not a name and
+ GUID of an existing variable.
+ @retval EFI_INVALID_PARAMETER Null-terminator is not found in the first VariableNameSize bytes of
+ the input VariableName buffer.
**/
EFI_STATUS
@@ -1259,16 +1263,35 @@ EmuGetNextVariableName (
VARIABLE_POINTER_TRACK Variable;
UINTN VarNameSize;
EFI_STATUS Status;
+ UINTN MaxLen;
if (VariableNameSize == NULL || VariableName == NULL || VendorGuid == NULL) {
return EFI_INVALID_PARAMETER;
}
+ //
+ // Calculate the possible maximum length of name string, including the Null terminator.
+ //
+ MaxLen = *VariableNameSize / sizeof (CHAR16);
+ if ((MaxLen == 0) ||
+ ((VariableName[MaxLen - 1] != 0) && (StrnLenS (VariableName, MaxLen) == MaxLen))) {
+ //
+ // Null-terminator is not found in the first VariableNameSize bytes of the input VariableName buffer.
+ //
+ return EFI_INVALID_PARAMETER;
+ }
+
AcquireLockOnlyAtBootTime(&Global->VariableServicesLock);
Status = FindVariable (VariableName, VendorGuid, &Variable, Global);
if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
+ if (VariableName[0] != 0) {
+ //
+ // The input values of VariableName and VendorGuid are not a name and GUID of an existing variable.
+ //
+ Status = EFI_INVALID_PARAMETER;
+ }
goto Done;
}
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 0a325de1659d..1e68c0a73a6d 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -2926,6 +2926,12 @@ VariableServiceGetNextVariableInternal (
Status = FindVariable (VariableName, VendorGuid, &Variable, &mVariableModuleGlobal->VariableGlobal, FALSE);
if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
+ if (VariableName[0] != 0) {
+ //
+ // The input values of VariableName and VendorGuid are not a name and GUID of an existing variable.
+ //
+ Status = EFI_INVALID_PARAMETER;
+ }
goto Done;
}
@@ -3065,6 +3071,7 @@ VariableServiceGetNextVariableName (
)
{
EFI_STATUS Status;
+ UINTN MaxLen;
UINTN VarNameSize;
VARIABLE_HEADER *VariablePtr;
@@ -3072,6 +3079,18 @@ VariableServiceGetNextVariableName (
return EFI_INVALID_PARAMETER;
}
+ //
+ // Calculate the possible maximum length of name string, including the Null terminator.
+ //
+ MaxLen = *VariableNameSize / sizeof (CHAR16);
+ if ((MaxLen == 0) ||
+ ((VariableName[MaxLen - 1] != 0) && (StrnLenS (VariableName, MaxLen) == MaxLen))) {
+ //
+ // Null-terminator is not found in the first VariableNameSize bytes of the input VariableName buffer.
+ //
+ return EFI_INVALID_PARAMETER;
+ }
+
AcquireLockOnlyAtBootTime(&mVariableModuleGlobal->VariableGlobal.VariableServicesLock);
Status = VariableServiceGetNextVariableInternal (VariableName, VendorGuid, &VariablePtr);
--
2.7.0.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
2017-06-23 8:08 [PATCH V2 0/3] Update comments and code for GetNextVariableName to follow UEFI 2.7 Star Zeng
2017-06-23 8:08 ` [PATCH V2 1/3] MdePkg: Update comments " Star Zeng
2017-06-23 8:08 ` [PATCH V2 2/3] MdeModulePkg Variable: Update " Star Zeng
@ 2017-06-23 8:08 ` Star Zeng
2017-06-23 8:20 ` Ni, Ruiyu
2017-06-23 8:10 ` [PATCH V2 0/3] Update comments and code for " Gao, Liming
3 siblings, 1 reply; 15+ messages in thread
From: Star Zeng @ 2017-06-23 8:08 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Liming Gao, Ruiyu Ni
"The size must be large enough to fit input string supplied in
VariableName buffer" is added in the description for VariableNameSize.
And two cases of EFI_INVALID_PARAMETER are added.
1. The input values of VariableName and VendorGuid are not a name and
GUID of an existing variable.
2. Null-terminator is not found in the first VariableNameSize bytes of
the input VariableName buffer.
This patch is to update code to follow them.
Cc: Liming Gao <liming.gao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng <star.zeng@intel.com>
---
DuetPkg/FSVariable/FSVariable.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/DuetPkg/FSVariable/FSVariable.c b/DuetPkg/FSVariable/FSVariable.c
index 34b79305c871..6069cfa8fb98 100644
--- a/DuetPkg/FSVariable/FSVariable.c
+++ b/DuetPkg/FSVariable/FSVariable.c
@@ -6,7 +6,7 @@ disk. They can be changed by user. BIOS is not able to protoect those.
Duet trusts all meta data from disk. If variable code, variable metadata and variable
data is modified in inproper way, the behavior is undefined.
-Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
which accompanies this distribution. The full text of the license may be found at
@@ -1400,14 +1400,33 @@ Returns:
VARIABLE_POINTER_TRACK Variable;
UINTN VarNameSize;
EFI_STATUS Status;
+ UINTN MaxLen;
if (VariableNameSize == NULL || VariableName == NULL || VendorGuid == NULL) {
return EFI_INVALID_PARAMETER;
}
+ //
+ // Calculate the possible maximum length of name string, including the Null terminator.
+ //
+ MaxLen = *VariableNameSize / sizeof (CHAR16);
+ if ((MaxLen == 0) ||
+ ((VariableName[MaxLen - 1] != 0) && (StrnLenS (VariableName, MaxLen) == MaxLen))) {
+ //
+ // Null-terminator is not found in the first VariableNameSize bytes of the input VariableName buffer.
+ //
+ return EFI_INVALID_PARAMETER;
+ }
+
Status = FindVariable (VariableName, VendorGuid, &Variable);
if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
+ if (VariableName[0] != 0) {
+ //
+ // The input values of VariableName and VendorGuid are not a name and GUID of an existing variable.
+ //
+ Status = EFI_INVALID_PARAMETER;
+ }
return Status;
}
--
2.7.0.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH V2 0/3] Update comments and code for GetNextVariableName to follow UEFI 2.7
2017-06-23 8:08 [PATCH V2 0/3] Update comments and code for GetNextVariableName to follow UEFI 2.7 Star Zeng
` (2 preceding siblings ...)
2017-06-23 8:08 ` [PATCH V2 3/3] DuetPkg FsVariable: " Star Zeng
@ 2017-06-23 8:10 ` Gao, Liming
3 siblings, 0 replies; 15+ messages in thread
From: Gao, Liming @ 2017-06-23 8:10 UTC (permalink / raw)
To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Zeng, Star
Reviewed-by: Liming Gao <liming.gao@intel.com>
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Star
>Zeng
>Sent: Friday, June 23, 2017 4:08 PM
>To: edk2-devel@lists.01.org
>Cc: Zeng, Star <star.zeng@intel.com>
>Subject: [edk2] [PATCH V2 0/3] Update comments and code for
>GetNextVariableName to follow UEFI 2.7
>
>V2: Add changes for EmuVariable and FsVariable.
>
>"The size must be large enough to fit input string supplied in
>VariableName buffer" is added in the description for VariableNameSize.
>And two cases of EFI_INVALID_PARAMETER are added.
>1. The input values of VariableName and VendorGuid are not a name and
> GUID of an existing variable.
>2. Null-terminator is not found in the first VariableNameSize bytes of
> the input VariableName buffer.
>
>Star Zeng (3):
> MdePkg: Update comments for GetNextVariableName to follow UEFI 2.7
> MdeModulePkg Variable: Update GetNextVariableName to follow UEFI 2.7
> DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
>
> DuetPkg/FSVariable/FSVariable.c | 21 +++++++++++++++++-
> .../Universal/Variable/EmuRuntimeDxe/EmuVariable.c | 25
>+++++++++++++++++++++-
> .../Universal/Variable/RuntimeDxe/Variable.c | 19 ++++++++++++++++
> MdePkg/Include/Uefi/UefiSpec.h | 7 +++++-
> 4 files changed, 69 insertions(+), 3 deletions(-)
>
>--
>2.7.0.windows.1
>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
2017-06-23 8:08 ` [PATCH V2 3/3] DuetPkg FsVariable: " Star Zeng
@ 2017-06-23 8:20 ` Ni, Ruiyu
2017-06-23 9:33 ` Zeng, Star
0 siblings, 1 reply; 15+ messages in thread
From: Ni, Ruiyu @ 2017-06-23 8:20 UTC (permalink / raw)
To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Gao, Liming
Star,
What's the benefit of this check "VariableName[MaxLen - 1] != 0"?
I think this check "StrnLenS (VariableName, MaxLen) == MaxLen" should be enough.
Thanks/Ray
> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, June 23, 2017 4:08 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName
> to follow UEFI 2.7
>
> "The size must be large enough to fit input string supplied in
> VariableName buffer" is added in the description for VariableNameSize.
> And two cases of EFI_INVALID_PARAMETER are added.
> 1. The input values of VariableName and VendorGuid are not a name and
> GUID of an existing variable.
> 2. Null-terminator is not found in the first VariableNameSize bytes of
> the input VariableName buffer.
>
> This patch is to update code to follow them.
>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
> DuetPkg/FSVariable/FSVariable.c | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/DuetPkg/FSVariable/FSVariable.c
> b/DuetPkg/FSVariable/FSVariable.c
> index 34b79305c871..6069cfa8fb98 100644
> --- a/DuetPkg/FSVariable/FSVariable.c
> +++ b/DuetPkg/FSVariable/FSVariable.c
> @@ -6,7 +6,7 @@ disk. They can be changed by user. BIOS is not able to
> protoect those.
> Duet trusts all meta data from disk. If variable code, variable metadata and
> variable
> data is modified in inproper way, the behavior is undefined.
>
> -Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD
> License
> which accompanies this distribution. The full text of the license may be
> found at
> @@ -1400,14 +1400,33 @@ Returns:
> VARIABLE_POINTER_TRACK Variable;
> UINTN VarNameSize;
> EFI_STATUS Status;
> + UINTN MaxLen;
>
> if (VariableNameSize == NULL || VariableName == NULL || VendorGuid ==
> NULL) {
> return EFI_INVALID_PARAMETER;
> }
>
> + //
> + // Calculate the possible maximum length of name string, including the Null
> terminator.
> + //
> + MaxLen = *VariableNameSize / sizeof (CHAR16);
> + if ((MaxLen == 0) ||
> + ((VariableName[MaxLen - 1] != 0) && (StrnLenS (VariableName, MaxLen)
> == MaxLen))) {
> + //
> + // Null-terminator is not found in the first VariableNameSize bytes of the
> input VariableName buffer.
> + //
> + return EFI_INVALID_PARAMETER;
> + }
> +
> Status = FindVariable (VariableName, VendorGuid, &Variable);
>
> if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
> + if (VariableName[0] != 0) {
> + //
> + // The input values of VariableName and VendorGuid are not a name
> and GUID of an existing variable.
> + //
> + Status = EFI_INVALID_PARAMETER;
> + }
> return Status;
> }
>
> --
> 2.7.0.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
2017-06-23 8:20 ` Ni, Ruiyu
@ 2017-06-23 9:33 ` Zeng, Star
2017-06-24 2:07 ` Ni, Ruiyu
0 siblings, 1 reply; 15+ messages in thread
From: Zeng, Star @ 2017-06-23 9:33 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Gao, Liming, Zeng, Star
Ray,
It is to pass the check quickly and avoid scanning all the chars in VariableName by StrnLenS for normal boot without invalid cases.
I did experiments in the code of GetNextVariableName with below debug code for normal boot on NT32 and my real platforms, all the cases will go into the branch "xxx 2".
if (((VariableName[MaxLen - 1] != 0))) {
DEBUG ((DEBUG_INFO, "xxx 1\n"));
} else {
DEBUG ((DEBUG_INFO, "xxx 2\n"));
}
Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu
Sent: Friday, June 23, 2017 4:20 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Gao, Liming <liming.gao@intel.com>
Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
Star,
What's the benefit of this check "VariableName[MaxLen - 1] != 0"?
I think this check "StrnLenS (VariableName, MaxLen) == MaxLen" should be enough.
Thanks/Ray
> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, June 23, 2017 4:08 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName
> to follow UEFI 2.7
>
> "The size must be large enough to fit input string supplied in
> VariableName buffer" is added in the description for VariableNameSize.
> And two cases of EFI_INVALID_PARAMETER are added.
> 1. The input values of VariableName and VendorGuid are not a name and
> GUID of an existing variable.
> 2. Null-terminator is not found in the first VariableNameSize bytes of
> the input VariableName buffer.
>
> This patch is to update code to follow them.
>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
> DuetPkg/FSVariable/FSVariable.c | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/DuetPkg/FSVariable/FSVariable.c
> b/DuetPkg/FSVariable/FSVariable.c
> index 34b79305c871..6069cfa8fb98 100644
> --- a/DuetPkg/FSVariable/FSVariable.c
> +++ b/DuetPkg/FSVariable/FSVariable.c
> @@ -6,7 +6,7 @@ disk. They can be changed by user. BIOS is not able to
> protoect those.
> Duet trusts all meta data from disk. If variable code, variable metadata and
> variable
> data is modified in inproper way, the behavior is undefined.
>
> -Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD
> License
> which accompanies this distribution. The full text of the license may be
> found at
> @@ -1400,14 +1400,33 @@ Returns:
> VARIABLE_POINTER_TRACK Variable;
> UINTN VarNameSize;
> EFI_STATUS Status;
> + UINTN MaxLen;
>
> if (VariableNameSize == NULL || VariableName == NULL || VendorGuid ==
> NULL) {
> return EFI_INVALID_PARAMETER;
> }
>
> + //
> + // Calculate the possible maximum length of name string, including the Null
> terminator.
> + //
> + MaxLen = *VariableNameSize / sizeof (CHAR16);
> + if ((MaxLen == 0) ||
> + ((VariableName[MaxLen - 1] != 0) && (StrnLenS (VariableName, MaxLen)
> == MaxLen))) {
> + //
> + // Null-terminator is not found in the first VariableNameSize bytes of the
> input VariableName buffer.
> + //
> + return EFI_INVALID_PARAMETER;
> + }
> +
> Status = FindVariable (VariableName, VendorGuid, &Variable);
>
> if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
> + if (VariableName[0] != 0) {
> + //
> + // The input values of VariableName and VendorGuid are not a name
> and GUID of an existing variable.
> + //
> + Status = EFI_INVALID_PARAMETER;
> + }
> return Status;
> }
>
> --
> 2.7.0.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
2017-06-23 9:33 ` Zeng, Star
@ 2017-06-24 2:07 ` Ni, Ruiyu
2017-06-26 3:04 ` Zeng, Star
0 siblings, 1 reply; 15+ messages in thread
From: Ni, Ruiyu @ 2017-06-24 2:07 UTC (permalink / raw)
To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Gao, Liming
Star,
I don't recommend to add the additional check for performance consideration.
Because we have no idea what the input VariableName buffer is like.
If the VariableName is like ['\0', '?', '?', '?'] with MaxLen equals to 4,
"VariableName[MaxLen-1] != 0" check is redundant.
The NT32 case you met cannot represent the all possible cases.
You could use the possibility theory to decide what the most efficient way is.
Additionally I think code readability is more important than efficiency.
In this case, we need the data about the performance improvement to decide
whether this check is necessary.
Regards,
Ray
>-----Original Message-----
>From: Zeng, Star
>Sent: Friday, June 23, 2017 5:33 PM
>To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
>Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
>Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
>
>Ray,
>
>It is to pass the check quickly and avoid scanning all the chars in VariableName by StrnLenS for normal boot without invalid
>cases.
>I did experiments in the code of GetNextVariableName with below debug code for normal boot on NT32 and my real
>platforms, all the cases will go into the branch "xxx 2".
> if (((VariableName[MaxLen - 1] != 0))) {
> DEBUG ((DEBUG_INFO, "xxx 1\n"));
> } else {
> DEBUG ((DEBUG_INFO, "xxx 2\n"));
> }
>
>
>Thanks,
>Star
>-----Original Message-----
>From: Ni, Ruiyu
>Sent: Friday, June 23, 2017 4:20 PM
>To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
>Cc: Gao, Liming <liming.gao@intel.com>
>Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
>
>Star,
>What's the benefit of this check "VariableName[MaxLen - 1] != 0"?
>I think this check "StrnLenS (VariableName, MaxLen) == MaxLen" should be enough.
>
>Thanks/Ray
>
>> -----Original Message-----
>> From: Zeng, Star
>> Sent: Friday, June 23, 2017 4:08 PM
>> To: edk2-devel@lists.01.org
>> Cc: Zeng, Star <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>;
>> Ni, Ruiyu <ruiyu.ni@intel.com>
>> Subject: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName
>> to follow UEFI 2.7
>>
>> "The size must be large enough to fit input string supplied in
>> VariableName buffer" is added in the description for VariableNameSize.
>> And two cases of EFI_INVALID_PARAMETER are added.
>> 1. The input values of VariableName and VendorGuid are not a name and
>> GUID of an existing variable.
>> 2. Null-terminator is not found in the first VariableNameSize bytes of
>> the input VariableName buffer.
>>
>> This patch is to update code to follow them.
>>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Star Zeng <star.zeng@intel.com>
>> ---
>> DuetPkg/FSVariable/FSVariable.c | 21 ++++++++++++++++++++-
>> 1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/DuetPkg/FSVariable/FSVariable.c
>> b/DuetPkg/FSVariable/FSVariable.c
>> index 34b79305c871..6069cfa8fb98 100644
>> --- a/DuetPkg/FSVariable/FSVariable.c
>> +++ b/DuetPkg/FSVariable/FSVariable.c
>> @@ -6,7 +6,7 @@ disk. They can be changed by user. BIOS is not able to
>> protoect those.
>> Duet trusts all meta data from disk. If variable code, variable metadata and
>> variable
>> data is modified in inproper way, the behavior is undefined.
>>
>> -Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
>> +Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
>> This program and the accompanying materials
>> are licensed and made available under the terms and conditions of the BSD
>> License
>> which accompanies this distribution. The full text of the license may be
>> found at
>> @@ -1400,14 +1400,33 @@ Returns:
>> VARIABLE_POINTER_TRACK Variable;
>> UINTN VarNameSize;
>> EFI_STATUS Status;
>> + UINTN MaxLen;
>>
>> if (VariableNameSize == NULL || VariableName == NULL || VendorGuid ==
>> NULL) {
>> return EFI_INVALID_PARAMETER;
>> }
>>
>> + //
>> + // Calculate the possible maximum length of name string, including the Null
>> terminator.
>> + //
>> + MaxLen = *VariableNameSize / sizeof (CHAR16);
>> + if ((MaxLen == 0) ||
>> + ((VariableName[MaxLen - 1] != 0) && (StrnLenS (VariableName, MaxLen)
>> == MaxLen))) {
>> + //
>> + // Null-terminator is not found in the first VariableNameSize bytes of the
>> input VariableName buffer.
>> + //
>> + return EFI_INVALID_PARAMETER;
>> + }
>> +
>> Status = FindVariable (VariableName, VendorGuid, &Variable);
>>
>> if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
>> + if (VariableName[0] != 0) {
>> + //
>> + // The input values of VariableName and VendorGuid are not a name
>> and GUID of an existing variable.
>> + //
>> + Status = EFI_INVALID_PARAMETER;
>> + }
>> return Status;
>> }
>>
>> --
>> 2.7.0.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
2017-06-24 2:07 ` Ni, Ruiyu
@ 2017-06-26 3:04 ` Zeng, Star
2017-06-26 5:36 ` Ni, Ruiyu
0 siblings, 1 reply; 15+ messages in thread
From: Zeng, Star @ 2017-06-26 3:04 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Gao, Liming, Zeng, Star
Ray,
The code is like low hanging fruit from my practice for me, and I don't think it hurts readability although it may not bring performance improvement, it depends on how many variables in variable region, how many times of calling GetNextVariableName, and how fast of GetNextVariableName.
The code practice I did is on NT32 and my real platforms. Is there anyone can make sure he/she tested all the systems in the world for their code?
Anyway, I can update the patch if you insist.
Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu
Sent: Saturday, June 24, 2017 10:08 AM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Gao, Liming <liming.gao@intel.com>
Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
Star,
I don't recommend to add the additional check for performance consideration.
Because we have no idea what the input VariableName buffer is like.
If the VariableName is like ['\0', '?', '?', '?'] with MaxLen equals to 4, "VariableName[MaxLen-1] != 0" check is redundant.
The NT32 case you met cannot represent the all possible cases.
You could use the possibility theory to decide what the most efficient way is.
Additionally I think code readability is more important than efficiency.
In this case, we need the data about the performance improvement to decide whether this check is necessary.
Regards,
Ray
>-----Original Message-----
>From: Zeng, Star
>Sent: Friday, June 23, 2017 5:33 PM
>To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
>Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star
><star.zeng@intel.com>
>Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
>GetNextVariableName to follow UEFI 2.7
>
>Ray,
>
>It is to pass the check quickly and avoid scanning all the chars in
>VariableName by StrnLenS for normal boot without invalid cases.
>I did experiments in the code of GetNextVariableName with below debug
>code for normal boot on NT32 and my real platforms, all the cases will go into the branch "xxx 2".
> if (((VariableName[MaxLen - 1] != 0))) {
> DEBUG ((DEBUG_INFO, "xxx 1\n"));
> } else {
> DEBUG ((DEBUG_INFO, "xxx 2\n"));
> }
>
>
>Thanks,
>Star
>-----Original Message-----
>From: Ni, Ruiyu
>Sent: Friday, June 23, 2017 4:20 PM
>To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
>Cc: Gao, Liming <liming.gao@intel.com>
>Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
>GetNextVariableName to follow UEFI 2.7
>
>Star,
>What's the benefit of this check "VariableName[MaxLen - 1] != 0"?
>I think this check "StrnLenS (VariableName, MaxLen) == MaxLen" should be enough.
>
>Thanks/Ray
>
>> -----Original Message-----
>> From: Zeng, Star
>> Sent: Friday, June 23, 2017 4:08 PM
>> To: edk2-devel@lists.01.org
>> Cc: Zeng, Star <star.zeng@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
>> Subject: [PATCH V2 3/3] DuetPkg FsVariable: Update
>> GetNextVariableName to follow UEFI 2.7
>>
>> "The size must be large enough to fit input string supplied in
>> VariableName buffer" is added in the description for VariableNameSize.
>> And two cases of EFI_INVALID_PARAMETER are added.
>> 1. The input values of VariableName and VendorGuid are not a name and
>> GUID of an existing variable.
>> 2. Null-terminator is not found in the first VariableNameSize bytes of
>> the input VariableName buffer.
>>
>> This patch is to update code to follow them.
>>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Star Zeng <star.zeng@intel.com>
>> ---
>> DuetPkg/FSVariable/FSVariable.c | 21 ++++++++++++++++++++-
>> 1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/DuetPkg/FSVariable/FSVariable.c
>> b/DuetPkg/FSVariable/FSVariable.c index 34b79305c871..6069cfa8fb98
>> 100644
>> --- a/DuetPkg/FSVariable/FSVariable.c
>> +++ b/DuetPkg/FSVariable/FSVariable.c
>> @@ -6,7 +6,7 @@ disk. They can be changed by user. BIOS is not able
>> to protoect those.
>> Duet trusts all meta data from disk. If variable code, variable
>> metadata and variable data is modified in inproper way, the behavior
>> is undefined.
>>
>> -Copyright (c) 2006 - 2016, Intel Corporation. All rights
>> reserved.<BR>
>> +Copyright (c) 2006 - 2017, Intel Corporation. All rights
>> +reserved.<BR>
>> This program and the accompanying materials are licensed and made
>> available under the terms and conditions of the BSD License which
>> accompanies this distribution. The full text of the license may be
>> found at @@ -1400,14 +1400,33 @@ Returns:
>> VARIABLE_POINTER_TRACK Variable;
>> UINTN VarNameSize;
>> EFI_STATUS Status;
>> + UINTN MaxLen;
>>
>> if (VariableNameSize == NULL || VariableName == NULL || VendorGuid ==
>> NULL) {
>> return EFI_INVALID_PARAMETER;
>> }
>>
>> + //
>> + // Calculate the possible maximum length of name string, including the Null
>> terminator.
>> + //
>> + MaxLen = *VariableNameSize / sizeof (CHAR16);
>> + if ((MaxLen == 0) ||
>> + ((VariableName[MaxLen - 1] != 0) && (StrnLenS (VariableName, MaxLen)
>> == MaxLen))) {
>> + //
>> + // Null-terminator is not found in the first VariableNameSize bytes of the
>> input VariableName buffer.
>> + //
>> + return EFI_INVALID_PARAMETER;
>> + }
>> +
>> Status = FindVariable (VariableName, VendorGuid, &Variable);
>>
>> if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
>> + if (VariableName[0] != 0) {
>> + //
>> + // The input values of VariableName and VendorGuid are not a name
>> and GUID of an existing variable.
>> + //
>> + Status = EFI_INVALID_PARAMETER;
>> + }
>> return Status;
>> }
>>
>> --
>> 2.7.0.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
2017-06-26 3:04 ` Zeng, Star
@ 2017-06-26 5:36 ` Ni, Ruiyu
2017-06-26 5:41 ` Zeng, Star
0 siblings, 1 reply; 15+ messages in thread
From: Ni, Ruiyu @ 2017-06-26 5:36 UTC (permalink / raw)
To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Gao, Liming
I understand your point.
But I do think it hurts readability.
BTW, what does the below change does?
if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
+ if (VariableName[0] != 0) {
+ //
+ // The input values of VariableName and VendorGuid are not a name and GUID of an existing variable.
+ //
+ Status = EFI_INVALID_PARAMETER;
+ }
return Status;
}
Thanks/Ray
> -----Original Message-----
> From: Zeng, Star
> Sent: Monday, June 26, 2017 11:05 AM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> GetNextVariableName to follow UEFI 2.7
>
> Ray,
>
> The code is like low hanging fruit from my practice for me, and I don't think it
> hurts readability although it may not bring performance improvement, it
> depends on how many variables in variable region, how many times of calling
> GetNextVariableName, and how fast of GetNextVariableName.
>
> The code practice I did is on NT32 and my real platforms. Is there anyone can
> make sure he/she tested all the systems in the world for their code?
>
>
> Anyway, I can update the patch if you insist.
>
>
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Saturday, June 24, 2017 10:08 AM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> GetNextVariableName to follow UEFI 2.7
>
> Star,
> I don't recommend to add the additional check for performance
> consideration.
> Because we have no idea what the input VariableName buffer is like.
> If the VariableName is like ['\0', '?', '?', '?'] with MaxLen equals to 4,
> "VariableName[MaxLen-1] != 0" check is redundant.
> The NT32 case you met cannot represent the all possible cases.
> You could use the possibility theory to decide what the most efficient way is.
>
> Additionally I think code readability is more important than efficiency.
> In this case, we need the data about the performance improvement to
> decide whether this check is necessary.
>
>
> Regards,
> Ray
>
> >-----Original Message-----
> >From: Zeng, Star
> >Sent: Friday, June 23, 2017 5:33 PM
> >To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> >Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star
> ><star.zeng@intel.com>
> >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> >GetNextVariableName to follow UEFI 2.7
> >
> >Ray,
> >
> >It is to pass the check quickly and avoid scanning all the chars in
> >VariableName by StrnLenS for normal boot without invalid cases.
> >I did experiments in the code of GetNextVariableName with below debug
> >code for normal boot on NT32 and my real platforms, all the cases will go
> into the branch "xxx 2".
> > if (((VariableName[MaxLen - 1] != 0))) {
> > DEBUG ((DEBUG_INFO, "xxx 1\n"));
> > } else {
> > DEBUG ((DEBUG_INFO, "xxx 2\n"));
> > }
> >
> >
> >Thanks,
> >Star
> >-----Original Message-----
> >From: Ni, Ruiyu
> >Sent: Friday, June 23, 2017 4:20 PM
> >To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> >Cc: Gao, Liming <liming.gao@intel.com>
> >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> >GetNextVariableName to follow UEFI 2.7
> >
> >Star,
> >What's the benefit of this check "VariableName[MaxLen - 1] != 0"?
> >I think this check "StrnLenS (VariableName, MaxLen) == MaxLen" should be
> enough.
> >
> >Thanks/Ray
> >
> >> -----Original Message-----
> >> From: Zeng, Star
> >> Sent: Friday, June 23, 2017 4:08 PM
> >> To: edk2-devel@lists.01.org
> >> Cc: Zeng, Star <star.zeng@intel.com>; Gao, Liming
> >> <liming.gao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> >> Subject: [PATCH V2 3/3] DuetPkg FsVariable: Update
> >> GetNextVariableName to follow UEFI 2.7
> >>
> >> "The size must be large enough to fit input string supplied in
> >> VariableName buffer" is added in the description for VariableNameSize.
> >> And two cases of EFI_INVALID_PARAMETER are added.
> >> 1. The input values of VariableName and VendorGuid are not a name and
> >> GUID of an existing variable.
> >> 2. Null-terminator is not found in the first VariableNameSize bytes of
> >> the input VariableName buffer.
> >>
> >> This patch is to update code to follow them.
> >>
> >> Cc: Liming Gao <liming.gao@intel.com>
> >> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Star Zeng <star.zeng@intel.com>
> >> ---
> >> DuetPkg/FSVariable/FSVariable.c | 21 ++++++++++++++++++++-
> >> 1 file changed, 20 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/DuetPkg/FSVariable/FSVariable.c
> >> b/DuetPkg/FSVariable/FSVariable.c index 34b79305c871..6069cfa8fb98
> >> 100644
> >> --- a/DuetPkg/FSVariable/FSVariable.c
> >> +++ b/DuetPkg/FSVariable/FSVariable.c
> >> @@ -6,7 +6,7 @@ disk. They can be changed by user. BIOS is not able
> >> to protoect those.
> >> Duet trusts all meta data from disk. If variable code, variable
> >> metadata and variable data is modified in inproper way, the behavior
> >> is undefined.
> >>
> >> -Copyright (c) 2006 - 2016, Intel Corporation. All rights
> >> reserved.<BR>
> >> +Copyright (c) 2006 - 2017, Intel Corporation. All rights
> >> +reserved.<BR>
> >> This program and the accompanying materials are licensed and made
> >> available under the terms and conditions of the BSD License which
> >> accompanies this distribution. The full text of the license may be
> >> found at @@ -1400,14 +1400,33 @@ Returns:
> >> VARIABLE_POINTER_TRACK Variable;
> >> UINTN VarNameSize;
> >> EFI_STATUS Status;
> >> + UINTN MaxLen;
> >>
> >> if (VariableNameSize == NULL || VariableName == NULL || VendorGuid
> >> ==
> >> NULL) {
> >> return EFI_INVALID_PARAMETER;
> >> }
> >>
> >> + //
> >> + // Calculate the possible maximum length of name string, including
> >> + the Null
> >> terminator.
> >> + //
> >> + MaxLen = *VariableNameSize / sizeof (CHAR16); if ((MaxLen == 0)
> >> + ||
> >> + ((VariableName[MaxLen - 1] != 0) && (StrnLenS (VariableName,
> >> + MaxLen)
> >> == MaxLen))) {
> >> + //
> >> + // Null-terminator is not found in the first VariableNameSize
> >> + bytes of the
> >> input VariableName buffer.
> >> + //
> >> + return EFI_INVALID_PARAMETER;
> >> + }
> >> +
> >> Status = FindVariable (VariableName, VendorGuid, &Variable);
> >>
> >> if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
> >> + if (VariableName[0] != 0) {
> >> + //
> >> + // The input values of VariableName and VendorGuid are not a
> >> + name
> >> and GUID of an existing variable.
> >> + //
> >> + Status = EFI_INVALID_PARAMETER;
> >> + }
> >> return Status;
> >> }
> >>
> >> --
> >> 2.7.0.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
2017-06-26 5:36 ` Ni, Ruiyu
@ 2017-06-26 5:41 ` Zeng, Star
2017-06-26 5:46 ` Ni, Ruiyu
0 siblings, 1 reply; 15+ messages in thread
From: Zeng, Star @ 2017-06-26 5:41 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Gao, Liming, Zeng, Star
It is to return EFI_INVALID_PARAMETER when the input VariableName and VendorGuid are not a valid variable to search next variable.
It is added from UEFI 2.7 spec.
Before the spec change, the code is to return EFI_NOT_FOUND at that case.
After the spec change, EFI_NOT_FOUND seemingly is reserved to indicate the ending of searching.
Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu
Sent: Monday, June 26, 2017 1:37 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Gao, Liming <liming.gao@intel.com>
Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
I understand your point.
But I do think it hurts readability.
BTW, what does the below change does?
if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
+ if (VariableName[0] != 0) {
+ //
+ // The input values of VariableName and VendorGuid are not a name and GUID of an existing variable.
+ //
+ Status = EFI_INVALID_PARAMETER;
+ }
return Status;
}
Thanks/Ray
> -----Original Message-----
> From: Zeng, Star
> Sent: Monday, June 26, 2017 11:05 AM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> GetNextVariableName to follow UEFI 2.7
>
> Ray,
>
> The code is like low hanging fruit from my practice for me, and I
> don't think it hurts readability although it may not bring performance
> improvement, it depends on how many variables in variable region, how
> many times of calling GetNextVariableName, and how fast of GetNextVariableName.
>
> The code practice I did is on NT32 and my real platforms. Is there
> anyone can make sure he/she tested all the systems in the world for their code?
>
>
> Anyway, I can update the patch if you insist.
>
>
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Saturday, June 24, 2017 10:08 AM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> GetNextVariableName to follow UEFI 2.7
>
> Star,
> I don't recommend to add the additional check for performance
> consideration.
> Because we have no idea what the input VariableName buffer is like.
> If the VariableName is like ['\0', '?', '?', '?'] with MaxLen equals
> to 4, "VariableName[MaxLen-1] != 0" check is redundant.
> The NT32 case you met cannot represent the all possible cases.
> You could use the possibility theory to decide what the most efficient way is.
>
> Additionally I think code readability is more important than efficiency.
> In this case, we need the data about the performance improvement to
> decide whether this check is necessary.
>
>
> Regards,
> Ray
>
> >-----Original Message-----
> >From: Zeng, Star
> >Sent: Friday, June 23, 2017 5:33 PM
> >To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> >Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star
> ><star.zeng@intel.com>
> >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> >GetNextVariableName to follow UEFI 2.7
> >
> >Ray,
> >
> >It is to pass the check quickly and avoid scanning all the chars in
> >VariableName by StrnLenS for normal boot without invalid cases.
> >I did experiments in the code of GetNextVariableName with below debug
> >code for normal boot on NT32 and my real platforms, all the cases
> >will go
> into the branch "xxx 2".
> > if (((VariableName[MaxLen - 1] != 0))) {
> > DEBUG ((DEBUG_INFO, "xxx 1\n"));
> > } else {
> > DEBUG ((DEBUG_INFO, "xxx 2\n"));
> > }
> >
> >
> >Thanks,
> >Star
> >-----Original Message-----
> >From: Ni, Ruiyu
> >Sent: Friday, June 23, 2017 4:20 PM
> >To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> >Cc: Gao, Liming <liming.gao@intel.com>
> >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> >GetNextVariableName to follow UEFI 2.7
> >
> >Star,
> >What's the benefit of this check "VariableName[MaxLen - 1] != 0"?
> >I think this check "StrnLenS (VariableName, MaxLen) == MaxLen" should
> >be
> enough.
> >
> >Thanks/Ray
> >
> >> -----Original Message-----
> >> From: Zeng, Star
> >> Sent: Friday, June 23, 2017 4:08 PM
> >> To: edk2-devel@lists.01.org
> >> Cc: Zeng, Star <star.zeng@intel.com>; Gao, Liming
> >> <liming.gao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> >> Subject: [PATCH V2 3/3] DuetPkg FsVariable: Update
> >> GetNextVariableName to follow UEFI 2.7
> >>
> >> "The size must be large enough to fit input string supplied in
> >> VariableName buffer" is added in the description for VariableNameSize.
> >> And two cases of EFI_INVALID_PARAMETER are added.
> >> 1. The input values of VariableName and VendorGuid are not a name and
> >> GUID of an existing variable.
> >> 2. Null-terminator is not found in the first VariableNameSize bytes of
> >> the input VariableName buffer.
> >>
> >> This patch is to update code to follow them.
> >>
> >> Cc: Liming Gao <liming.gao@intel.com>
> >> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Star Zeng <star.zeng@intel.com>
> >> ---
> >> DuetPkg/FSVariable/FSVariable.c | 21 ++++++++++++++++++++-
> >> 1 file changed, 20 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/DuetPkg/FSVariable/FSVariable.c
> >> b/DuetPkg/FSVariable/FSVariable.c index 34b79305c871..6069cfa8fb98
> >> 100644
> >> --- a/DuetPkg/FSVariable/FSVariable.c
> >> +++ b/DuetPkg/FSVariable/FSVariable.c
> >> @@ -6,7 +6,7 @@ disk. They can be changed by user. BIOS is not able
> >> to protoect those.
> >> Duet trusts all meta data from disk. If variable code, variable
> >> metadata and variable data is modified in inproper way, the
> >> behavior is undefined.
> >>
> >> -Copyright (c) 2006 - 2016, Intel Corporation. All rights
> >> reserved.<BR>
> >> +Copyright (c) 2006 - 2017, Intel Corporation. All rights
> >> +reserved.<BR>
> >> This program and the accompanying materials are licensed and made
> >> available under the terms and conditions of the BSD License which
> >> accompanies this distribution. The full text of the license may be
> >> found at @@ -1400,14 +1400,33 @@ Returns:
> >> VARIABLE_POINTER_TRACK Variable;
> >> UINTN VarNameSize;
> >> EFI_STATUS Status;
> >> + UINTN MaxLen;
> >>
> >> if (VariableNameSize == NULL || VariableName == NULL ||
> >> VendorGuid ==
> >> NULL) {
> >> return EFI_INVALID_PARAMETER;
> >> }
> >>
> >> + //
> >> + // Calculate the possible maximum length of name string,
> >> + including the Null
> >> terminator.
> >> + //
> >> + MaxLen = *VariableNameSize / sizeof (CHAR16); if ((MaxLen == 0)
> >> + ||
> >> + ((VariableName[MaxLen - 1] != 0) && (StrnLenS (VariableName,
> >> + MaxLen)
> >> == MaxLen))) {
> >> + //
> >> + // Null-terminator is not found in the first VariableNameSize
> >> + bytes of the
> >> input VariableName buffer.
> >> + //
> >> + return EFI_INVALID_PARAMETER;
> >> + }
> >> +
> >> Status = FindVariable (VariableName, VendorGuid, &Variable);
> >>
> >> if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
> >> + if (VariableName[0] != 0) {
> >> + //
> >> + // The input values of VariableName and VendorGuid are not a
> >> + name
> >> and GUID of an existing variable.
> >> + //
> >> + Status = EFI_INVALID_PARAMETER;
> >> + }
> >> return Status;
> >> }
> >>
> >> --
> >> 2.7.0.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
2017-06-26 5:41 ` Zeng, Star
@ 2017-06-26 5:46 ` Ni, Ruiyu
2017-06-26 5:52 ` Zeng, Star
0 siblings, 1 reply; 15+ messages in thread
From: Ni, Ruiyu @ 2017-06-26 5:46 UTC (permalink / raw)
To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Gao, Liming
Can you add more comments here to describe the purpose is to change the return status
from Not Found to Invalid Parameter, and the reason of choosing Invalid Parameter?
Thanks/Ray
> -----Original Message-----
> From: Zeng, Star
> Sent: Monday, June 26, 2017 1:41 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> GetNextVariableName to follow UEFI 2.7
>
> It is to return EFI_INVALID_PARAMETER when the input VariableName and
> VendorGuid are not a valid variable to search next variable.
> It is added from UEFI 2.7 spec.
> Before the spec change, the code is to return EFI_NOT_FOUND at that case.
> After the spec change, EFI_NOT_FOUND seemingly is reserved to indicate
> the ending of searching.
>
>
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Monday, June 26, 2017 1:37 PM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> GetNextVariableName to follow UEFI 2.7
>
> I understand your point.
> But I do think it hurts readability.
>
> BTW, what does the below change does?
> if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
> + if (VariableName[0] != 0) {
> + //
> + // The input values of VariableName and VendorGuid are not a name
> and GUID of an existing variable.
> + //
> + Status = EFI_INVALID_PARAMETER;
> + }
> return Status;
> }
>
>
> Thanks/Ray
>
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Monday, June 26, 2017 11:05 AM
> > To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> > Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star
> > <star.zeng@intel.com>
> > Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > GetNextVariableName to follow UEFI 2.7
> >
> > Ray,
> >
> > The code is like low hanging fruit from my practice for me, and I
> > don't think it hurts readability although it may not bring performance
> > improvement, it depends on how many variables in variable region, how
> > many times of calling GetNextVariableName, and how fast of
> GetNextVariableName.
> >
> > The code practice I did is on NT32 and my real platforms. Is there
> > anyone can make sure he/she tested all the systems in the world for their
> code?
> >
> >
> > Anyway, I can update the patch if you insist.
> >
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Ni, Ruiyu
> > Sent: Saturday, June 24, 2017 10:08 AM
> > To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> > Cc: Gao, Liming <liming.gao@intel.com>
> > Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > GetNextVariableName to follow UEFI 2.7
> >
> > Star,
> > I don't recommend to add the additional check for performance
> > consideration.
> > Because we have no idea what the input VariableName buffer is like.
> > If the VariableName is like ['\0', '?', '?', '?'] with MaxLen equals
> > to 4, "VariableName[MaxLen-1] != 0" check is redundant.
> > The NT32 case you met cannot represent the all possible cases.
> > You could use the possibility theory to decide what the most efficient way
> is.
> >
> > Additionally I think code readability is more important than efficiency.
> > In this case, we need the data about the performance improvement to
> > decide whether this check is necessary.
> >
> >
> > Regards,
> > Ray
> >
> > >-----Original Message-----
> > >From: Zeng, Star
> > >Sent: Friday, June 23, 2017 5:33 PM
> > >To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> > >Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star
> > ><star.zeng@intel.com>
> > >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > >GetNextVariableName to follow UEFI 2.7
> > >
> > >Ray,
> > >
> > >It is to pass the check quickly and avoid scanning all the chars in
> > >VariableName by StrnLenS for normal boot without invalid cases.
> > >I did experiments in the code of GetNextVariableName with below debug
> > >code for normal boot on NT32 and my real platforms, all the cases
> > >will go
> > into the branch "xxx 2".
> > > if (((VariableName[MaxLen - 1] != 0))) {
> > > DEBUG ((DEBUG_INFO, "xxx 1\n"));
> > > } else {
> > > DEBUG ((DEBUG_INFO, "xxx 2\n"));
> > > }
> > >
> > >
> > >Thanks,
> > >Star
> > >-----Original Message-----
> > >From: Ni, Ruiyu
> > >Sent: Friday, June 23, 2017 4:20 PM
> > >To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> > >Cc: Gao, Liming <liming.gao@intel.com>
> > >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > >GetNextVariableName to follow UEFI 2.7
> > >
> > >Star,
> > >What's the benefit of this check "VariableName[MaxLen - 1] != 0"?
> > >I think this check "StrnLenS (VariableName, MaxLen) == MaxLen" should
> > >be
> > enough.
> > >
> > >Thanks/Ray
> > >
> > >> -----Original Message-----
> > >> From: Zeng, Star
> > >> Sent: Friday, June 23, 2017 4:08 PM
> > >> To: edk2-devel@lists.01.org
> > >> Cc: Zeng, Star <star.zeng@intel.com>; Gao, Liming
> > >> <liming.gao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> > >> Subject: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > >> GetNextVariableName to follow UEFI 2.7
> > >>
> > >> "The size must be large enough to fit input string supplied in
> > >> VariableName buffer" is added in the description for VariableNameSize.
> > >> And two cases of EFI_INVALID_PARAMETER are added.
> > >> 1. The input values of VariableName and VendorGuid are not a name
> and
> > >> GUID of an existing variable.
> > >> 2. Null-terminator is not found in the first VariableNameSize bytes of
> > >> the input VariableName buffer.
> > >>
> > >> This patch is to update code to follow them.
> > >>
> > >> Cc: Liming Gao <liming.gao@intel.com>
> > >> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > >> Contributed-under: TianoCore Contribution Agreement 1.0
> > >> Signed-off-by: Star Zeng <star.zeng@intel.com>
> > >> ---
> > >> DuetPkg/FSVariable/FSVariable.c | 21 ++++++++++++++++++++-
> > >> 1 file changed, 20 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/DuetPkg/FSVariable/FSVariable.c
> > >> b/DuetPkg/FSVariable/FSVariable.c index 34b79305c871..6069cfa8fb98
> > >> 100644
> > >> --- a/DuetPkg/FSVariable/FSVariable.c
> > >> +++ b/DuetPkg/FSVariable/FSVariable.c
> > >> @@ -6,7 +6,7 @@ disk. They can be changed by user. BIOS is not able
> > >> to protoect those.
> > >> Duet trusts all meta data from disk. If variable code, variable
> > >> metadata and variable data is modified in inproper way, the
> > >> behavior is undefined.
> > >>
> > >> -Copyright (c) 2006 - 2016, Intel Corporation. All rights
> > >> reserved.<BR>
> > >> +Copyright (c) 2006 - 2017, Intel Corporation. All rights
> > >> +reserved.<BR>
> > >> This program and the accompanying materials are licensed and made
> > >> available under the terms and conditions of the BSD License which
> > >> accompanies this distribution. The full text of the license may be
> > >> found at @@ -1400,14 +1400,33 @@ Returns:
> > >> VARIABLE_POINTER_TRACK Variable;
> > >> UINTN VarNameSize;
> > >> EFI_STATUS Status;
> > >> + UINTN MaxLen;
> > >>
> > >> if (VariableNameSize == NULL || VariableName == NULL ||
> > >> VendorGuid ==
> > >> NULL) {
> > >> return EFI_INVALID_PARAMETER;
> > >> }
> > >>
> > >> + //
> > >> + // Calculate the possible maximum length of name string,
> > >> + including the Null
> > >> terminator.
> > >> + //
> > >> + MaxLen = *VariableNameSize / sizeof (CHAR16); if ((MaxLen == 0)
> > >> + ||
> > >> + ((VariableName[MaxLen - 1] != 0) && (StrnLenS (VariableName,
> > >> + MaxLen)
> > >> == MaxLen))) {
> > >> + //
> > >> + // Null-terminator is not found in the first VariableNameSize
> > >> + bytes of the
> > >> input VariableName buffer.
> > >> + //
> > >> + return EFI_INVALID_PARAMETER;
> > >> + }
> > >> +
> > >> Status = FindVariable (VariableName, VendorGuid, &Variable);
> > >>
> > >> if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
> > >> + if (VariableName[0] != 0) {
> > >> + //
> > >> + // The input values of VariableName and VendorGuid are not a
> > >> + name
> > >> and GUID of an existing variable.
> > >> + //
> > >> + Status = EFI_INVALID_PARAMETER;
> > >> + }
> > >> return Status;
> > >> }
> > >>
> > >> --
> > >> 2.7.0.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
2017-06-26 5:46 ` Ni, Ruiyu
@ 2017-06-26 5:52 ` Zeng, Star
2017-06-26 6:18 ` Ni, Ruiyu
0 siblings, 1 reply; 15+ messages in thread
From: Zeng, Star @ 2017-06-26 5:52 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Gao, Liming, Zeng, Star
Before UEFI 2.6a and 2.7, the behavior is unpredictable, our *CODE* chose to return EFI_NOT_FOUND.
"Passing in a VariableName parameter that is neither a Null-terminated string nor a value that was returned on the previous call to
GetNextVariableName() may also produce unpredictable results."
Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu
Sent: Monday, June 26, 2017 1:47 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Gao, Liming <liming.gao@intel.com>
Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
Can you add more comments here to describe the purpose is to change the return status from Not Found to Invalid Parameter, and the reason of choosing Invalid Parameter?
Thanks/Ray
> -----Original Message-----
> From: Zeng, Star
> Sent: Monday, June 26, 2017 1:41 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> GetNextVariableName to follow UEFI 2.7
>
> It is to return EFI_INVALID_PARAMETER when the input VariableName and
> VendorGuid are not a valid variable to search next variable.
> It is added from UEFI 2.7 spec.
> Before the spec change, the code is to return EFI_NOT_FOUND at that case.
> After the spec change, EFI_NOT_FOUND seemingly is reserved to indicate
> the ending of searching.
>
>
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Monday, June 26, 2017 1:37 PM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> GetNextVariableName to follow UEFI 2.7
>
> I understand your point.
> But I do think it hurts readability.
>
> BTW, what does the below change does?
> if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
> + if (VariableName[0] != 0) {
> + //
> + // The input values of VariableName and VendorGuid are not a
> + name
> and GUID of an existing variable.
> + //
> + Status = EFI_INVALID_PARAMETER;
> + }
> return Status;
> }
>
>
> Thanks/Ray
>
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Monday, June 26, 2017 11:05 AM
> > To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> > Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star
> > <star.zeng@intel.com>
> > Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > GetNextVariableName to follow UEFI 2.7
> >
> > Ray,
> >
> > The code is like low hanging fruit from my practice for me, and I
> > don't think it hurts readability although it may not bring
> > performance improvement, it depends on how many variables in
> > variable region, how many times of calling GetNextVariableName, and
> > how fast of
> GetNextVariableName.
> >
> > The code practice I did is on NT32 and my real platforms. Is there
> > anyone can make sure he/she tested all the systems in the world for
> > their
> code?
> >
> >
> > Anyway, I can update the patch if you insist.
> >
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Ni, Ruiyu
> > Sent: Saturday, June 24, 2017 10:08 AM
> > To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> > Cc: Gao, Liming <liming.gao@intel.com>
> > Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > GetNextVariableName to follow UEFI 2.7
> >
> > Star,
> > I don't recommend to add the additional check for performance
> > consideration.
> > Because we have no idea what the input VariableName buffer is like.
> > If the VariableName is like ['\0', '?', '?', '?'] with MaxLen equals
> > to 4, "VariableName[MaxLen-1] != 0" check is redundant.
> > The NT32 case you met cannot represent the all possible cases.
> > You could use the possibility theory to decide what the most
> > efficient way
> is.
> >
> > Additionally I think code readability is more important than efficiency.
> > In this case, we need the data about the performance improvement to
> > decide whether this check is necessary.
> >
> >
> > Regards,
> > Ray
> >
> > >-----Original Message-----
> > >From: Zeng, Star
> > >Sent: Friday, June 23, 2017 5:33 PM
> > >To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> > >Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star
> > ><star.zeng@intel.com>
> > >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > >GetNextVariableName to follow UEFI 2.7
> > >
> > >Ray,
> > >
> > >It is to pass the check quickly and avoid scanning all the chars in
> > >VariableName by StrnLenS for normal boot without invalid cases.
> > >I did experiments in the code of GetNextVariableName with below
> > >debug code for normal boot on NT32 and my real platforms, all the
> > >cases will go
> > into the branch "xxx 2".
> > > if (((VariableName[MaxLen - 1] != 0))) {
> > > DEBUG ((DEBUG_INFO, "xxx 1\n")); } else {
> > > DEBUG ((DEBUG_INFO, "xxx 2\n")); }
> > >
> > >
> > >Thanks,
> > >Star
> > >-----Original Message-----
> > >From: Ni, Ruiyu
> > >Sent: Friday, June 23, 2017 4:20 PM
> > >To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> > >Cc: Gao, Liming <liming.gao@intel.com>
> > >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > >GetNextVariableName to follow UEFI 2.7
> > >
> > >Star,
> > >What's the benefit of this check "VariableName[MaxLen - 1] != 0"?
> > >I think this check "StrnLenS (VariableName, MaxLen) == MaxLen"
> > >should be
> > enough.
> > >
> > >Thanks/Ray
> > >
> > >> -----Original Message-----
> > >> From: Zeng, Star
> > >> Sent: Friday, June 23, 2017 4:08 PM
> > >> To: edk2-devel@lists.01.org
> > >> Cc: Zeng, Star <star.zeng@intel.com>; Gao, Liming
> > >> <liming.gao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> > >> Subject: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > >> GetNextVariableName to follow UEFI 2.7
> > >>
> > >> "The size must be large enough to fit input string supplied in
> > >> VariableName buffer" is added in the description for VariableNameSize.
> > >> And two cases of EFI_INVALID_PARAMETER are added.
> > >> 1. The input values of VariableName and VendorGuid are not a name
> and
> > >> GUID of an existing variable.
> > >> 2. Null-terminator is not found in the first VariableNameSize bytes of
> > >> the input VariableName buffer.
> > >>
> > >> This patch is to update code to follow them.
> > >>
> > >> Cc: Liming Gao <liming.gao@intel.com>
> > >> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > >> Contributed-under: TianoCore Contribution Agreement 1.0
> > >> Signed-off-by: Star Zeng <star.zeng@intel.com>
> > >> ---
> > >> DuetPkg/FSVariable/FSVariable.c | 21 ++++++++++++++++++++-
> > >> 1 file changed, 20 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/DuetPkg/FSVariable/FSVariable.c
> > >> b/DuetPkg/FSVariable/FSVariable.c index
> > >> 34b79305c871..6069cfa8fb98
> > >> 100644
> > >> --- a/DuetPkg/FSVariable/FSVariable.c
> > >> +++ b/DuetPkg/FSVariable/FSVariable.c
> > >> @@ -6,7 +6,7 @@ disk. They can be changed by user. BIOS is not
> > >> able to protoect those.
> > >> Duet trusts all meta data from disk. If variable code, variable
> > >> metadata and variable data is modified in inproper way, the
> > >> behavior is undefined.
> > >>
> > >> -Copyright (c) 2006 - 2016, Intel Corporation. All rights
> > >> reserved.<BR>
> > >> +Copyright (c) 2006 - 2017, Intel Corporation. All rights
> > >> +reserved.<BR>
> > >> This program and the accompanying materials are licensed and
> > >> made available under the terms and conditions of the BSD License
> > >> which accompanies this distribution. The full text of the
> > >> license may be found at @@ -1400,14 +1400,33 @@ Returns:
> > >> VARIABLE_POINTER_TRACK Variable;
> > >> UINTN VarNameSize;
> > >> EFI_STATUS Status;
> > >> + UINTN MaxLen;
> > >>
> > >> if (VariableNameSize == NULL || VariableName == NULL ||
> > >> VendorGuid ==
> > >> NULL) {
> > >> return EFI_INVALID_PARAMETER;
> > >> }
> > >>
> > >> + //
> > >> + // Calculate the possible maximum length of name string,
> > >> + including the Null
> > >> terminator.
> > >> + //
> > >> + MaxLen = *VariableNameSize / sizeof (CHAR16); if ((MaxLen ==
> > >> + 0)
> > >> + ||
> > >> + ((VariableName[MaxLen - 1] != 0) && (StrnLenS
> > >> + (VariableName,
> > >> + MaxLen)
> > >> == MaxLen))) {
> > >> + //
> > >> + // Null-terminator is not found in the first
> > >> + VariableNameSize bytes of the
> > >> input VariableName buffer.
> > >> + //
> > >> + return EFI_INVALID_PARAMETER; }
> > >> +
> > >> Status = FindVariable (VariableName, VendorGuid, &Variable);
> > >>
> > >> if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
> > >> + if (VariableName[0] != 0) {
> > >> + //
> > >> + // The input values of VariableName and VendorGuid are not
> > >> + a name
> > >> and GUID of an existing variable.
> > >> + //
> > >> + Status = EFI_INVALID_PARAMETER;
> > >> + }
> > >> return Status;
> > >> }
> > >>
> > >> --
> > >> 2.7.0.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
2017-06-26 5:52 ` Zeng, Star
@ 2017-06-26 6:18 ` Ni, Ruiyu
2017-06-26 6:31 ` Zeng, Star
0 siblings, 1 reply; 15+ messages in thread
From: Ni, Ruiyu @ 2017-06-26 6:18 UTC (permalink / raw)
To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Gao, Liming
Thanks! Could you please put the comments in code?
But why do you say it's unpredictable? The behavior is to return EFI_NOT_FOUND.
Thanks/Ray
> -----Original Message-----
> From: Zeng, Star
> Sent: Monday, June 26, 2017 1:52 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> GetNextVariableName to follow UEFI 2.7
>
> Before UEFI 2.6a and 2.7, the behavior is unpredictable, our *CODE* chose to
> return EFI_NOT_FOUND.
>
> "Passing in a VariableName parameter that is neither a Null-terminated string
> nor a value that was returned on the previous call to
> GetNextVariableName() may also produce unpredictable results."
>
>
>
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Monday, June 26, 2017 1:47 PM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> GetNextVariableName to follow UEFI 2.7
>
> Can you add more comments here to describe the purpose is to change the
> return status from Not Found to Invalid Parameter, and the reason of
> choosing Invalid Parameter?
>
> Thanks/Ray
>
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Monday, June 26, 2017 1:41 PM
> > To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> > Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star
> > <star.zeng@intel.com>
> > Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > GetNextVariableName to follow UEFI 2.7
> >
> > It is to return EFI_INVALID_PARAMETER when the input VariableName and
> > VendorGuid are not a valid variable to search next variable.
> > It is added from UEFI 2.7 spec.
> > Before the spec change, the code is to return EFI_NOT_FOUND at that case.
> > After the spec change, EFI_NOT_FOUND seemingly is reserved to indicate
> > the ending of searching.
> >
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Ni, Ruiyu
> > Sent: Monday, June 26, 2017 1:37 PM
> > To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> > Cc: Gao, Liming <liming.gao@intel.com>
> > Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > GetNextVariableName to follow UEFI 2.7
> >
> > I understand your point.
> > But I do think it hurts readability.
> >
> > BTW, what does the below change does?
> > if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
> > + if (VariableName[0] != 0) {
> > + //
> > + // The input values of VariableName and VendorGuid are not a
> > + name
> > and GUID of an existing variable.
> > + //
> > + Status = EFI_INVALID_PARAMETER;
> > + }
> > return Status;
> > }
> >
> >
> > Thanks/Ray
> >
> > > -----Original Message-----
> > > From: Zeng, Star
> > > Sent: Monday, June 26, 2017 11:05 AM
> > > To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> > > Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star
> > > <star.zeng@intel.com>
> > > Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > > GetNextVariableName to follow UEFI 2.7
> > >
> > > Ray,
> > >
> > > The code is like low hanging fruit from my practice for me, and I
> > > don't think it hurts readability although it may not bring
> > > performance improvement, it depends on how many variables in
> > > variable region, how many times of calling GetNextVariableName, and
> > > how fast of
> > GetNextVariableName.
> > >
> > > The code practice I did is on NT32 and my real platforms. Is there
> > > anyone can make sure he/she tested all the systems in the world for
> > > their
> > code?
> > >
> > >
> > > Anyway, I can update the patch if you insist.
> > >
> > >
> > > Thanks,
> > > Star
> > > -----Original Message-----
> > > From: Ni, Ruiyu
> > > Sent: Saturday, June 24, 2017 10:08 AM
> > > To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> > > Cc: Gao, Liming <liming.gao@intel.com>
> > > Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > > GetNextVariableName to follow UEFI 2.7
> > >
> > > Star,
> > > I don't recommend to add the additional check for performance
> > > consideration.
> > > Because we have no idea what the input VariableName buffer is like.
> > > If the VariableName is like ['\0', '?', '?', '?'] with MaxLen equals
> > > to 4, "VariableName[MaxLen-1] != 0" check is redundant.
> > > The NT32 case you met cannot represent the all possible cases.
> > > You could use the possibility theory to decide what the most
> > > efficient way
> > is.
> > >
> > > Additionally I think code readability is more important than efficiency.
> > > In this case, we need the data about the performance improvement to
> > > decide whether this check is necessary.
> > >
> > >
> > > Regards,
> > > Ray
> > >
> > > >-----Original Message-----
> > > >From: Zeng, Star
> > > >Sent: Friday, June 23, 2017 5:33 PM
> > > >To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> > > >Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star
> > > ><star.zeng@intel.com>
> > > >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > > >GetNextVariableName to follow UEFI 2.7
> > > >
> > > >Ray,
> > > >
> > > >It is to pass the check quickly and avoid scanning all the chars in
> > > >VariableName by StrnLenS for normal boot without invalid cases.
> > > >I did experiments in the code of GetNextVariableName with below
> > > >debug code for normal boot on NT32 and my real platforms, all the
> > > >cases will go
> > > into the branch "xxx 2".
> > > > if (((VariableName[MaxLen - 1] != 0))) {
> > > > DEBUG ((DEBUG_INFO, "xxx 1\n")); } else {
> > > > DEBUG ((DEBUG_INFO, "xxx 2\n")); }
> > > >
> > > >
> > > >Thanks,
> > > >Star
> > > >-----Original Message-----
> > > >From: Ni, Ruiyu
> > > >Sent: Friday, June 23, 2017 4:20 PM
> > > >To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> > > >Cc: Gao, Liming <liming.gao@intel.com>
> > > >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > > >GetNextVariableName to follow UEFI 2.7
> > > >
> > > >Star,
> > > >What's the benefit of this check "VariableName[MaxLen - 1] != 0"?
> > > >I think this check "StrnLenS (VariableName, MaxLen) == MaxLen"
> > > >should be
> > > enough.
> > > >
> > > >Thanks/Ray
> > > >
> > > >> -----Original Message-----
> > > >> From: Zeng, Star
> > > >> Sent: Friday, June 23, 2017 4:08 PM
> > > >> To: edk2-devel@lists.01.org
> > > >> Cc: Zeng, Star <star.zeng@intel.com>; Gao, Liming
> > > >> <liming.gao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> > > >> Subject: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > > >> GetNextVariableName to follow UEFI 2.7
> > > >>
> > > >> "The size must be large enough to fit input string supplied in
> > > >> VariableName buffer" is added in the description for
> VariableNameSize.
> > > >> And two cases of EFI_INVALID_PARAMETER are added.
> > > >> 1. The input values of VariableName and VendorGuid are not a name
> > and
> > > >> GUID of an existing variable.
> > > >> 2. Null-terminator is not found in the first VariableNameSize bytes of
> > > >> the input VariableName buffer.
> > > >>
> > > >> This patch is to update code to follow them.
> > > >>
> > > >> Cc: Liming Gao <liming.gao@intel.com>
> > > >> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > > >> Contributed-under: TianoCore Contribution Agreement 1.0
> > > >> Signed-off-by: Star Zeng <star.zeng@intel.com>
> > > >> ---
> > > >> DuetPkg/FSVariable/FSVariable.c | 21 ++++++++++++++++++++-
> > > >> 1 file changed, 20 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/DuetPkg/FSVariable/FSVariable.c
> > > >> b/DuetPkg/FSVariable/FSVariable.c index
> > > >> 34b79305c871..6069cfa8fb98
> > > >> 100644
> > > >> --- a/DuetPkg/FSVariable/FSVariable.c
> > > >> +++ b/DuetPkg/FSVariable/FSVariable.c
> > > >> @@ -6,7 +6,7 @@ disk. They can be changed by user. BIOS is not
> > > >> able to protoect those.
> > > >> Duet trusts all meta data from disk. If variable code, variable
> > > >> metadata and variable data is modified in inproper way, the
> > > >> behavior is undefined.
> > > >>
> > > >> -Copyright (c) 2006 - 2016, Intel Corporation. All rights
> > > >> reserved.<BR>
> > > >> +Copyright (c) 2006 - 2017, Intel Corporation. All rights
> > > >> +reserved.<BR>
> > > >> This program and the accompanying materials are licensed and
> > > >> made available under the terms and conditions of the BSD License
> > > >> which accompanies this distribution. The full text of the
> > > >> license may be found at @@ -1400,14 +1400,33 @@ Returns:
> > > >> VARIABLE_POINTER_TRACK Variable;
> > > >> UINTN VarNameSize;
> > > >> EFI_STATUS Status;
> > > >> + UINTN MaxLen;
> > > >>
> > > >> if (VariableNameSize == NULL || VariableName == NULL ||
> > > >> VendorGuid ==
> > > >> NULL) {
> > > >> return EFI_INVALID_PARAMETER;
> > > >> }
> > > >>
> > > >> + //
> > > >> + // Calculate the possible maximum length of name string,
> > > >> + including the Null
> > > >> terminator.
> > > >> + //
> > > >> + MaxLen = *VariableNameSize / sizeof (CHAR16); if ((MaxLen ==
> > > >> + 0)
> > > >> + ||
> > > >> + ((VariableName[MaxLen - 1] != 0) && (StrnLenS
> > > >> + (VariableName,
> > > >> + MaxLen)
> > > >> == MaxLen))) {
> > > >> + //
> > > >> + // Null-terminator is not found in the first
> > > >> + VariableNameSize bytes of the
> > > >> input VariableName buffer.
> > > >> + //
> > > >> + return EFI_INVALID_PARAMETER; }
> > > >> +
> > > >> Status = FindVariable (VariableName, VendorGuid, &Variable);
> > > >>
> > > >> if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
> > > >> + if (VariableName[0] != 0) {
> > > >> + //
> > > >> + // The input values of VariableName and VendorGuid are not
> > > >> + a name
> > > >> and GUID of an existing variable.
> > > >> + //
> > > >> + Status = EFI_INVALID_PARAMETER;
> > > >> + }
> > > >> return Status;
> > > >> }
> > > >>
> > > >> --
> > > >> 2.7.0.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
2017-06-26 6:18 ` Ni, Ruiyu
@ 2017-06-26 6:31 ` Zeng, Star
0 siblings, 0 replies; 15+ messages in thread
From: Zeng, Star @ 2017-06-26 6:31 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Gao, Liming, Zeng, Star
I meant
the behavior by the spec was unpredictable.
"Passing in a VariableName parameter that is neither a Null-terminated
string nor a value that was returned on the previous call to
GetNextVariableName() may also produce *unpredictable* results."
The behavior by the code was to return EFI_NOT_FOUND, it was our code's implementation choice.
Do you mean which piece of comments to be put in code? :)
Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu
Sent: Monday, June 26, 2017 2:18 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Gao, Liming <liming.gao@intel.com>
Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7
Thanks! Could you please put the comments in code?
But why do you say it's unpredictable? The behavior is to return EFI_NOT_FOUND.
Thanks/Ray
> -----Original Message-----
> From: Zeng, Star
> Sent: Monday, June 26, 2017 1:52 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> GetNextVariableName to follow UEFI 2.7
>
> Before UEFI 2.6a and 2.7, the behavior is unpredictable, our *CODE*
> chose to return EFI_NOT_FOUND.
>
> "Passing in a VariableName parameter that is neither a Null-terminated
> string nor a value that was returned on the previous call to
> GetNextVariableName() may also produce unpredictable results."
>
>
>
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Monday, June 26, 2017 1:47 PM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> GetNextVariableName to follow UEFI 2.7
>
> Can you add more comments here to describe the purpose is to change
> the return status from Not Found to Invalid Parameter, and the reason
> of choosing Invalid Parameter?
>
> Thanks/Ray
>
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Monday, June 26, 2017 1:41 PM
> > To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> > Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star
> > <star.zeng@intel.com>
> > Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > GetNextVariableName to follow UEFI 2.7
> >
> > It is to return EFI_INVALID_PARAMETER when the input VariableName
> > and VendorGuid are not a valid variable to search next variable.
> > It is added from UEFI 2.7 spec.
> > Before the spec change, the code is to return EFI_NOT_FOUND at that case.
> > After the spec change, EFI_NOT_FOUND seemingly is reserved to
> > indicate the ending of searching.
> >
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Ni, Ruiyu
> > Sent: Monday, June 26, 2017 1:37 PM
> > To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> > Cc: Gao, Liming <liming.gao@intel.com>
> > Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > GetNextVariableName to follow UEFI 2.7
> >
> > I understand your point.
> > But I do think it hurts readability.
> >
> > BTW, what does the below change does?
> > if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
> > + if (VariableName[0] != 0) {
> > + //
> > + // The input values of VariableName and VendorGuid are not a
> > + name
> > and GUID of an existing variable.
> > + //
> > + Status = EFI_INVALID_PARAMETER;
> > + }
> > return Status;
> > }
> >
> >
> > Thanks/Ray
> >
> > > -----Original Message-----
> > > From: Zeng, Star
> > > Sent: Monday, June 26, 2017 11:05 AM
> > > To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> > > Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star
> > > <star.zeng@intel.com>
> > > Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > > GetNextVariableName to follow UEFI 2.7
> > >
> > > Ray,
> > >
> > > The code is like low hanging fruit from my practice for me, and I
> > > don't think it hurts readability although it may not bring
> > > performance improvement, it depends on how many variables in
> > > variable region, how many times of calling GetNextVariableName,
> > > and how fast of
> > GetNextVariableName.
> > >
> > > The code practice I did is on NT32 and my real platforms. Is there
> > > anyone can make sure he/she tested all the systems in the world
> > > for their
> > code?
> > >
> > >
> > > Anyway, I can update the patch if you insist.
> > >
> > >
> > > Thanks,
> > > Star
> > > -----Original Message-----
> > > From: Ni, Ruiyu
> > > Sent: Saturday, June 24, 2017 10:08 AM
> > > To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> > > Cc: Gao, Liming <liming.gao@intel.com>
> > > Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > > GetNextVariableName to follow UEFI 2.7
> > >
> > > Star,
> > > I don't recommend to add the additional check for performance
> > > consideration.
> > > Because we have no idea what the input VariableName buffer is like.
> > > If the VariableName is like ['\0', '?', '?', '?'] with MaxLen
> > > equals to 4, "VariableName[MaxLen-1] != 0" check is redundant.
> > > The NT32 case you met cannot represent the all possible cases.
> > > You could use the possibility theory to decide what the most
> > > efficient way
> > is.
> > >
> > > Additionally I think code readability is more important than efficiency.
> > > In this case, we need the data about the performance improvement
> > > to decide whether this check is necessary.
> > >
> > >
> > > Regards,
> > > Ray
> > >
> > > >-----Original Message-----
> > > >From: Zeng, Star
> > > >Sent: Friday, June 23, 2017 5:33 PM
> > > >To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> > > >Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star
> > > ><star.zeng@intel.com>
> > > >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > > >GetNextVariableName to follow UEFI 2.7
> > > >
> > > >Ray,
> > > >
> > > >It is to pass the check quickly and avoid scanning all the chars
> > > >in VariableName by StrnLenS for normal boot without invalid cases.
> > > >I did experiments in the code of GetNextVariableName with below
> > > >debug code for normal boot on NT32 and my real platforms, all the
> > > >cases will go
> > > into the branch "xxx 2".
> > > > if (((VariableName[MaxLen - 1] != 0))) {
> > > > DEBUG ((DEBUG_INFO, "xxx 1\n")); } else {
> > > > DEBUG ((DEBUG_INFO, "xxx 2\n")); }
> > > >
> > > >
> > > >Thanks,
> > > >Star
> > > >-----Original Message-----
> > > >From: Ni, Ruiyu
> > > >Sent: Friday, June 23, 2017 4:20 PM
> > > >To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> > > >Cc: Gao, Liming <liming.gao@intel.com>
> > > >Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > > >GetNextVariableName to follow UEFI 2.7
> > > >
> > > >Star,
> > > >What's the benefit of this check "VariableName[MaxLen - 1] != 0"?
> > > >I think this check "StrnLenS (VariableName, MaxLen) == MaxLen"
> > > >should be
> > > enough.
> > > >
> > > >Thanks/Ray
> > > >
> > > >> -----Original Message-----
> > > >> From: Zeng, Star
> > > >> Sent: Friday, June 23, 2017 4:08 PM
> > > >> To: edk2-devel@lists.01.org
> > > >> Cc: Zeng, Star <star.zeng@intel.com>; Gao, Liming
> > > >> <liming.gao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> > > >> Subject: [PATCH V2 3/3] DuetPkg FsVariable: Update
> > > >> GetNextVariableName to follow UEFI 2.7
> > > >>
> > > >> "The size must be large enough to fit input string supplied in
> > > >> VariableName buffer" is added in the description for
> VariableNameSize.
> > > >> And two cases of EFI_INVALID_PARAMETER are added.
> > > >> 1. The input values of VariableName and VendorGuid are not a
> > > >> name
> > and
> > > >> GUID of an existing variable.
> > > >> 2. Null-terminator is not found in the first VariableNameSize bytes of
> > > >> the input VariableName buffer.
> > > >>
> > > >> This patch is to update code to follow them.
> > > >>
> > > >> Cc: Liming Gao <liming.gao@intel.com>
> > > >> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > > >> Contributed-under: TianoCore Contribution Agreement 1.0
> > > >> Signed-off-by: Star Zeng <star.zeng@intel.com>
> > > >> ---
> > > >> DuetPkg/FSVariable/FSVariable.c | 21 ++++++++++++++++++++-
> > > >> 1 file changed, 20 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/DuetPkg/FSVariable/FSVariable.c
> > > >> b/DuetPkg/FSVariable/FSVariable.c index
> > > >> 34b79305c871..6069cfa8fb98
> > > >> 100644
> > > >> --- a/DuetPkg/FSVariable/FSVariable.c
> > > >> +++ b/DuetPkg/FSVariable/FSVariable.c
> > > >> @@ -6,7 +6,7 @@ disk. They can be changed by user. BIOS is not
> > > >> able to protoect those.
> > > >> Duet trusts all meta data from disk. If variable code,
> > > >> variable metadata and variable data is modified in inproper
> > > >> way, the behavior is undefined.
> > > >>
> > > >> -Copyright (c) 2006 - 2016, Intel Corporation. All rights
> > > >> reserved.<BR>
> > > >> +Copyright (c) 2006 - 2017, Intel Corporation. All rights
> > > >> +reserved.<BR>
> > > >> This program and the accompanying materials are licensed and
> > > >> made available under the terms and conditions of the BSD
> > > >> License which accompanies this distribution. The full text of
> > > >> the license may be found at @@ -1400,14 +1400,33 @@ Returns:
> > > >> VARIABLE_POINTER_TRACK Variable;
> > > >> UINTN VarNameSize;
> > > >> EFI_STATUS Status;
> > > >> + UINTN MaxLen;
> > > >>
> > > >> if (VariableNameSize == NULL || VariableName == NULL ||
> > > >> VendorGuid ==
> > > >> NULL) {
> > > >> return EFI_INVALID_PARAMETER;
> > > >> }
> > > >>
> > > >> + //
> > > >> + // Calculate the possible maximum length of name string,
> > > >> + including the Null
> > > >> terminator.
> > > >> + //
> > > >> + MaxLen = *VariableNameSize / sizeof (CHAR16); if ((MaxLen
> > > >> + ==
> > > >> + 0)
> > > >> + ||
> > > >> + ((VariableName[MaxLen - 1] != 0) && (StrnLenS
> > > >> + (VariableName,
> > > >> + MaxLen)
> > > >> == MaxLen))) {
> > > >> + //
> > > >> + // Null-terminator is not found in the first
> > > >> + VariableNameSize bytes of the
> > > >> input VariableName buffer.
> > > >> + //
> > > >> + return EFI_INVALID_PARAMETER; }
> > > >> +
> > > >> Status = FindVariable (VariableName, VendorGuid, &Variable);
> > > >>
> > > >> if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
> > > >> + if (VariableName[0] != 0) {
> > > >> + //
> > > >> + // The input values of VariableName and VendorGuid are
> > > >> + not a name
> > > >> and GUID of an existing variable.
> > > >> + //
> > > >> + Status = EFI_INVALID_PARAMETER;
> > > >> + }
> > > >> return Status;
> > > >> }
> > > >>
> > > >> --
> > > >> 2.7.0.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-06-26 6:30 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-23 8:08 [PATCH V2 0/3] Update comments and code for GetNextVariableName to follow UEFI 2.7 Star Zeng
2017-06-23 8:08 ` [PATCH V2 1/3] MdePkg: Update comments " Star Zeng
2017-06-23 8:08 ` [PATCH V2 2/3] MdeModulePkg Variable: Update " Star Zeng
2017-06-23 8:08 ` [PATCH V2 3/3] DuetPkg FsVariable: " Star Zeng
2017-06-23 8:20 ` Ni, Ruiyu
2017-06-23 9:33 ` Zeng, Star
2017-06-24 2:07 ` Ni, Ruiyu
2017-06-26 3:04 ` Zeng, Star
2017-06-26 5:36 ` Ni, Ruiyu
2017-06-26 5:41 ` Zeng, Star
2017-06-26 5:46 ` Ni, Ruiyu
2017-06-26 5:52 ` Zeng, Star
2017-06-26 6:18 ` Ni, Ruiyu
2017-06-26 6:31 ` Zeng, Star
2017-06-23 8:10 ` [PATCH V2 0/3] Update comments and code for " Gao, Liming
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox