[PATCH 0/2] Fix bugs in BmpSupportLib

[PATCH 0/2] Fix bugs in BmpSupportLib

[Ruiyu Ni]

Ruiyu Ni (2):
  MdeModulePkg/BmpSupportLib: Correct debug message
  MdeModulePkg/BmpSupportLib: Check PixelHeight/PixelWidth against 0

 MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

-- 
2.16.1.windows.1

[PATCH 1/2] MdeModulePkg/BmpSupportLib: Correct debug message

[Ruiyu Ni]

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c
index 467cd6a58d..2c23e2c61c 100644
--- a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c
+++ b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c
@@ -288,7 +288,7 @@ TranslateBmpToGopBlt (
     DEBUG ((
       DEBUG_ERROR,
       "TranslateBmpToGopBlt: invalid BltBuffer needed size... PixelWidth:0x%x PixelHeight:0x%x\n",
-      BltBufferSize
+      BmpHeader->PixelWidth, BmpHeader->PixelHeight
       ));
 
     return RETURN_UNSUPPORTED;
@@ -304,7 +304,7 @@ TranslateBmpToGopBlt (
   if (EFI_ERROR (Status)) {
     DEBUG ((
       DEBUG_ERROR,
-      "TranslateBmpToGopBlt: invalid BltBuffer needed size... BltBufferSize:0x%lx struct size:0x%x\n",
+      "TranslateBmpToGopBlt: invalid BltBuffer needed size... PixelWidth x PixelHeight:0x%x struct size:0x%x\n",
       Temp, sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)
       ));
 
-- 
2.16.1.windows.1

Re: [PATCH 1/2] MdeModulePkg/BmpSupportLib: Correct debug message

[Zeng, Star]

Please add reference to bugzilla link into the commit log.

https://bugzilla.tianocore.org/show_bug.cgi?id=944

Reviewed-by: Star Zeng <star.zeng@intel.com>


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Monday, June 25, 2018 3:37 PM
To: edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [edk2] [PATCH 1/2] MdeModulePkg/BmpSupportLib: Correct debug message

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c
index 467cd6a58d..2c23e2c61c 100644
--- a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c
+++ b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c
@@ -288,7 +288,7 @@ TranslateBmpToGopBlt (
     DEBUG ((
       DEBUG_ERROR,
       "TranslateBmpToGopBlt: invalid BltBuffer needed size... PixelWidth:0x%x PixelHeight:0x%x\n",
-      BltBufferSize
+      BmpHeader->PixelWidth, BmpHeader->PixelHeight
       ));
 
     return RETURN_UNSUPPORTED;
@@ -304,7 +304,7 @@ TranslateBmpToGopBlt (
   if (EFI_ERROR (Status)) {
     DEBUG ((
       DEBUG_ERROR,
-      "TranslateBmpToGopBlt: invalid BltBuffer needed size... BltBufferSize:0x%lx struct size:0x%x\n",
+      "TranslateBmpToGopBlt: invalid BltBuffer needed size... PixelWidth x PixelHeight:0x%x struct size:0x%x\n",
       Temp, sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)
       ));
 
-- 
2.16.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[PATCH 2/2] MdeModulePkg/BmpSupportLib: Check PixelHeight/PixelWidth against 0

[Ruiyu Ni]

The patch adds check logic to make sure that for a input BMP file,
the width or height is not 0; for a input GOP blt buffer, the width
or height is not 0. Otherwise, UNSUPPORTED status is returned.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
---
 MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c
index 2c23e2c61c..6196262d14 100644
--- a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c
+++ b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c
@@ -148,6 +148,11 @@ TranslateBmpToGopBlt (
     return RETURN_UNSUPPORTED;
   }
 
+  if ((BmpHeader->PixelHeight == 0) || (BmpHeader->PixelWidth == 0)) {
+    DEBUG ((DEBUG_ERROR, "TranslateBmpToGopBlt: BmpHeader->PixelHeight or BmpHeader->PixelWidth is 0.\n"));
+    return RETURN_UNSUPPORTED;
+  }
+
   //
   // Only support BITMAPINFOHEADER format.
   // BITMAPFILEHEADER + BITMAPINFOHEADER = BMP_IMAGE_HEADER
@@ -484,6 +489,10 @@ TranslateGopBltToBmp (
     return RETURN_INVALID_PARAMETER;
   }
 
+  if ((PixelHeight == 0) || (PixelWidth == 0)) {
+    return RETURN_UNSUPPORTED;
+  }
+
   //
   // Allocate memory for BMP file.
   //
-- 
2.16.1.windows.1

Re: [PATCH 2/2] MdeModulePkg/BmpSupportLib: Check PixelHeight/PixelWidth against 0

[Zeng, Star]

Please add reference to bugzilla link into the commit log.

https://bugzilla.tianocore.org/show_bug.cgi?id=944

Another, is there any reason to only add debug message for " if ((BmpHeader->PixelHeight == 0) || (BmpHeader->PixelWidth == 0)) { ", but not " if ((PixelHeight == 0) || (PixelWidth == 0)) { " ???


Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu 
Sent: Monday, June 25, 2018 3:37 PM
To: edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: [PATCH 2/2] MdeModulePkg/BmpSupportLib: Check PixelHeight/PixelWidth against 0

The patch adds check logic to make sure that for a input BMP file, the width or height is not 0; for a input GOP blt buffer, the width or height is not 0. Otherwise, UNSUPPORTED status is returned.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
---
 MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c
index 2c23e2c61c..6196262d14 100644
--- a/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c
+++ b/MdeModulePkg/Library/BaseBmpSupportLib/BmpSupportLib.c
@@ -148,6 +148,11 @@ TranslateBmpToGopBlt (
     return RETURN_UNSUPPORTED;
   }
 
+  if ((BmpHeader->PixelHeight == 0) || (BmpHeader->PixelWidth == 0)) {
+    DEBUG ((DEBUG_ERROR, "TranslateBmpToGopBlt: BmpHeader->PixelHeight or BmpHeader->PixelWidth is 0.\n"));
+    return RETURN_UNSUPPORTED;
+  }
+
   //
   // Only support BITMAPINFOHEADER format.
   // BITMAPFILEHEADER + BITMAPINFOHEADER = BMP_IMAGE_HEADER @@ -484,6 +489,10 @@ TranslateGopBltToBmp (
     return RETURN_INVALID_PARAMETER;
   }
 
+  if ((PixelHeight == 0) || (PixelWidth == 0)) {
+    return RETURN_UNSUPPORTED;
+  }
+
   //
   // Allocate memory for BMP file.
   //
--
2.16.1.windows.1

Re: [PATCH 2/2] MdeModulePkg/BmpSupportLib: Check PixelHeight/PixelWidth against 0

[Ni, Ruiyu]

On 7/2/2018 6:05 PM, Zeng, Star wrote:
> Please add reference to bugzilla link into the commit log.
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=944
OK
> 
> Another, is there any reason to only add debug message for " if ((BmpHeader->PixelHeight == 0) || (BmpHeader->PixelWidth == 0)) { ", but not " if ((PixelHeight == 0) || (PixelWidth == 0)) { " ???
PixelHeight and PixelWidth are passed in from caller directly.
But when caller passes in BmpImage buffer, it doesn't know the format
of the BMP header, so a debug message is shown to tell caller what happens.

-- 
Thanks,
Ray

Re: [PATCH 2/2] MdeModulePkg/BmpSupportLib: Check PixelHeight/PixelWidth against 0

[Zeng, Star]

So, you mean caller knows ((PixelHeight == 0) || (PixelWidth == 0)), and caller knows TranslateGopBltToBmp will return RETURN_UNSUPPORTED for that?


Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu 
Sent: Tuesday, July 3, 2018 10:03 AM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [PATCH 2/2] MdeModulePkg/BmpSupportLib: Check PixelHeight/PixelWidth against 0

On 7/2/2018 6:05 PM, Zeng, Star wrote:
> Please add reference to bugzilla link into the commit log.
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=944
OK
> 
> Another, is there any reason to only add debug message for " if ((BmpHeader->PixelHeight == 0) || (BmpHeader->PixelWidth == 0)) { ", but not " if ((PixelHeight == 0) || (PixelWidth == 0)) { " ???
PixelHeight and PixelWidth are passed in from caller directly.
But when caller passes in BmpImage buffer, it doesn't know the format of the BMP header, so a debug message is shown to tell caller what happens.

--
Thanks,
Ray

Re: [PATCH 2/2] MdeModulePkg/BmpSupportLib: Check PixelHeight/PixelWidth against 0

[Ni, Ruiyu]

On 7/3/2018 10:10 AM, Zeng, Star wrote:
> So, you mean caller knows ((PixelHeight == 0) || (PixelWidth == 0)), and caller knows TranslateGopBltToBmp will return RETURN_UNSUPPORTED for that?
Caller knows an error status may return when any of
PixelHeight/PixelWidth is 0.
But that's not the reason that I only add debug message for BmpHeader
case.

For first API:
TranslateBmpToGopBlt (
   IN     VOID                           *BmpImage,
   IN     UINTN                          BmpImageSize,
   IN OUT EFI_GRAPHICS_OUTPUT_BLT_PIXEL  **GopBlt,
   IN OUT UINTN                          *GopBltSize,
   OUT    UINTN                          *PixelHeight,
   OUT    UINTN                          *PixelWidth

Caller only needs to read a bmp file from a file system, then passes
the buffer to the above API. Caller doesn't know the format of BMP.
API checks for that. And using debug message to help caller understand
the detailed error reason.

For second API:
TranslateGopBltToBmp (
   IN     EFI_GRAPHICS_OUTPUT_BLT_PIXEL  *GopBlt,
   IN     UINT32                         PixelHeight,
   IN     UINT32                         PixelWidth,
   IN OUT VOID                           **BmpImage,
   IN OUT UINT32                         *BmpImageSize

PixelHeight and PixelWidth are parameters.


> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Tuesday, July 3, 2018 10:03 AM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [PATCH 2/2] MdeModulePkg/BmpSupportLib: Check PixelHeight/PixelWidth against 0
> 
> On 7/2/2018 6:05 PM, Zeng, Star wrote:
>> Please add reference to bugzilla link into the commit log.
>>
>> https://bugzilla.tianocore.org/show_bug.cgi?id=944
> OK
>>
>> Another, is there any reason to only add debug message for " if ((BmpHeader->PixelHeight == 0) || (BmpHeader->PixelWidth == 0)) { ", but not " if ((PixelHeight == 0) || (PixelWidth == 0)) { " ???
> PixelHeight and PixelWidth are passed in from caller directly.
> But when caller passes in BmpImage buffer, it doesn't know the format of the BMP header, so a debug message is shown to tell caller what happens.
> 
> --
> Thanks,
> Ray
> 


-- 
Thanks,
Ray

Re: [PATCH 2/2] MdeModulePkg/BmpSupportLib: Check PixelHeight/PixelWidth against 0

[Zeng, Star]

Ok. Reviewed-by: Star Zeng <star.zeng@intel.com>.


Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu 
Sent: Tuesday, July 3, 2018 10:17 AM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [PATCH 2/2] MdeModulePkg/BmpSupportLib: Check PixelHeight/PixelWidth against 0

On 7/3/2018 10:10 AM, Zeng, Star wrote:
> So, you mean caller knows ((PixelHeight == 0) || (PixelWidth == 0)), and caller knows TranslateGopBltToBmp will return RETURN_UNSUPPORTED for that?
Caller knows an error status may return when any of PixelHeight/PixelWidth is 0.
But that's not the reason that I only add debug message for BmpHeader case.

For first API:
TranslateBmpToGopBlt (
   IN     VOID                           *BmpImage,
   IN     UINTN                          BmpImageSize,
   IN OUT EFI_GRAPHICS_OUTPUT_BLT_PIXEL  **GopBlt,
   IN OUT UINTN                          *GopBltSize,
   OUT    UINTN                          *PixelHeight,
   OUT    UINTN                          *PixelWidth

Caller only needs to read a bmp file from a file system, then passes the buffer to the above API. Caller doesn't know the format of BMP.
API checks for that. And using debug message to help caller understand the detailed error reason.

For second API:
TranslateGopBltToBmp (
   IN     EFI_GRAPHICS_OUTPUT_BLT_PIXEL  *GopBlt,
   IN     UINT32                         PixelHeight,
   IN     UINT32                         PixelWidth,
   IN OUT VOID                           **BmpImage,
   IN OUT UINT32                         *BmpImageSize

PixelHeight and PixelWidth are parameters.


> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Tuesday, July 3, 2018 10:03 AM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [PATCH 2/2] MdeModulePkg/BmpSupportLib: Check 
> PixelHeight/PixelWidth against 0
> 
> On 7/2/2018 6:05 PM, Zeng, Star wrote:
>> Please add reference to bugzilla link into the commit log.
>>
>> https://bugzilla.tianocore.org/show_bug.cgi?id=944
> OK
>>
>> Another, is there any reason to only add debug message for " if ((BmpHeader->PixelHeight == 0) || (BmpHeader->PixelWidth == 0)) { ", but not " if ((PixelHeight == 0) || (PixelWidth == 0)) { " ???
> PixelHeight and PixelWidth are passed in from caller directly.
> But when caller passes in BmpImage buffer, it doesn't know the format of the BMP header, so a debug message is shown to tell caller what happens.
> 
> --
> Thanks,
> Ray
> 


--
Thanks,
Ray