Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable

[Philippe Mathieu-Daudé]

On 11/21/19 8:22 PM, Kubacki, Michael A wrote:

Hi Philippe,

I will split the change into two patches.

Thanks.

In the current implementation of VariableSmmRuntimeDxe.c there is
not an actual problem. The Status variable is set to EFI_NOT_FOUND
and RtPtrTrack is only used if Status is not set to an error code which
requires the RtPtrTrack variable to be set. Given that this is a
local variable, the initial value is undefined so I'd prefer to solve this by
initializing the variable value. This will better accommodate future
changes to the function by having a reasonable default value.

Regarding your suggestion, the expected behavior is that
RtPtrTrack.CurrPtr is assigned in the for loop and the value is
used in the if block after the for block so jumping to the
Done label would not be appropriate. The variable would also
still be uninitialized.

Yes you are right.

Thanks,
Michael

-----Original Message-----
From: Philippe Mathieu-Daudé <phi...@redhat.com>
Sent: Thursday, November 21, 2019 8:16 AM
To: devel@edk2.groups.io; Kubacki, Michael A
<michael.a.kuba...@intel.com>
Cc: Gao, Liming <liming....@intel.com>; Kinney, Michael D
<michael.d.kin...@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; Wu,
Hao A <hao.a...@intel.com>
Subject: Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Initialize
local variable

Hi Michael,

On 11/21/19 3:32 AM, Kubacki, Michael A wrote:

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2364

Fixes a new build warning in VS2012 introduced in f8ff4cca7c.

This patch initializes the local variable "Variable" in
VariableServiceGetNextVariableInternal () and the local variable
"RtPtrTrack" in FindVariableInRuntimeCache ().
This enusres the pointers in the structures are initialized

Typo "this ensures"

in the case no variable stores exist in the list of variable stores.

Cc: Liming Gao <liming....@intel.com>
Cc: Michael D Kinney <michael.d.kin...@intel.com>
Cc: Jian J Wang <jian.j.w...@intel.com>
Cc: Hao A Wu <hao.a...@intel.com>
Signed-off-by: Michael Kubacki <michael.a.kuba...@intel.com>
---
   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c       | 2

++

MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.
c | 2 ++

   2 files changed, 4 insertions(+)

diff --git
a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c
index d458f1c608..f6d187543d 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c
@@ -551,6 +551,8 @@ VariableServiceGetNextVariableInternal (
       return EFI_INVALID_PARAMETER;
     }

+  ZeroMem (&Variable, sizeof (Variable));
+

I agree with this change.

     // Check if the variable exists in the given variable store list
     for (StoreType = (VARIABLE_STORE_TYPE) 0; StoreType <

VariableStoreTypeMax; StoreType++) {

       if (VariableStoreList[StoreType] == NULL) { diff --git

a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
e.c

b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
e.c

index d525998ae3..2cf0ed32ae 100644
---

a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
e.c

+++

b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
e

+++ .c
@@ -590,6 +590,8 @@ FindVariableInRuntimeCache (
       return EFI_INVALID_PARAMETER;
     }

+  ZeroMem (&RtPtrTrack, sizeof (RtPtrTrack));
+
     //
     // The UEFI specification restricts Runtime Services callers from invoking

the same or certain other Runtime Service

     // functions prior to completion and return from a previous
Runtime Service call. These restrictions prevent

Here this seems overkill, what about:

-- >8 --
---
a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
e.c
+++
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
e.c
@@ -626,6 +626,10 @@ FindVariableInRuntimeCache (
         }
       }

+    if (RtPtrTrack.CurrPtr) {
+      goto Done;
+    }
+
       if (!EFI_ERROR (Status)) {
         //
         // Get data size

---

Can you split this patch in 2?
If so you can add to the 1st part:
Reviewed-by: Philippe Mathieu-Daude <phi...@redhat.com>

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#51181): https://edk2.groups.io/g/devel/message/51181
Mute This Topic: https://groups.io/mt/60998486/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-