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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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é <philmd@redhat.com>
>> Sent: Thursday, November 21, 2019 8:16 AM
>> To: devel@edk2.groups.io; Kubacki, Michael A
>> <michael.a.kubacki@intel.com>
>> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu,
>> Hao A <hao.a.wu@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.gao@intel.com>
>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>> Cc: Hao A Wu <hao.a.wu@intel.com>
>>> Signed-off-by: Michael Kubacki <michael.a.kubacki@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 <philmd@redhat.com>
>