Re: [PATCH] RedfishPkg/RedfishPlatformConfigDxe: Fix string assert issue

["Chang, Abner" <abner.chang@amd.com>]

[AMD Official Use Only - General]

Reviewed-by: Abner Chang <abner.chang@amd.com>

> -----Original Message-----
> From: Nickle Wang <nicklew@nvidia.com>
> Sent: Monday, May 15, 2023 12:25 PM
> To: devel@edk2.groups.io
> Cc: Chang, Abner <Abner.Chang@amd.com>; Igor Kulchytskyy
> <igork@ami.com>
> Subject: [PATCH] RedfishPkg/RedfishPlatformConfigDxe: Fix string assert
> issue
> 
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> 
> 
> When calling SetValue() with string type input, there is
> assertion of providing zero string ID to HII string function.
> Fix this issue by creating string ID for input string buffer.
> Fix Unicode and Ascii code convert issue together.
> Add text op-code support
> 
> Signed-off-by: Nickle Wang <nicklew@nvidia.com>
> Cc: Abner Chang <abner.chang@amd.com>
> Cc: Igor Kulchytskyy <igork@ami.com>
> ---
>  .../RedfishPlatformConfigDxe.h                |  14 +++
>  .../RedfishPlatformConfigImpl.h               |  16 +++
>  .../RedfishPlatformConfigDxe.c                | 116 ++++++++++++++++--
>  .../RedfishPlatformConfigImpl.c               |  50 +++++---
>  4 files changed, 169 insertions(+), 27 deletions(-)
> 
> diff --git
> a/RedfishPkg/RedfishPlatformConfigDxe/RedfishPlatformConfigDxe.h
> b/RedfishPkg/RedfishPlatformConfigDxe/RedfishPlatformConfigDxe.h
> index 67697ecda787..c86bc6e9ce2d 100644
> --- a/RedfishPkg/RedfishPlatformConfigDxe/RedfishPlatformConfigDxe.h
> +++ b/RedfishPkg/RedfishPlatformConfigDxe/RedfishPlatformConfigDxe.h
> @@ -78,4 +78,18 @@ typedef struct {
>  #define REDFISH_PLATFORM_CONFIG_DEBUG    DEBUG_VERBOSE
>  #define REDFISH_MENU_PATH_SIZE           8
> 
> +/**
> +  Convert input unicode string to ascii string. It's caller's
> +  responsibility to free returned buffer using FreePool().
> +
> +  @param[in]  UnicodeString     Unicode string to be converted.
> +
> +  @retval CHAR8 *               Ascii string on return.
> +
> +**/
> +CHAR8 *
> +StrToAsciiStr (
> +  IN  EFI_STRING  UnicodeString
> +  );
> +
>  #endif
> diff --git
> a/RedfishPkg/RedfishPlatformConfigDxe/RedfishPlatformConfigImpl.h
> b/RedfishPkg/RedfishPlatformConfigDxe/RedfishPlatformConfigImpl.h
> index 9ef032748663..9f4312decf50 100644
> --- a/RedfishPkg/RedfishPlatformConfigDxe/RedfishPlatformConfigImpl.h
> +++ b/RedfishPkg/RedfishPlatformConfigDxe/RedfishPlatformConfigImpl.h
> @@ -250,6 +250,22 @@ ProcessPendingList (
>    IN  LIST_ENTRY  *PendingList
>    );
> 
> +/**
> +  Delete a string from HII Package List by given HiiHandle.
> +
> +  @param[in]  StringId           Id of the string in HII database.
> +  @param[in]  HiiHandle          The HII package list handle.
> +
> +  @retval EFI_SUCCESS            The string was deleted successfully.
> +  @retval EFI_INVALID_PARAMETER  StringId is zero.
> +
> +**/
> +EFI_STATUS
> +HiiDeleteString (
> +  IN  EFI_STRING_ID   StringId,
> +  IN  EFI_HII_HANDLE  HiiHandle
> +  );
> +
>  /**
>    Retrieves a unicode string from a string package in a given language. The
>    returned string is allocated using AllocatePool().  The caller is responsible
> diff --git
> a/RedfishPkg/RedfishPlatformConfigDxe/RedfishPlatformConfigDxe.c
> b/RedfishPkg/RedfishPlatformConfigDxe/RedfishPlatformConfigDxe.c
> index d3902f4127c1..1172d1094b06 100644
> --- a/RedfishPkg/RedfishPlatformConfigDxe/RedfishPlatformConfigDxe.c
> +++ b/RedfishPkg/RedfishPlatformConfigDxe/RedfishPlatformConfigDxe.c
> @@ -1172,6 +1172,7 @@ HiiValueToRedfishValue (
>    UINTN          Index;
>    UINTN          Count;
>    EFI_STRING_ID  *StringIdArray;
> +  CHAR8          NullChar;
> 
>    if ((HiiHandle == NULL) || (HiiStatement == NULL) || (Value == NULL) ||
> (RedfishValue == NULL) || IS_EMPTY_STRING (FullSchema)) {
>      return EFI_INVALID_PARAMETER;
> @@ -1180,6 +1181,7 @@ HiiValueToRedfishValue (
>    StringIdArray = NULL;
>    Count         = 0;
>    Status        = EFI_SUCCESS;
> +  NullChar      = '\0';
> 
>    switch (HiiStatement->Operand) {
>      case EFI_IFR_ONE_OF_OP:
> @@ -1205,9 +1207,18 @@ HiiValueToRedfishValue (
>          break;
>        }
> 
> -      RedfishValue->Type         = RedfishValueTypeString;
> -      RedfishValue->Value.Buffer = AllocatePool (StrLen ((CHAR16 *)Value-
> >Buffer) + 1);
> -      UnicodeStrToAsciiStrS ((CHAR16 *)Value->Buffer, RedfishValue-
> >Value.Buffer, StrLen ((CHAR16 *)Value->Buffer) + 1);
> +      if (Value->Buffer == NULL) {
> +        RedfishValue->Value.Buffer = AllocateCopyPool (sizeof (NullChar),
> &NullChar);
> +      } else {
> +        RedfishValue->Value.Buffer = StrToAsciiStr ((EFI_STRING)Value-
> >Buffer);
> +      }
> +
> +      if (RedfishValue->Value.Buffer == NULL) {
> +        Status = EFI_OUT_OF_RESOURCES;
> +        break;
> +      }
> +
> +      RedfishValue->Type = RedfishValueTypeString;
>        break;
>      case EFI_IFR_CHECKBOX_OP:
>      case EFI_IFR_NUMERIC_OP:
> @@ -1256,6 +1267,30 @@ HiiValueToRedfishValue (
> 
>        FreePool (StringIdArray);
>        break;
> +    case EFI_IFR_TEXT_OP:
> +      //
> +      // Use text two as the value
> +      //
> +      if (HiiStatement->ExtraData.TextTwo == 0x00) {
> +        Status = EFI_NOT_FOUND;
> +        break;
> +      }
> +
> +      RedfishValue->Value.Buffer = HiiGetRedfishAsciiString (HiiHandle,
> FullSchema, HiiStatement->ExtraData.TextTwo);
> +      if (RedfishValue->Value.Buffer == NULL) {
> +        //
> +        // No x-uefi-redfish string defined. Try to get string in English.
> +        //
> +        RedfishValue->Value.Buffer = HiiGetEnglishAsciiString (HiiHandle,
> HiiStatement->ExtraData.TextTwo);
> +      }
> +
> +      if (RedfishValue->Value.Buffer == NULL) {
> +        Status = EFI_OUT_OF_RESOURCES;
> +        break;
> +      }
> +
> +      RedfishValue->Type = RedfishValueTypeString;
> +      break;
>      default:
>        DEBUG ((DEBUG_ERROR, "%a: catch unsupported type: 0x%x! Please
> contact with author if we need to support this type.\n", __func__,
> HiiStatement->Operand));
>        ASSERT (FALSE);
> @@ -1284,7 +1319,7 @@ StrToUnicodeStr (
>    EFI_STRING  Buffer;
>    EFI_STATUS  Status;
> 
> -  if ((AsciiString == NULL) || (AsciiString[0] == '\0')) {
> +  if (AsciiString == NULL) {
>      return NULL;
>    }
> 
> @@ -1303,6 +1338,43 @@ StrToUnicodeStr (
>    return Buffer;
>  }
> 
> +/**
> +  Convert input unicode string to ascii string. It's caller's
> +  responsibility to free returned buffer using FreePool().
> +
> +  @param[in]  UnicodeString     Unicode string to be converted.
> +
> +  @retval CHAR8 *               Ascii string on return.
> +
> +**/
> +CHAR8 *
> +StrToAsciiStr (
> +  IN  EFI_STRING  UnicodeString
> +  )
> +{
> +  UINTN       StringLen;
> +  CHAR8       *Buffer;
> +  EFI_STATUS  Status;
> +
> +  if (UnicodeString == NULL) {
> +    return NULL;
> +  }
> +
> +  StringLen = StrLen (UnicodeString) + 1;
> +  Buffer    = AllocatePool (StringLen * sizeof (CHAR8));
> +  if (Buffer == NULL) {
> +    return NULL;
> +  }
> +
> +  Status = UnicodeStrToAsciiStrS (UnicodeString, Buffer, StringLen);
> +  if (EFI_ERROR (Status)) {
> +    FreePool (Buffer);
> +    return NULL;
> +  }
> +
> +  return Buffer;
> +}
> +
>  /**
>    Return the full Redfish schema string from the given Schema and Version.
> 
> @@ -1641,6 +1713,17 @@ RedfishPlatformConfigSetStatementCommon (
>      }
>    }
> 
> +  if ((TargetStatement->HiiStatement->Operand == EFI_IFR_STRING_OP)
> && (StatementValue->Type == EFI_IFR_TYPE_STRING)) {
> +    //
> +    // Create string ID for new string.
> +    //
> +    StatementValue->Value.string = HiiSetString (TargetStatement-
> >ParentForm->ParentFormset->HiiHandle, 0x00,
> (EFI_STRING)StatementValue->Buffer, NULL);
> +    if (StatementValue->Value.string == 0) {
> +      DEBUG ((DEBUG_ERROR, "%a: can not create string id\n", __func__));
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> +  }
> +
>    Status = RedfishPlatformConfigSaveQuestionValue (
>               TargetStatement->ParentForm->ParentFormset->HiiFormSet,
>               TargetStatement->ParentForm->HiiForm,
> @@ -1649,10 +1732,13 @@ RedfishPlatformConfigSetStatementCommon (
>               );
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR, "%a: failed to save question value: %r\n",
> __func__, Status));
> -    return Status;
>    }
> 
> -  return EFI_SUCCESS;
> +  if (StatementValue->Value.string != 0) {
> +    HiiDeleteString (StatementValue->Value.string, TargetStatement-
> >ParentForm->ParentFormset->HiiHandle);
> +  }
> +
> +  return Status;
>  }
> 
>  /**
> @@ -1712,9 +1798,14 @@ RedfishPlatformConfigProtocolSetValue (
> 
>        break;
>      case RedfishValueTypeString:
> +      if (Value.Value.Buffer == NULL) {
> +        Status = EFI_INVALID_PARAMETER;
> +        goto RELEASE_RESOURCE;
> +      }
> +
>        NewValue.Type      = EFI_IFR_TYPE_STRING;
> -      NewValue.BufferLen = (UINT16)AsciiStrSize (Value.Value.Buffer);
> -      NewValue.Buffer    = AllocateCopyPool (NewValue.BufferLen,
> Value.Value.Buffer);
> +      NewValue.BufferLen = (UINT16)(AsciiStrSize (Value.Value.Buffer) *
> sizeof (CHAR16));
> +      NewValue.Buffer    = (UINT8 *)StrToUnicodeStr (Value.Value.Buffer);
>        if (NewValue.Buffer == NULL) {
>          Status = EFI_OUT_OF_RESOURCES;
>          goto RELEASE_RESOURCE;
> @@ -1742,6 +1833,10 @@ RELEASE_RESOURCE:
>      FreePool (FullSchema);
>    }
> 
> +  if ((Value.Type == RedfishValueTypeString) && (NewValue.Buffer !=
> NULL)) {
> +    FreePool (NewValue.Buffer);
> +  }
> +
>    return Status;
>  }
> 
> @@ -1784,6 +1879,7 @@ RedfishPlatformConfigProtocolGetConfigureLang (
>      return EFI_INVALID_PARAMETER;
>    }
> 
> +  ZeroMem (&StatementList, sizeof (StatementList));
>    *Count                       = 0;
>    *ConfigureLangList           = NULL;
>    FullSchema                   = NULL;
> @@ -1849,7 +1945,9 @@ RELEASE_RESOURCE:
>      FreePool (FullSchema);
>    }
> 
> -  ReleaseStatementList (&StatementList);
> +  if (StatementList.Count > 0) {
> +    ReleaseStatementList (&StatementList);
> +  }
> 
>    return Status;
>  }
> diff --git
> a/RedfishPkg/RedfishPlatformConfigDxe/RedfishPlatformConfigImpl.c
> b/RedfishPkg/RedfishPlatformConfigDxe/RedfishPlatformConfigImpl.c
> index 889448fe3870..e4a905aec44e 100644
> --- a/RedfishPkg/RedfishPlatformConfigDxe/RedfishPlatformConfigImpl.c
> +++ b/RedfishPkg/RedfishPlatformConfigDxe/RedfishPlatformConfigImpl.c
> @@ -143,6 +143,34 @@ DumpFormsetList (
>    return EFI_SUCCESS;
>  }
> 
> +/**
> +  Delete a string from HII Package List by given HiiHandle.
> +
> +  @param[in]  StringId           Id of the string in HII database.
> +  @param[in]  HiiHandle          The HII package list handle.
> +
> +  @retval EFI_SUCCESS            The string was deleted successfully.
> +  @retval EFI_INVALID_PARAMETER  StringId is zero.
> +
> +**/
> +EFI_STATUS
> +HiiDeleteString (
> +  IN  EFI_STRING_ID   StringId,
> +  IN  EFI_HII_HANDLE  HiiHandle
> +  )
> +{
> +  CHAR16  NullChar;
> +
> +  if (StringId == 0x00) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  NullChar = CHAR_NULL;
> +  HiiSetString (HiiHandle, StringId, &NullChar, NULL);
> +
> +  return EFI_SUCCESS;
> +}
> +
>  /**
>    Retrieves a unicode string from a string package in a given language. The
>    returned string is allocated using AllocatePool().  The caller is responsible
> @@ -259,7 +287,6 @@ HiiGetRedfishAsciiString (
>    )
>  {
>    EFI_STRING  HiiString;
> -  UINTN       StringSize;
>    CHAR8       *AsciiString;
> 
>    HiiString = HiiGetRedfishString (HiiHandle, Language, StringId);
> @@ -268,15 +295,9 @@ HiiGetRedfishAsciiString (
>      return NULL;
>    }
> 
> -  StringSize  = (StrLen (HiiString) + 1) * sizeof (CHAR8);
> -  AsciiString = AllocatePool (StringSize);
> -  if (AsciiString == NULL) {
> -    return NULL;
> -  }
> -
> -  UnicodeStrToAsciiStrS (HiiString, AsciiString, StringSize);
> -
> +  AsciiString = StrToAsciiStr (HiiString);
>    FreePool (HiiString);
> +
>    return AsciiString;
>  }
> 
> @@ -322,7 +343,6 @@ HiiGetEnglishAsciiString (
>    )
>  {
>    EFI_STRING  HiiString;
> -  UINTN       StringSize;
>    CHAR8       *AsciiString;
> 
>    HiiString = HiiGetRedfishString (HiiHandle, ENGLISH_LANGUAGE_CODE,
> StringId);
> @@ -331,15 +351,9 @@ HiiGetEnglishAsciiString (
>      return NULL;
>    }
> 
> -  StringSize  = (StrLen (HiiString) + 1) * sizeof (CHAR8);
> -  AsciiString = AllocatePool (StringSize);
> -  if (AsciiString == NULL) {
> -    return NULL;
> -  }
> -
> -  UnicodeStrToAsciiStrS (HiiString, AsciiString, StringSize);
> -
> +  AsciiString = StrToAsciiStr (HiiString);
>    FreePool (HiiString);
> +
>    return AsciiString;
>  }
> 
> --
> 2.17.1