diff mbox

[edk2,v2,2/3] MdePkg/BaseMemoryLib*: add missing ASSERT()s

Message ID 1474272831-20840-3-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel Sept. 19, 2016, 8:13 a.m. UTC 
Add the ASSERT() statements to CopyGuid (), CompareGuid() and
IsZeroGuid() that are mentioned in the respective comments but
were missing from the actual code.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 MdePkg/Library/BaseMemoryLib/MemLibGuid.c       | 8 ++++++++
 MdePkg/Library/BaseMemoryLibMmx/MemLibGuid.c    | 8 ++++++++
 MdePkg/Library/BaseMemoryLibOptDxe/MemLibGuid.c | 8 ++++++++
 MdePkg/Library/BaseMemoryLibOptPei/MemLibGuid.c | 8 ++++++++
 MdePkg/Library/BaseMemoryLibRepStr/MemLibGuid.c | 8 ++++++++
 MdePkg/Library/BaseMemoryLibSse2/MemLibGuid.c   | 8 ++++++++
 MdePkg/Library/PeiMemoryLib/MemLibGuid.c        | 8 ++++++++
 MdePkg/Library/UefiMemoryLib/MemLibGuid.c       | 8 ++++++++
 8 files changed, 64 insertions(+)

-- 
2.7.4

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

Comments

Ard Biesheuvel Sept. 20, 2016, 8:38 a.m. UTC | #1 
On 20 September 2016 at 03:00, Wu, Hao A <hao.a.wu@intel.com> wrote:
> Hi Ard,

>

> The NULL checks for the input Guids in APIs CopyGuid(), CompareGuid() and

> IsZeroGuid() are implicitly done within calls to BaseLib APIs

> ReadUnaligned64() and WriteUnaligned64().

>

> So I think the functions behavior matches with their comments. What do you

> think?

>


I disagree. ReadUnaligned64 and WriteUnaligned64 could theoretically
be implemented by a version of BaseLib that does not contain such
ASSERT()s
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Sept. 20, 2016, 12:16 p.m. UTC | #2 
On 20 September 2016 at 13:02, Wu, Hao A <hao.a.wu@intel.com> wrote:
>> -----Original Message-----

>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard

>> Biesheuvel

>> Sent: Tuesday, September 20, 2016 4:38 PM

>> To: Wu, Hao A

>> Cc: leif.lindholm@linaro.org; edk2-devel@lists.01.org;

>> vishalo@qti.qualcomm.com; Gao, Liming

