Yes. The allocated HelpString is required to be free after HiiSetString.

Thanks
Liming

-----邮件原件-----
发件人: discuss@edk2.groups.io <discuss@edk2.groups.io> 代表
Konstantin Aladyshev
发送时间: 2022年10月2日 5:17
收件人: discuss <discuss@edk2.groups.io>; zhichao.gao@...; Ni, Ray
<ray.ni@...>
主题: [edk2-discuss] Memory leak in the BootManager code

Hello!

I have a question about dynamic content generation in the form
EFI_HII_CONFIG_ACCESS_PROTOCOL.Callback() code.
Usually such content is generated with the help of labels on the
EFI_BROWSER_ACTION_FORM_OPEN action. Example is the BootManager
form.

The thing that I want to discuss is that strings for the generated
elements are produced via the `HiiSetString(HiiHandle, 0, <Text>,
NULL)` calls.
But if we use 0 as the second argument, this would produce new string
packages every time we enter the BootManager form. Isn't it a memory
leak?

Example:
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/Bo
otManagerUiLib/BootManager.c

BootManagerCallback() calls UpdateBootManager() on each form open
which creates new strings via:
```
Token = HiiSetString (HiiHandle, 0, BootOption[Index].Description, NULL);
HelpToken = HiiSetString (HiiHandle, 0, HelpString, NULL);
```

Best regards,
Konstantin Aladyshev

-----Original Message-----
From: Konstantin Aladyshev <aladyshev22@...>
Sent: Saturday, October 8, 2022 4:15 PM
To: Gao, Liming <gaoliming@...>
Cc: discuss@edk2.groups.io; Gao, Zhichao <zhichao.gao@...>; Ni, Ray
<ray.ni@...>
Subject: Re: [edk2-discuss] Memory leak in the BootManager code

I'm not just asking about the fact that the dynamically allocated
'HelpString' must be freed, i.e. this:
```
HelpString = AllocateZeroPool (...);
HiiSetString(HiiHandle, 0, HelpString, NULL)
FreePool(HelpString);
```

I'm asking, do we need to do something to free the newly allocated
EFI_HII_STRING_BLOCK's?

Because every call to the "HiiSetString(HiiHandle, 0, <Text>, NULL)"
eventually calls the "EFI_HII_STRING_PROTOCOL.NewString()" function.
This function modifies the current 'EFI_HII_STRING_PACKAGE' adding
another 'EFI_HII_STRING_BLOCK' to it:
```
typedef struct {
UINT8 BlockType;
UINT8 BlockBody[];
} EFI_HII_STRING_BLOCK;
```
Even if we would free the "HelpString" itself, these
'EFI_HII_STRING_BLOCK's would be piling up in the HII Database. This
is the memory leak that I'm talking about.

And I don't see any way in the UEFI specification to remove some
'EFI_HII_STRING_BLOCK' from the 'EFI_HII_STRING_PACKAGE' package. So
the only solution that I see is to keep a list of all the allocated
StringId's by the Driver, and try to reuse them when possible. But
this seems cumbersome, and I don't see such an implementation anywhere
in EDKII.

Best regards,
Konstantin Aladyshev


On Sat, Oct 8, 2022 at 5:32 AM gaoliming <gaoliming@...> wrote:

Yes. The allocated HelpString is required to be free after HiiSetString.

Thanks
Liming

-----邮件原件-----
发件人: discuss@edk2.groups.io <discuss@edk2.groups.io> 代表
Konstantin Aladyshev
发送时间: 2022年10月2日 5:17
收件人: discuss <discuss@edk2.groups.io>; zhichao.gao@...; Ni,

Ray

<ray.ni@...>
主题: [edk2-discuss] Memory leak in the BootManager code

Hello!

I have a question about dynamic content generation in the form
EFI_HII_CONFIG_ACCESS_PROTOCOL.Callback() code.
Usually such content is generated with the help of labels on the
EFI_BROWSER_ACTION_FORM_OPEN action. Example is the

BootManager

form.

The thing that I want to discuss is that strings for the generated
elements are produced via the `HiiSetString(HiiHandle, 0, <Text>,
NULL)` calls.
But if we use 0 as the second argument, this would produce new string
packages every time we enter the BootManager form. Isn't it a memory
leak?

Example:

https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/B
o

otManagerUiLib/BootManager.c

BootManagerCallback() calls UpdateBootManager() on each form open
which creates new strings via:
```
Token = HiiSetString (HiiHandle, 0, BootOption[Index].Description, NULL);
HelpToken = HiiSetString (HiiHandle, 0, HelpString, NULL);
```

Best regards,
Konstantin Aladyshev