Discussion:
[edk2] [PATCH v2] IntelFrameworkPkg FrameworkUefiLib: Fix ASSERT in CatVSPrint
Hao Wu
2015-07-09 00:53:43 UTC
Permalink
This commit will resolve issue brought by r17740.

BufferToReturn = AllocateCopyPool(SizeRequired, String);

The above using of AllocateCopyPool() will cause ASSERT if 'String' is
NULL. Therefore, proper check for 'String' is needed.

The above using of AllocateCopyPool() will read contents out of the scope
of 'String'. Potential risk for 'String' allocated at the boundary of
memory region.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu <***@intel.com>
---
IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c b/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
index 9a9503e..fbf9403 100644
--- a/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
+++ b/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
@@ -754,12 +754,18 @@ CatVSPrint (
SizeRequired = sizeof(CHAR16) + (CharactersRequired * sizeof(CHAR16));
}

- BufferToReturn = AllocateCopyPool(SizeRequired, String);
+ BufferToReturn = AllocatePool(SizeRequired);

if (BufferToReturn == NULL) {
return NULL;
}

+ BufferToReturn[0] = L'\0';
+
+ if (String != NULL) {
+ StrCpyS(BufferToReturn, SizeRequired, String);
+ }
+
UnicodeVSPrint(BufferToReturn + StrLen(BufferToReturn), (CharactersRequired+1) * sizeof(CHAR16), FormatString, Marker);

ASSERT(StrSize(BufferToReturn)==SizeRequired);
--
1.9.5.msysgit.0
Heyi Guo
2015-07-09 02:12:28 UTC
Permalink
Hi Hao,

Shall we also change code in MdePkg for the same function to keep code
style consistent?

Thanks.
Post by Hao Wu
This commit will resolve issue brought by r17740.
BufferToReturn = AllocateCopyPool(SizeRequired, String);
The above using of AllocateCopyPool() will cause ASSERT if 'String' is
NULL. Therefore, proper check for 'String' is needed.
The above using of AllocateCopyPool() will read contents out of the scope
of 'String'. Potential risk for 'String' allocated at the boundary of
memory region.
Contributed-under: TianoCore Contribution Agreement 1.0
---
IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c b/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
index 9a9503e..fbf9403 100644
--- a/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
+++ b/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
@@ -754,12 +754,18 @@ CatVSPrint (
SizeRequired = sizeof(CHAR16) + (CharactersRequired * sizeof(CHAR16));
}
- BufferToReturn = AllocateCopyPool(SizeRequired, String);
+ BufferToReturn = AllocatePool(SizeRequired);
if (BufferToReturn == NULL) {
return NULL;
}
+ BufferToReturn[0] = L'\0';
+
+ if (String != NULL) {
+ StrCpyS(BufferToReturn, SizeRequired, String);
+ }
+
UnicodeVSPrint(BufferToReturn + StrLen(BufferToReturn), (CharactersRequired+1) * sizeof(CHAR16), FormatString, Marker);
ASSERT(StrSize(BufferToReturn)==SizeRequired);
Wu, Hao A
2015-07-09 02:19:10 UTC
Permalink
Hi,

There are two options:
1. If the reviewers agree to keep using AllocateZeroPool() here, I can
modify the patch to keep align with MdePkg.

2. If they prefer to use AllocatePool() here, then I think codes in MdePkg
shall be updated.

