Re: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable: Move FindVariable after AutoUpdateLangVariable

["Guomin Jiang" <guomin.jiang@intel.com>]

Hi Huang,

>From issue statement, I guess that
1. AutoUpdateLangVariable() invoked, and it will invoke FindVariable() first, at the same time, reclaim occur and Variable.CurrPtr is invalid, it return with success
2. UpdateVariable() is invoked when The old Lang's State is valid and the new Lang's State is also valid.
3. In the situation, FindVariable() checked Lang's State and only enable one Lang's State. But it didn't in fact.
4. BmForEachVariable() deadloop in the situation.

Am I right?

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ming
> Huang via groups.io
> Sent: Monday, June 29, 2020 2:06 PM
> To: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Gao, Liming <liming.gao@intel.com>
> Cc: lidongzhan@huawei.com; huangming23@huawei.com;
> songdongkuang@huawei.com; wanghuiqiang@huawei.com;
> qiuliangen@huawei.com; shenlimei@huawei.com; xiewenyi2@huawei.com
> Subject: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable: Move
> FindVariable after AutoUpdateLangVariable
> 
> When occur reclaim in AutoUpdateLangVariable(), the CurrPtr of Variable is
> invalid. The State will be update with wrong position after UpdateVariable in
> this situation and two valid PlatformLang or Lang variables will exist.
> BmForEachVariable() will enter endless loop while exist two valid
> PlatformLang variables. So FindVariable() should be invoked atfer
> AutoUpdateLangVariable().
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=2667
> 
> Signed-off-by: Ming Huang <huangming23@huawei.com>
> ---
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 26
> ++++++++++----------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index 1e71fc6..0cec981 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -2741,6 +2741,19 @@ VariableServiceSetVariable (
>      mVariableModuleGlobal->NonVolatileLastVariableOffset = (UINTN)
> NextVariable - (UINTN) Point;
>    }
> 
> +  if (!FeaturePcdGet (PcdUefiVariableDefaultLangDeprecate)) {
> +    //
> +    // Hook the operation of setting PlatformLangCodes/PlatformLang and
> LangCodes/Lang.
> +    //
> +    Status = AutoUpdateLangVariable (VariableName, Data, DataSize);
> +    if (EFI_ERROR (Status)) {
> +      //
> +      // The auto update operation failed, directly return to avoid
> inconsistency between PlatformLang and Lang.
> +      //
> +      goto Done;
> +    }
> +  }
> +
>    //
>    // Check whether the input variable is already existed.
>    //
> @@ -2763,19 +2776,6 @@ VariableServiceSetVariable (
>      }
>    }
> 
> -  if (!FeaturePcdGet (PcdUefiVariableDefaultLangDeprecate)) {
> -    //
> -    // Hook the operation of setting PlatformLangCodes/PlatformLang and
> LangCodes/Lang.
> -    //
> -    Status = AutoUpdateLangVariable (VariableName, Data, DataSize);
> -    if (EFI_ERROR (Status)) {
> -      //
> -      // The auto update operation failed, directly return to avoid inconsistency
> between PlatformLang and Lang.
> -      //
> -      goto Done;
> -    }
> -  }
> -
>    if (mVariableModuleGlobal->VariableGlobal.AuthSupport) {
>      Status = AuthVariableLibProcessVariable (VariableName, VendorGuid,
> Data, DataSize, Attributes);
>    } else {
> --
> 2.8.1
> 
> 
>