>> Subject: Re: [edk2] [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing

>> ASSERT()s

>>

>> On 20 September 2016 at 03:00, Wu, Hao A <hao.a.wu@intel.com> wrote:

>> > Hi Ard,

>> >

>> > The NULL checks for the input Guids in APIs CopyGuid(), CompareGuid() and

>> > IsZeroGuid() are implicitly done within calls to BaseLib APIs

>> > ReadUnaligned64() and WriteUnaligned64().

>> >

>> > So I think the functions behavior matches with their comments. What do you

>> > think?

>> >

>>

>> I disagree. ReadUnaligned64 and WriteUnaligned64 could theoretically

>> be implemented by a version of BaseLib that does not contain such

>> ASSERT()s

>

> The comments for APIs ReadUnaligned64 and WriteUnaligned64 in BaseLib

> mention the ASSERT() for inputting a NULL buffer.

>

> I think instances of BaseLib should follow the comments.

>


I agree with this. But that does not justify omitting them here.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Sept. 20, 2016, 12:40 p.m. UTC | #3 
On 20 September 2016 at 13:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 20 September 2016 at 13:02, Wu, Hao A <hao.a.wu@intel.com> wrote:

>>> -----Original Message-----

>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard

>>> Biesheuvel

>>> Sent: Tuesday, September 20, 2016 4:38 PM

>>> To: Wu, Hao A

>>> Cc: leif.lindholm@linaro.org; edk2-devel@lists.01.org;

>>> vishalo@qti.qualcomm.com; Gao, Liming

>>> Subject: Re: [edk2] [PATCH v2 2/3] MdePkg/BaseMemoryLib*: add missing

>>> ASSERT()s

>>>

>>> On 20 September 2016 at 03:00, Wu, Hao A <hao.a.wu@intel.com> wrote:

>>> > Hi Ard,

>>> >

>>> > The NULL checks for the input Guids in APIs CopyGuid(), CompareGuid() and

>>> > IsZeroGuid() are implicitly done within calls to BaseLib APIs

>>> > ReadUnaligned64() and WriteUnaligned64().

>>> >

>>> > So I think the functions behavior matches with their comments. What do you

>>> > think?

>>> >

>>>

>>> I disagree. ReadUnaligned64 and WriteUnaligned64 could theoretically

>>> be implemented by a version of BaseLib that does not contain such

>>> ASSERT()s

>>

>> The comments for APIs ReadUnaligned64 and WriteUnaligned64 in BaseLib

>> mention the ASSERT() for inputting a NULL buffer.

>>

>> I think instances of BaseLib should follow the comments.

>>

>

> I agree with this. But that does not justify omitting them here.


... but actually, it makes no sense to argue about this, since the
user visible outcome is the same. I will withdraw the patch.

Thanks,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

diff --git a/MdePkg/Library/BaseMemoryLib/MemLibGuid.c b/MdePkg/Library/BaseMemoryLib/MemLibGuid.c
index b2590f83caef..dff9bde653a9 100644
--- a/MdePkg/Library/BaseMemoryLib/MemLibGuid.c
+++ b/MdePkg/Library/BaseMemoryLib/MemLibGuid.c
@@ -47,6 +47,9 @@  CopyGuid (
   IN CONST GUID  *SourceGuid
   )
 {
+  ASSERT (DestinationGuid != NULL);
+  ASSERT (SourceGuid != NULL);
+
   WriteUnaligned64 (
     (UINT64*)DestinationGuid,
     ReadUnaligned64 ((CONST UINT64*)SourceGuid)
@@ -86,6 +89,9 @@  CompareGuid (
   UINT64  HighPartOfGuid1;
   UINT64  HighPartOfGuid2;
 
+  ASSERT (Guid1 != NULL);
+  ASSERT (Guid2 != NULL);
+
   LowPartOfGuid1  = ReadUnaligned64 ((CONST UINT64*) Guid1);
   LowPartOfGuid2  = ReadUnaligned64 ((CONST UINT64*) Guid2);
   HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
@@ -164,6 +170,8 @@  IsZeroGuid (
   UINT64  LowPartOfGuid;
   UINT64  HighPartOfGuid;
 
+  ASSERT (Guid != NULL);
+
   LowPartOfGuid  = ReadUnaligned64 ((CONST UINT64*) Guid);
   HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
 
diff --git a/MdePkg/Library/BaseMemoryLibMmx/MemLibGuid.c b/MdePkg/Library/BaseMemoryLibMmx/MemLibGuid.c
index cbb385fddfba..60babaf0dc49 100644
--- a/MdePkg/Library/BaseMemoryLibMmx/MemLibGuid.c
+++ b/MdePkg/Library/BaseMemoryLibMmx/MemLibGuid.c
@@ -47,6 +47,9 @@  CopyGuid (
   IN CONST GUID  *SourceGuid
   )
 {
+  ASSERT (DestinationGuid != NULL);
+  ASSERT (SourceGuid != NULL);
+
   WriteUnaligned64 (
     (UINT64*)DestinationGuid,
     ReadUnaligned64 ((CONST UINT64*)SourceGuid)
@@ -86,6 +89,9 @@  CompareGuid (
   UINT64  HighPartOfGuid1;
   UINT64  HighPartOfGuid2;
 
+  ASSERT (Guid1 != NULL);
+  ASSERT (Guid2 != NULL);
+
   LowPartOfGuid1  = ReadUnaligned64 ((CONST UINT64*) Guid1);
   LowPartOfGuid2  = ReadUnaligned64 ((CONST UINT64*) Guid2);
   HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
@@ -164,6 +170,8 @@  IsZeroGuid (
   UINT64  LowPartOfGuid;
   UINT64  HighPartOfGuid;
 
+  ASSERT (Guid != NULL);
+
   LowPartOfGuid  = ReadUnaligned64 ((CONST UINT64*) Guid);
   HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
 
diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/MemLibGuid.c b/MdePkg/Library/BaseMemoryLibOptDxe/MemLibGuid.c
index cbb385fddfba..60babaf0dc49 100644
--- a/MdePkg/Library/BaseMemoryLibOptDxe/MemLibGuid.c
+++ b/MdePkg/Library/BaseMemoryLibOptDxe/MemLibGuid.c
@@ -47,6 +47,9 @@  CopyGuid (
   IN CONST GUID  *SourceGuid
   )
 {
+  ASSERT (DestinationGuid != NULL);
+  ASSERT (SourceGuid != NULL);
+
   WriteUnaligned64 (
     (UINT64*)DestinationGuid,
     ReadUnaligned64 ((CONST UINT64*)SourceGuid)
@@ -86,6 +89,9 @@  CompareGuid (
   UINT64  HighPartOfGuid1;
   UINT64  HighPartOfGuid2;
 
+  ASSERT (Guid1 != NULL);
+  ASSERT (Guid2 != NULL);
+
   LowPartOfGuid1  = ReadUnaligned64 ((CONST UINT64*) Guid1);
   LowPartOfGuid2  = ReadUnaligned64 ((CONST UINT64*) Guid2);
   HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
@@ -164,6 +170,8 @@  IsZeroGuid (
   UINT64  LowPartOfGuid;
   UINT64  HighPartOfGuid;
 
+  ASSERT (Guid != NULL);
+
   LowPartOfGuid  = ReadUnaligned64 ((CONST UINT64*) Guid);
   HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
 
diff --git a/MdePkg/Library/BaseMemoryLibOptPei/MemLibGuid.c b/MdePkg/Library/BaseMemoryLibOptPei/MemLibGuid.c
index cbb385fddfba..60babaf0dc49 100644
--- a/MdePkg/Library/BaseMemoryLibOptPei/MemLibGuid.c
+++ b/MdePkg/Library/BaseMemoryLibOptPei/MemLibGuid.c
@@ -47,6 +47,9 @@  CopyGuid (
   IN CONST GUID  *SourceGuid
   )
 {
+  ASSERT (DestinationGuid != NULL);
+  ASSERT (SourceGuid != NULL);
+
   WriteUnaligned64 (
     (UINT64*)DestinationGuid,
     ReadUnaligned64 ((CONST UINT64*)SourceGuid)
@@ -86,6 +89,9 @@  CompareGuid (
   UINT64  HighPartOfGuid1;
   UINT64  HighPartOfGuid2;
 
+  ASSERT (Guid1 != NULL);
+  ASSERT (Guid2 != NULL);
+
   LowPartOfGuid1  = ReadUnaligned64 ((CONST UINT64*) Guid1);
   LowPartOfGuid2  = ReadUnaligned64 ((CONST UINT64*) Guid2);
   HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
@@ -164,6 +170,8 @@  IsZeroGuid (
   UINT64  LowPartOfGuid;
   UINT64  HighPartOfGuid;
 
+  ASSERT (Guid != NULL);
+
   LowPartOfGuid  = ReadUnaligned64 ((CONST UINT64*) Guid);
   HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
 
diff --git a/MdePkg/Library/BaseMemoryLibRepStr/MemLibGuid.c b/MdePkg/Library/BaseMemoryLibRepStr/MemLibGuid.c
index cbb385fddfba..60babaf0dc49 100644
--- a/MdePkg/Library/BaseMemoryLibRepStr/MemLibGuid.c
+++ b/MdePkg/Library/BaseMemoryLibRepStr/MemLibGuid.c
@@ -47,6 +47,9 @@  CopyGuid (
   IN CONST GUID  *SourceGuid
   )
 {
+  ASSERT (DestinationGuid != NULL);
+  ASSERT (SourceGuid != NULL);
+
   WriteUnaligned64 (
     (UINT64*)DestinationGuid,
     ReadUnaligned64 ((CONST UINT64*)SourceGuid)
@@ -86,6 +89,9 @@  CompareGuid (
   UINT64  HighPartOfGuid1;
   UINT64  HighPartOfGuid2;
 
+  ASSERT (Guid1 != NULL);
+  ASSERT (Guid2 != NULL);
+
   LowPartOfGuid1  = ReadUnaligned64 ((CONST UINT64*) Guid1);
   LowPartOfGuid2  = ReadUnaligned64 ((CONST UINT64*) Guid2);
   HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
@@ -164,6 +170,8 @@  IsZeroGuid (
   UINT64  LowPartOfGuid;
   UINT64  HighPartOfGuid;
 
+  ASSERT (Guid != NULL);
+
   LowPartOfGuid  = ReadUnaligned64 ((CONST UINT64*) Guid);
   HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
 
diff --git a/MdePkg/Library/BaseMemoryLibSse2/MemLibGuid.c b/MdePkg/Library/BaseMemoryLibSse2/MemLibGuid.c
index cbb385fddfba..60babaf0dc49 100644
--- a/MdePkg/Library/BaseMemoryLibSse2/MemLibGuid.c
+++ b/MdePkg/Library/BaseMemoryLibSse2/MemLibGuid.c
@@ -47,6 +47,9 @@  CopyGuid (
   IN CONST GUID  *SourceGuid
   )
 {
+  ASSERT (DestinationGuid != NULL);
+  ASSERT (SourceGuid != NULL);
+
   WriteUnaligned64 (
     (UINT64*)DestinationGuid,
     ReadUnaligned64 ((CONST UINT64*)SourceGuid)
@@ -86,6 +89,9 @@  CompareGuid (
   UINT64  HighPartOfGuid1;
   UINT64  HighPartOfGuid2;
 
+  ASSERT (Guid1 != NULL);
+  ASSERT (Guid2 != NULL);
+
   LowPartOfGuid1  = ReadUnaligned64 ((CONST UINT64*) Guid1);
   LowPartOfGuid2  = ReadUnaligned64 ((CONST UINT64*) Guid2);
   HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
@@ -164,6 +170,8 @@  IsZeroGuid (
   UINT64  LowPartOfGuid;
   UINT64  HighPartOfGuid;
 
+  ASSERT (Guid != NULL);
+
   LowPartOfGuid  = ReadUnaligned64 ((CONST UINT64*) Guid);
   HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
 
diff --git a/MdePkg/Library/PeiMemoryLib/MemLibGuid.c b/MdePkg/Library/PeiMemoryLib/MemLibGuid.c
index cbb385fddfba..60babaf0dc49 100644
--- a/MdePkg/Library/PeiMemoryLib/MemLibGuid.c
+++ b/MdePkg/Library/PeiMemoryLib/MemLibGuid.c
@@ -47,6 +47,9 @@  CopyGuid (
   IN CONST GUID  *SourceGuid
   )
 {
+  ASSERT (DestinationGuid != NULL);
+  ASSERT (SourceGuid != NULL);
+
   WriteUnaligned64 (
     (UINT64*)DestinationGuid,
     ReadUnaligned64 ((CONST UINT64*)SourceGuid)
@@ -86,6 +89,9 @@  CompareGuid (
   UINT64  HighPartOfGuid1;
   UINT64  HighPartOfGuid2;
 
+  ASSERT (Guid1 != NULL);
+  ASSERT (Guid2 != NULL);
+
   LowPartOfGuid1  = ReadUnaligned64 ((CONST UINT64*) Guid1);
   LowPartOfGuid2  = ReadUnaligned64 ((CONST UINT64*) Guid2);
   HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
@@ -164,6 +170,8 @@  IsZeroGuid (
   UINT64  LowPartOfGuid;
   UINT64  HighPartOfGuid;
 
+  ASSERT (Guid != NULL);
+
   LowPartOfGuid  = ReadUnaligned64 ((CONST UINT64*) Guid);
   HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);
 
diff --git a/MdePkg/Library/UefiMemoryLib/MemLibGuid.c b/MdePkg/Library/UefiMemoryLib/MemLibGuid.c
index cbb385fddfba..60babaf0dc49 100644
--- a/MdePkg/Library/UefiMemoryLib/MemLibGuid.c
+++ b/MdePkg/Library/UefiMemoryLib/MemLibGuid.c
@@ -47,6 +47,9 @@  CopyGuid (
   IN CONST GUID  *SourceGuid
   )
 {
+  ASSERT (DestinationGuid != NULL);
+  ASSERT (SourceGuid != NULL);
+
   WriteUnaligned64 (
     (UINT64*)DestinationGuid,
     ReadUnaligned64 ((CONST UINT64*)SourceGuid)
@@ -86,6 +89,9 @@  CompareGuid (
   UINT64  HighPartOfGuid1;
   UINT64  HighPartOfGuid2;
 
+  ASSERT (Guid1 != NULL);
+  ASSERT (Guid2 != NULL);
+
   LowPartOfGuid1  = ReadUnaligned64 ((CONST UINT64*) Guid1);
   LowPartOfGuid2  = ReadUnaligned64 ((CONST UINT64*) Guid2);
   HighPartOfGuid1 = ReadUnaligned64 ((CONST UINT64*) Guid1 + 1);
@@ -164,6 +170,8 @@  IsZeroGuid (
   UINT64  LowPartOfGuid;
   UINT64  HighPartOfGuid;
 
+  ASSERT (Guid != NULL);
+
   LowPartOfGuid  = ReadUnaligned64 ((CONST UINT64*) Guid);
   HighPartOfGuid = ReadUnaligned64 ((CONST UINT64*) Guid + 1);