Re: [edk2-devel] [edk2-platforms][PATCH V3 11/17] Platform/ARM/NorFlashDxe: Fix memory leak in NorFlashCreateInstance()

2024-05-23 Thread Sami Mujawar 

Hi Sahil,

Thank you for this patch.

I have a minor suggession marked inline as [SAMI].

Otherwise this patch looks good to me.

Reviewed-by: Sami Mujawar 

Regards,

Sami Mujawar

On 23/05/2024 11:55 am, Sahil Kaushal wrote:

From: sahil

This patch adds error_handler1 and error_handler2 labels in
NorFlashCreateInstance() function to handle the cleanup.

error_handler1: Frees just the Instance structure as the
ShadowBuffer is not allocated yet.

error_handler2: Frees both Instance and Instance->ShadowBuffer.

Signed-off-by: sahil
---
  Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c  | 18 
+-
  Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c | 19 
++-
  2 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c 
b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c
index e01b05d91978..fd47bd9e4c63 100644
--- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c
+++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c
@@ -135,7 +135,8 @@ NorFlashCreateInstance (
  


Instance->ShadowBuffer = AllocateRuntimePool (BlockSize);

if (Instance->ShadowBuffer == NULL) {

-return EFI_OUT_OF_RESOURCES;

+Status = EFI_OUT_OF_RESOURCES;

+goto error_handler1;

}

  


if (SupportFvb) {

@@ -152,8 +153,7 @@ NorFlashCreateInstance (
  NULL

  );

  if (EFI_ERROR (Status)) {

-  FreePool (Instance);

-  return Status;

+  goto error_handler2;

  }

} else {

  Status = gBS->InstallMultipleProtocolInterfaces (

@@ -167,12 +167,20 @@ NorFlashCreateInstance (
  NULL

  );

  if (EFI_ERROR (Status)) {

-  FreePool (Instance);

-  return Status;

+  goto error_handler2;

  }

}

  


*NorFlashInstance = Instance;

[SNIP]

+  return EFI_SUCCESS;

+
+error_handler1:

+  FreePool (Instance);

+  return Status;

+

+error_handler2:

+  FreePool (Instance->ShadowBuffer);

+  FreePool (Instance);

return Status;


[/SNIP]

[SAMI] I think the above code can be simplified as below:

---

+ return Status;
+

+error_handler2:

+  FreePool (Instance->ShadowBuffer);

+error_handler2:

+  FreePool (Instance);

   return Status;
---

A similar change is reuired later in this patch below.

If you agree, I will fix this up before merging the patch.

[/SAMI]



  }

  


diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c 
b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
index 16fe3762e125..17dfe26627dd 100644
--- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
+++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
@@ -129,7 +129,8 @@ NorFlashCreateInstance (
  


Instance->ShadowBuffer = AllocateRuntimePool (BlockSize);

if (Instance->ShadowBuffer == NULL) {

-return EFI_OUT_OF_RESOURCES;

+Status = EFI_OUT_OF_RESOURCES;

+goto error_handler1;

}

  


if (SupportFvb) {

@@ -142,16 +143,24 @@ NorFlashCreateInstance (
>FvbProtocol

);

  if (EFI_ERROR (Status)) {

-  FreePool (Instance);

-  return Status;

+  goto error_handler2;

  }

} else {

  DEBUG ((DEBUG_ERROR, "standalone MM NOR Flash driver only support 
FVB.\n"));

-FreePool (Instance);

-return EFI_UNSUPPORTED;

+Status = EFI_UNSUPPORTED;

+goto error_handler2;

}

  


*NorFlashInstance = Instance;

+  return EFI_SUCCESS;

+

+error_handler1:

+  FreePool (Instance);

+  return Status;

+

+error_handler2:

+  FreePool (Instance->ShadowBuffer);

+  FreePool (Instance);

return Status;

  }

  




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119164): https://edk2.groups.io/g/devel/message/119164
Mute This Topic: https://groups.io/mt/106260149/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-platforms][PATCH V3 11/17] Platform/ARM/NorFlashDxe: Fix memory leak in NorFlashCreateInstance()

2024-05-23 Thread Sahil Kaushal 
From: sahil 

This patch adds error_handler1 and error_handler2 labels in
NorFlashCreateInstance() function to handle the cleanup.

error_handler1: Frees just the Instance structure as the
ShadowBuffer is not allocated yet.

error_handler2: Frees both Instance and Instance->ShadowBuffer.

Signed-off-by: sahil 
---
 Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c  | 18 +-
 Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c | 19 
++-
 2 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c 
b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c
index e01b05d91978..fd47bd9e4c63 100644
--- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c
+++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c
@@ -135,7 +135,8 @@ NorFlashCreateInstance (
 
   Instance->ShadowBuffer = AllocateRuntimePool (BlockSize);
   if (Instance->ShadowBuffer == NULL) {
-return EFI_OUT_OF_RESOURCES;
+Status = EFI_OUT_OF_RESOURCES;
+goto error_handler1;
   }
 
   if (SupportFvb) {
@@ -152,8 +153,7 @@ NorFlashCreateInstance (
 NULL
 );
 if (EFI_ERROR (Status)) {
-  FreePool (Instance);
-  return Status;
+  goto error_handler2;
 }
   } else {
 Status = gBS->InstallMultipleProtocolInterfaces (
@@ -167,12 +167,20 @@ NorFlashCreateInstance (
 NULL
 );
 if (EFI_ERROR (Status)) {
-  FreePool (Instance);
-  return Status;
+  goto error_handler2;
 }
   }
 
   *NorFlashInstance = Instance;
+  return EFI_SUCCESS;
+
+error_handler1:
+  FreePool (Instance);
+  return Status;
+
+error_handler2:
+  FreePool (Instance->ShadowBuffer);
+  FreePool (Instance);
   return Status;
 }
 
diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c 
b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
index 16fe3762e125..17dfe26627dd 100644
--- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
+++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
@@ -129,7 +129,8 @@ NorFlashCreateInstance (
 
   Instance->ShadowBuffer = AllocateRuntimePool (BlockSize);
   if (Instance->ShadowBuffer == NULL) {
-return EFI_OUT_OF_RESOURCES;
+Status = EFI_OUT_OF_RESOURCES;
+goto error_handler1;
   }
 
   if (SupportFvb) {
@@ -142,16 +143,24 @@ NorFlashCreateInstance (
   >FvbProtocol
   );
 if (EFI_ERROR (Status)) {
-  FreePool (Instance);
-  return Status;
+  goto error_handler2;
 }
   } else {
 DEBUG ((DEBUG_ERROR, "standalone MM NOR Flash driver only support 
FVB.\n"));
-FreePool (Instance);
-return EFI_UNSUPPORTED;
+Status = EFI_UNSUPPORTED;
+goto error_handler2;
   }
 
   *NorFlashInstance = Instance;
+  return EFI_SUCCESS;
+
+error_handler1:
+  FreePool (Instance);
+  return Status;
+
+error_handler2:
+  FreePool (Instance->ShadowBuffer);
+  FreePool (Instance);
   return Status;
 }
 
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119153): https://edk2.groups.io/g/devel/message/119153
Mute This Topic: https://groups.io/mt/106260149/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-