[PATCH] Fix memory leak in custom FS label retrieval

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

buffer is AllocatePool()'d but not FreePool()'d on error
conditions, resulting in a memory leak. Fix it by calling
FreePool() on error exit paths.

Signed-off-by: Christian Storm <christi...@siemens.com>
---
utils.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/utils.c b/utils.c
index 6238c8b..42ff741 100644
--- a/utils.c
+++ b/utils.c
@@ -89,10 +89,12 @@ CHAR16 *get_volume_custom_label(EFI_FILE_HANDLE fh)
fh->Open, 5, fh, &tmp, L"EFILABEL", EFI_FILE_MODE_READ,
EFI_FILE_ARCHIVE | EFI_FILE_HIDDEN | EFI_FILE_SYSTEM);
if (status != EFI_SUCCESS) {
+ (VOID)FreePool(buffer);
return NULL;
}
status = uefi_call_wrapper(tmp->Read, 3, tmp, &buffsize, buffer);
if (status != EFI_SUCCESS) {
+ (VOID)FreePool(buffer);
return NULL;
}
buffer[buffsize/sizeof(CHAR16)] = L'\0';
--
2.32.0

[Jan Kiszka]

Why "(VOID)"? We are not doing this so far.

Jan

--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

[Christian Storm]

Hi Jan,

> > buffer is AllocatePool()'d but not FreePool()'d on error
> > conditions, resulting in a memory leak. Fix it by calling
> > FreePool() on error exit paths.
> >
> > Signed-off-by: Christian Storm <christi...@siemens.com>
> > ---
> > utils.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/utils.c b/utils.c
> > index 6238c8b..42ff741 100644
> > --- a/utils.c
> > +++ b/utils.c
> > @@ -89,10 +89,12 @@ CHAR16 *get_volume_custom_label(EFI_FILE_HANDLE fh)
> > fh->Open, 5, fh, &tmp, L"EFILABEL", EFI_FILE_MODE_READ,
> > EFI_FILE_ARCHIVE | EFI_FILE_HIDDEN | EFI_FILE_SYSTEM);
> > if (status != EFI_SUCCESS) {
> > + (VOID)FreePool(buffer);
> > return NULL;
> > }
> > status = uefi_call_wrapper(tmp->Read, 3, tmp, &buffsize, buffer);
> > if (status != EFI_SUCCESS) {
> > + (VOID)FreePool(buffer);
> > return NULL;
> > }
> > buffer[buffsize/sizeof(CHAR16)] = L'\0';
> >
>
> Why "(VOID)"?

Since I'm ignoring the return code and want to make this fact explicit.
There are two possible return codes from FreePool():
EFI_SUCCESS → The memory was returned to the system.
EFI_INVALID_PARAMETER → Buffer was invalid.
The first one is not interesting, and the second one does not happen,
so I've opted for ignoring it altogether.

> We are not doing this so far.

We are, in fact in several places, grep for "(void)" or look at your
commit d6875df in which you fixed up lowercase (void) → (VOID).

That said, I can drop the prefixing (VOID), I don't care too much but
I do think explicitness helps, particular in critical components such
as a bootloader.


Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Technology, T RDA IOT SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany

[Jan Kiszka]

FreePool() is so far handled like free(), thus without voiding any
return value.

I'll drop it here for now. If we want it back, it has to be done
consistently.