Best Regards,
Hao Wu
-----Original Message-----
Sent: Thursday, July 09, 2015 10:12 AM
Shumin; Wu, Hao A
Subject: Re: [edk2] [PATCH v2] IntelFrameworkPkg FrameworkUefiLib: Fix
ASSERT in CatVSPrint
Hi Hao,
Shall we also change code in MdePkg for the same function to keep code
style consistent?
Thanks.
Post by Hao Wu
This commit will resolve issue brought by r17740.
BufferToReturn = AllocateCopyPool(SizeRequired, String);
The above using of AllocateCopyPool() will cause ASSERT if 'String' is
NULL. Therefore, proper check for 'String' is needed.
The above using of AllocateCopyPool() will read contents out of the scope
of 'String'. Potential risk for 'String' allocated at the boundary of
memory region.
Contributed-under: TianoCore Contribution Agreement 1.0
---
IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c | 8
+++++++-
Post by Hao Wu
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
b/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
Post by Hao Wu
index 9a9503e..fbf9403 100644
--- a/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
+++ b/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
@@ -754,12 +754,18 @@ CatVSPrint (
SizeRequired = sizeof(CHAR16) + (CharactersRequired * sizeof(CHAR16));
}
- BufferToReturn = AllocateCopyPool(SizeRequired, String);
+ BufferToReturn = AllocatePool(SizeRequired);
if (BufferToReturn == NULL) {
return NULL;
}
+ BufferToReturn[0] = L'\0';
+
+ if (String != NULL) {
+ StrCpyS(BufferToReturn, SizeRequired, String);
+ }
+
UnicodeVSPrint(BufferToReturn + StrLen(BufferToReturn),
(CharactersRequired+1) * sizeof(CHAR16), FormatString, Marker);
Post by Hao Wu
ASSERT(StrSize(BufferToReturn)==SizeRequired);
Jordan Justen
2015-07-09 15:39:10 UTC
Permalink
Post by Hao Wu
This commit will resolve issue brought by r17740.
BufferToReturn = AllocateCopyPool(SizeRequired, String);
The above using of AllocateCopyPool() will cause ASSERT if 'String' is
NULL. Therefore, proper check for 'String' is needed.
The above using of AllocateCopyPool() will read contents out of the scope
of 'String'. Potential risk for 'String' allocated at the boundary of
memory region.
Contributed-under: TianoCore Contribution Agreement 1.0
---
IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c b/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
index 9a9503e..fbf9403 100644
--- a/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
+++ b/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
@@ -754,12 +754,18 @@ CatVSPrint (
SizeRequired = sizeof(CHAR16) + (CharactersRequired * sizeof(CHAR16));
}
- BufferToReturn = AllocateCopyPool(SizeRequired, String);
+ BufferToReturn = AllocatePool(SizeRequired);
if (BufferToReturn == NULL) {
return NULL;
}
+ BufferToReturn[0] = L'\0';
I think it is better to put this as an else case just below.
Post by Hao Wu
+ if (String != NULL) {
+ StrCpyS(BufferToReturn, SizeRequired, String);
+ }
+
UnicodeVSPrint(BufferToReturn + StrLen(BufferToReturn), (CharactersRequired+1) * sizeof(CHAR16), FormatString, Marker);
ASSERT(StrSize(BufferToReturn)==SizeRequired);
--
1.9.5.msysgit.0
Wu, Hao A
2015-07-10 00:36:21 UTC
Permalink
-----Original Message-----
From: Justen, Jordan L
Sent: Thursday, July 09, 2015 11:39 PM
Cc: Wu, Hao A
Subject: Re: [PATCH v2] IntelFrameworkPkg FrameworkUefiLib: Fix ASSERT in
CatVSPrint
Post by Hao Wu
This commit will resolve issue brought by r17740.
BufferToReturn = AllocateCopyPool(SizeRequired, String);
The above using of AllocateCopyPool() will cause ASSERT if 'String' is
NULL. Therefore, proper check for 'String' is needed.
The above using of AllocateCopyPool() will read contents out of the scope
of 'String'. Potential risk for 'String' allocated at the boundary of
memory region.
Contributed-under: TianoCore Contribution Agreement 1.0
---
IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c | 8
+++++++-
Post by Hao Wu
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
b/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
Post by Hao Wu
index 9a9503e..fbf9403 100644
--- a/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
+++ b/IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLibPrint.c
@@ -754,12 +754,18 @@ CatVSPrint (
SizeRequired = sizeof(CHAR16) + (CharactersRequired * sizeof(CHAR16));
}
- BufferToReturn = AllocateCopyPool(SizeRequired, String);
+ BufferToReturn = AllocatePool(SizeRequired);
if (BufferToReturn == NULL) {
return NULL;
}
+ BufferToReturn[0] = L'\0';
I think it is better to put this as an else case just below.
Yes, I will modify it and commit the patch.
Post by Hao Wu
+ if (String != NULL) {
+ StrCpyS(BufferToReturn, SizeRequired, String);
+ }
+
UnicodeVSPrint(BufferToReturn + StrLen(BufferToReturn),
(CharactersRequired+1) * sizeof(CHAR16), FormatString, Marker);
Post by Hao Wu
ASSERT(StrSize(BufferToReturn)==SizeRequired);
--
1.9.5.msysgit.0
Loading...