[PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult

[PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult

[Laszlo Ersek]

Repo:   https://github.com/lersek/edk2.git
Branch: signed_range_checks

Based on the discussion starting at
<https://lists.01.org/pipermail/edk2-devel/2018-February/021178.html>.

Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>

Laszlo Ersek (4):
  MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Sub()
  MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Add()
  MdePkg/BaseSafeIntLib: clean up parentheses in MIN_INT64_MAGNITUDE
  MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Mult()

 MdePkg/Library/BaseSafeIntLib/SafeIntLib.c | 110 ++++++++++++++++----
 1 file changed, 88 insertions(+), 22 deletions(-)

-- 
2.14.1.3.gb7cf6e02401b

[PATCH 1/4] MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Sub()

[Laszlo Ersek]

The subtraction in the assignment

  SignedResult = Minuend - Subtrahend;

is performed with unchecked INT64 operands. According to ISO C, if the
mathematical result of signed integer subtraction cannot be represented in
the result type, the behavior is undefined. (Refer to ISO C99 6.5p5.
6.2.5p9 only exempts unsigned integers, and 6.3.1.3p3 does not apply
because it treats the conversion of integers that have been successfully
evaluated first.)

Replace the after-the-fact result checking with checks on the operands,
and only perform the subtraction if it is safe.

Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdePkg/Library/BaseSafeIntLib/SafeIntLib.c | 50 +++++++++++++++-----
 1 file changed, 39 insertions(+), 11 deletions(-)

diff --git a/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c b/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c
index d846160ba0d1..8e857927b067 100644
--- a/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c
+++ b/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c
@@ -3837,27 +3837,55 @@ SafeInt64Sub (
   )
 {
   RETURN_STATUS  Status;
-  INT64          SignedResult;
 
   if (Result == NULL) {
     return RETURN_INVALID_PARAMETER;
   }
 
-  SignedResult = Minuend - Subtrahend;
-
   //
-  // Subtracting a positive number from a positive number never overflows.
-  // Subtracting a negative number from a negative number never overflows.
-  // If you subtract a negative number from a positive number, you expect a positive result.
-  // If you subtract a positive number from a negative number, you expect a negative result.
-  // Overflow if inputs vary in sign and the output does not have the same sign as the first input.
+  // * A Subtrahend of zero can never cause underflow or overflow.
   //
-  if (((Minuend < 0) != (Subtrahend < 0)) &&
-      ((Minuend < 0) != (SignedResult < 0))) {
+  // * A positive Subtrahend can only cause underflow. The underflow condition
+  //   is:
+  //
+  //     (Minuend - Subtrahend) < MIN_INT64
+  //
+  //   Adding Subtrahend to both sides yields
+  //
+  //     Minuend < (MIN_INT64 + Subtrahend)
+  //
+  //   This condition can be coded directly in C because the RHS will neither
+  //   underflow nor overflow. That is due to the starting condition:
+  //
+  //     0 < Subtrahend <= MAX_INT64
+  //
+  //   Adding MIN_INT64 to all three sides yields
+  //
+  //     MIN_INT64 < (MIN_INT64 + Subtrahend) <= (MIN_INT64 + MAX_INT64) = -1
+  //
+  // * A negative Subtrahend can only cause overflow. The overflow condition is
+  //
+  //     (Minuend - Subtrahend) > MAX_INT64
+  //
+  //   Adding Subtrahend to both sides yields
+  //
+  //     Minuend > (MAX_INT64 + Subtrahend)
+  //
+  //   This condition can be coded directly in C because the RHS will neither
+  //   underflow nor overflow. That is due to the starting condition:
+  //
+  //     MIN_INT64 <= Subtrahend < 0
+  //
+  //   Adding MAX_INT64 to all three sides yields
+  //
+  //     -1 = (MAX_INT64 + MIN_INT64) <= (MAX_INT64 + Subtrahend) < MAX_INT64
+  //
+  if (((Subtrahend > 0) && (Minuend < (MIN_INT64 + Subtrahend))) ||
+      ((Subtrahend < 0) && (Minuend > (MAX_INT64 + Subtrahend)))) {
     *Result = INT64_ERROR;
     Status = RETURN_BUFFER_TOO_SMALL;
   } else {
-    *Result = SignedResult;
+    *Result = Minuend - Subtrahend;
     Status = RETURN_SUCCESS;
   }
 
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 2/4] MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Add()

[Laszlo Ersek]

The addition in the assignment

  SignedResult = Augend + Addend;

is performed with unchecked INT64 operands. According to ISO C, if the
mathematical result of signed integer addition cannot be represented in
the result type, the behavior is undefined. (Refer to ISO C99 6.5p5.
6.2.5p9 only exempts unsigned integers, and 6.3.1.3p3 does not apply
because it treats the conversion of integers that have been successfully
evaluated first.)

Replace the after-the-fact result checking with checks on the operands,
and only perform the addition if it is safe.

Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdePkg/Library/BaseSafeIntLib/SafeIntLib.c | 56 ++++++++++++++++----
 1 file changed, 46 insertions(+), 10 deletions(-)

diff --git a/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c b/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c
index 8e857927b067..56d97cf65601 100644
--- a/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c
+++ b/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c
@@ -3631,26 +3631,62 @@ SafeInt64Add (
   )
 {
   RETURN_STATUS  Status;
-  INT64          SignedResult;
 
   if (Result == NULL) {
     return RETURN_INVALID_PARAMETER;
   }
 
-  SignedResult = Augend + Addend;
-
   //
-  // Adding positive to negative never overflows.
-  // If you add two positive numbers, you expect a positive result.
-  // If you add two negative numbers, you expect a negative result.
-  // Overflow if inputs are the same sign and output is not that sign.
+  // * An Addend of zero can never cause underflow or overflow.
   //
-  if (((Augend < 0) == (Addend < 0))  &&
-      ((Augend < 0) != (SignedResult < 0))) {
+  // * A positive Addend can only cause overflow. The overflow condition is
+  //
+  //     (Augend + Addend) > MAX_INT64
+  //
+  //   Subtracting Addend from both sides yields
+  //
+  //     Augend > (MAX_INT64 - Addend)
+  //
+  //   This condition can be coded directly in C because the RHS will neither
+  //   underflow nor overflow. That is due to the starting condition:
+  //
+  //     0 < Addend <= MAX_INT64
+  //
+  //   Multiplying all three sides by (-1) yields
+  //
+  //     0 > (-Addend) >= (-MAX_INT64)
+  //
+  //   Adding MAX_INT64 to all three sides yields
+  //
+  //     MAX_INT64 > (MAX_INT64 - Addend) >= 0
+  //
+  // * A negative Addend can only cause underflow. The underflow condition is
+  //
+  //     (Augend + Addend) < MIN_INT64
+  //
+  //   Subtracting Addend from both sides yields
+  //
+  //     Augend < (MIN_INT64 - Addend)
+  //
+  //   This condition can be coded directly in C because the RHS will neither
+  //   underflow nor overflow. That is due to the starting condition:
+  //
+  //     MIN_INT64 <= Addend < 0
+  //
+  //   Multiplying all three sides by (-1) yields
+  //
+  //     (-MIN_INT64) >= (-Addend) > 0
+  //
+  //   Adding MIN_INT64 to all three sides yields
+  //
+  //     0 >= (MIN_INT64 - Addend) > MIN_INT64
+  //
+  if (((Addend > 0) && (Augend > (MAX_INT64 - Addend))) ||
+      ((Addend < 0) && (Augend < (MIN_INT64 - Addend)))) {
     *Result = INT64_ERROR;
     Status = RETURN_BUFFER_TOO_SMALL;
   } else {
-    *Result = SignedResult;
+    *Result = Augend + Addend;
     Status = RETURN_SUCCESS;
   }
 
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 3/4] MdePkg/BaseSafeIntLib: clean up parentheses in MIN_INT64_MAGNITUDE

[Laszlo Ersek]

The definition of the MIN_INT64_MAGNITUDE macro is correct, but it's
harder to read than necessary: the sub-expression

      (( (UINT64) - (MIN_INT64 + 1) ))

is doubly parenthesized. Reusing one pair of the outer parens, rewrite the
sub-expression (without change in meaning) so that the minus sign cannot
be mistaken for subtraction:

      ( (UINT64)(- (MIN_INT64 + 1)) )

The resultant macro definition matches the following expressions in
SafeInt64Mult():

>     //
>     // Avoid negating the most negative number.
>     //
>     UnsignedMultiplicand = ((UINT64)(- (Multiplicand + 1))) + 1;

and

>     //
>     // Avoid negating the most negative number.
>     //
>     UnsignedMultiplier = ((UINT64)(- (Multiplier + 1))) + 1;

Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdePkg/Library/BaseSafeIntLib/SafeIntLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c b/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c
index 56d97cf65601..de91ffeca2a5 100644
--- a/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c
+++ b/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c
@@ -33,7 +33,7 @@
 //
 // Magnitude of MIN_INT64 as expressed by a UINT64 number.
 //
-#define MIN_INT64_MAGNITUDE ((((UINT64) - (MIN_INT64 + 1))) + 1)
+#define MIN_INT64_MAGNITUDE (((UINT64)(- (MIN_INT64 + 1))) + 1)
 
 //
 // Conversion functions
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 4/4] MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Mult()

[Laszlo Ersek]

If we have to negate UnsignedResult (due to exactly one of Multiplicand
and Multiplier being negative), and UnsignedResult is exactly
MIN_INT64_MAGNITUDE (value 2^63), then the statement

        *Result = - ((INT64)UnsignedResult);

invokes both implementation-defined behavior and undefined behavior.

First, MIN_INT64_MAGNITUDE is not representable as INT64, therefore the
result of the (inner) conversion

  (INT64)MIN_INT64_MAGNITUDE

is implementation-defined, or an implementation-defined signal is raised,
according to ISO C99 6.3.1.3p3.

Second, if we assume that the C language implementation defines the
conversion to INT64 simply as reinterpreting the bit pattern
0x8000_0000_0000_0000 as a signed integer in two's complement
representation, then the conversion immediately produces the negative
value MIN_INT64 (value -(2^63)). In turn, the (outer) negation

  -(MIN_INT64)

invokes undefined behavior, because the mathematical result of the
negation, namely 2^63, cannot be represented in an INT64 object. (Not even
mentioning the fact that the mathematical result would be incorrect.) In
practice, the undefined negation of MIN_INT64 happens to produce an
unchanged, valid-looking result on x86, i.e. (-(MIN_INT64)) == MIN_INT64.

We can summarize this as the undefined -- effectless -- negation canceling
out the botched -- auto-negating -- implementation-defined conversion.
Instead of relying on such behavior, dedicate a branch to this situation:
assign MIN_INT64 directly. The branch can be triggered e.g. by multiplying
(2^62) by (-2).

Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdePkg/Library/BaseSafeIntLib/SafeIntLib.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c b/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c
index de91ffeca2a5..c5f13d7e0828 100644
--- a/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c
+++ b/MdePkg/Library/BaseSafeIntLib/SafeIntLib.c
@@ -4143,6 +4143,8 @@ SafeInt64Mult (
       if (UnsignedResult > MIN_INT64_MAGNITUDE) {
         *Result = INT64_ERROR;
         Status = RETURN_BUFFER_TOO_SMALL;
+      } else if (UnsignedResult == MIN_INT64_MAGNITUDE) {
+        *Result = MIN_INT64;
       } else {
         *Result = - ((INT64)UnsignedResult);
       }
-- 
2.14.1.3.gb7cf6e02401b

Re: [PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult

[Ard Biesheuvel]

On 15 February 2018 at 18:36, Laszlo Ersek <lersek@redhat.com> wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: signed_range_checks
>
> Based on the discussion starting at
> <https://lists.01.org/pipermail/edk2-devel/2018-February/021178.html>.
>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
>
> Laszlo Ersek (4):
>   MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Sub()
>   MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Add()
>   MdePkg/BaseSafeIntLib: clean up parentheses in MIN_INT64_MAGNITUDE
>   MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Mult()
>
>  MdePkg/Library/BaseSafeIntLib/SafeIntLib.c | 110 ++++++++++++++++----
>  1 file changed, 88 insertions(+), 22 deletions(-)
>

Hello Laszlo,

Thanks a lot for taking the time to fix this library. I am not a C
scholar, but I have reviewed these patches to the best of my
abilities.

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

I take it we don't need to add -fwrapv now?

Thanks again,
Ard.

Re: [PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult

[Laszlo Ersek]

On 02/16/18 12:28, Ard Biesheuvel wrote:
> On 15 February 2018 at 18:36, Laszlo Ersek <lersek@redhat.com> wrote:
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: signed_range_checks
>>
>> Based on the discussion starting at
>> <https://lists.01.org/pipermail/edk2-devel/2018-February/021178.html>.
>>
>> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Sean Brogan <sean.brogan@microsoft.com>
>>
>> Laszlo Ersek (4):
>>   MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Sub()
>>   MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Add()
>>   MdePkg/BaseSafeIntLib: clean up parentheses in MIN_INT64_MAGNITUDE
>>   MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Mult()
>>
>>  MdePkg/Library/BaseSafeIntLib/SafeIntLib.c | 110 ++++++++++++++++----
>>  1 file changed, 88 insertions(+), 22 deletions(-)
>>
> 
> Hello Laszlo,
> 
> Thanks a lot for taking the time to fix this library. I am not a C
> scholar, but I have reviewed these patches to the best of my
> abilities.
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Great, thank you!

> I take it we don't need to add -fwrapv now?

That's my understanding.

Before starting work on this series, I tried to investigate how far
"-fwrapv" support goes back, considering edk2's toolchains.

With gcc, the earliest version we target is gcc-4.3 (not due to GCC4x
but to UNIXGCC, ELFGCC (presumably), and CYGGCC). "-fwrapv" is available
in gcc-4.3, according to the documentation.

Under CLANG38, "-fwrapv" is also available (I have clang-3.8.1 installed
locally).

However, I couldn't check:
- any VS toolchain
- CLANG35 (the online docs don't seem to list "-fwrapv" -- in fact I
failed to find comprehensive docs for clang-3.5)
- ICC / RVCT / XCODE5 / ...

So, I thought it'd be best to make the code safe.

These patches should cover the signed integer "workhorse" functions, so
I don't think we need "-fwrapv" right now. I also skimmed the rest of
"MdePkg/Library/BaseSafeIntLib/SafeIntLib.c", and given the time I could
spend, things looked OK.

Thank you!
Laszlo

Re: [PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult

[Kinney, Michael D]

Hi Laszlo,

This patch series passed the SafeIntLib unit
tests for the following builds:

* VS2015x86 IA32
* VS2015x86 X64
* VS2015x86 EBC
* GCC IA32
* GCC X64

Tested-by: Michael D Kinney <michael.d.kinney@intel.com>

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, February 15, 2018 10:37 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Gao,
> Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Sean Brogan
> <sean.brogan@microsoft.com>
> Subject: [PATCH 0/4] MdePkg/BaseSafeIntLib: fix
> undefined behavior in INT64 Sub/Add/Mult
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: signed_range_checks
> 
> Based on the discussion starting at
> <https://lists.01.org/pipermail/edk2-devel/2018-
> February/021178.html>.
> 
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> 
> Laszlo Ersek (4):
>   MdePkg/BaseSafeIntLib: fix undefined behavior in
> SafeInt64Sub()
>   MdePkg/BaseSafeIntLib: fix undefined behavior in
> SafeInt64Add()
>   MdePkg/BaseSafeIntLib: clean up parentheses in
> MIN_INT64_MAGNITUDE
>   MdePkg/BaseSafeIntLib: fix undefined behavior in
> SafeInt64Mult()
> 
>  MdePkg/Library/BaseSafeIntLib/SafeIntLib.c | 110
> ++++++++++++++++----
>  1 file changed, 88 insertions(+), 22 deletions(-)
> 
> --
> 2.14.1.3.gb7cf6e02401b

Re: [PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult

[Laszlo Ersek]

On 02/16/18 19:11, Kinney, Michael D wrote:
> Hi Laszlo,
> 
> This patch series passed the SafeIntLib unit
> tests for the following builds:
> 
> * VS2015x86 IA32
> * VS2015x86 X64
> * VS2015x86 EBC
> * GCC IA32
> * GCC X64
> 
> Tested-by: Michael D Kinney <michael.d.kinney@intel.com>

Awesome, thank you! I've been secretly hoping that there's a test suite
for this library :)

Given Ard's R-b and your maintainer T-b, should I wait for more feedback
from Bret, Liming and Sean, before I push the series? (I'm fine waiting
a few more days if they intend to provide feedback; I just wouldn't like
to wait in vain.)

Thank you!
Laszlo

> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Thursday, February 15, 2018 10:37 AM
>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Gao,
>> Liming <liming.gao@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Sean Brogan
>> <sean.brogan@microsoft.com>
>> Subject: [PATCH 0/4] MdePkg/BaseSafeIntLib: fix
>> undefined behavior in INT64 Sub/Add/Mult
>>
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: signed_range_checks
>>
>> Based on the discussion starting at
>> <https://lists.01.org/pipermail/edk2-devel/2018-
>> February/021178.html>.
>>
>> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Sean Brogan <sean.brogan@microsoft.com>
>>
>> Laszlo Ersek (4):
>>   MdePkg/BaseSafeIntLib: fix undefined behavior in
>> SafeInt64Sub()
>>   MdePkg/BaseSafeIntLib: fix undefined behavior in
>> SafeInt64Add()
>>   MdePkg/BaseSafeIntLib: clean up parentheses in
>> MIN_INT64_MAGNITUDE
>>   MdePkg/BaseSafeIntLib: fix undefined behavior in
>> SafeInt64Mult()
>>
>>  MdePkg/Library/BaseSafeIntLib/SafeIntLib.c | 110
>> ++++++++++++++++----
>>  1 file changed, 88 insertions(+), 22 deletions(-)
>>
>> --
>> 2.14.1.3.gb7cf6e02401b
> 

Re: [PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult

[Kinney, Michael D]

Laszlo,

The tests for the SafeIntLib are in an edk2-staging
branch.

https://github.com/tianocore/edk2-staging/tree/edk2-test


In this directory

https://github.com/tianocore/edk2-staging/tree/edk2-test/MdePkgUnitTest/SafeIntLib

Thanks for the very detailed analysis/comments in the patch.

Let's wait till Tuesday next week for any additional feedback.

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, February 16, 2018 12:50 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Gao,
> Liming <liming.gao@intel.com>; Sean Brogan
> <sean.brogan@microsoft.com>
> Subject: Re: [PATCH 0/4] MdePkg/BaseSafeIntLib: fix
> undefined behavior in INT64 Sub/Add/Mult
> 
> On 02/16/18 19:11, Kinney, Michael D wrote:
> > Hi Laszlo,
> >
> > This patch series passed the SafeIntLib unit
> > tests for the following builds:
> >
> > * VS2015x86 IA32
> > * VS2015x86 X64
> > * VS2015x86 EBC
> > * GCC IA32
> > * GCC X64
> >
> > Tested-by: Michael D Kinney
> <michael.d.kinney@intel.com>
> 
> Awesome, thank you! I've been secretly hoping that
> there's a test suite
> for this library :)
> 
> Given Ard's R-b and your maintainer T-b, should I wait
> for more feedback
> from Bret, Liming and Sean, before I push the series?
> (I'm fine waiting
> a few more days if they intend to provide feedback; I
> just wouldn't like
> to wait in vain.)
> 
> Thank you!
> Laszlo
> 
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Thursday, February 15, 2018 10:37 AM
> >> To: edk2-devel-01 <edk2-devel@lists.01.org>
> >> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Gao,
> >> Liming <liming.gao@intel.com>; Kinney, Michael D
> >> <michael.d.kinney@intel.com>; Sean Brogan
> >> <sean.brogan@microsoft.com>
> >> Subject: [PATCH 0/4] MdePkg/BaseSafeIntLib: fix
> >> undefined behavior in INT64 Sub/Add/Mult
> >>
> >> Repo:   https://github.com/lersek/edk2.git
> >> Branch: signed_range_checks
> >>
> >> Based on the discussion starting at
> >> <https://lists.01.org/pipermail/edk2-devel/2018-
> >> February/021178.html>.
> >>
> >> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> >> Cc: Liming Gao <liming.gao@intel.com>
> >> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >> Cc: Sean Brogan <sean.brogan@microsoft.com>
> >>
> >> Laszlo Ersek (4):
> >>   MdePkg/BaseSafeIntLib: fix undefined behavior in
> >> SafeInt64Sub()
> >>   MdePkg/BaseSafeIntLib: fix undefined behavior in
> >> SafeInt64Add()
> >>   MdePkg/BaseSafeIntLib: clean up parentheses in
> >> MIN_INT64_MAGNITUDE
> >>   MdePkg/BaseSafeIntLib: fix undefined behavior in
> >> SafeInt64Mult()
> >>
> >>  MdePkg/Library/BaseSafeIntLib/SafeIntLib.c | 110
> >> ++++++++++++++++----
> >>  1 file changed, 88 insertions(+), 22 deletions(-)
> >>
> >> --
> >> 2.14.1.3.gb7cf6e02401b
> >

Re: [PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult

[Laszlo Ersek]

On 02/17/18 04:07, Kinney, Michael D wrote:
> Laszlo,
> 
> The tests for the SafeIntLib are in an edk2-staging
> branch.
> 
> https://github.com/tianocore/edk2-staging/tree/edk2-test
> 
> 
> In this directory
> 
> https://github.com/tianocore/edk2-staging/tree/edk2-test/MdePkgUnitTest/SafeIntLib
> 
> Thanks for the very detailed analysis/comments in the patch.
> 
> Let's wait till Tuesday next week for any additional feedback.

Thanks everyone; series pushed as 44e6186eeadf..75505d161133.

Cheers
Laszlo


>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Friday, February 16, 2018 12:50 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>;
>> edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Gao,
>> Liming <liming.gao@intel.com>; Sean Brogan
>> <sean.brogan@microsoft.com>
>> Subject: Re: [PATCH 0/4] MdePkg/BaseSafeIntLib: fix
>> undefined behavior in INT64 Sub/Add/Mult
>>
>> On 02/16/18 19:11, Kinney, Michael D wrote:
>>> Hi Laszlo,
>>>
>>> This patch series passed the SafeIntLib unit
>>> tests for the following builds:
>>>
>>> * VS2015x86 IA32
>>> * VS2015x86 X64
>>> * VS2015x86 EBC
>>> * GCC IA32
>>> * GCC X64
>>>
>>> Tested-by: Michael D Kinney
>> <michael.d.kinney@intel.com>
>>
>> Awesome, thank you! I've been secretly hoping that
>> there's a test suite
>> for this library :)
>>
>> Given Ard's R-b and your maintainer T-b, should I wait
>> for more feedback
>> from Bret, Liming and Sean, before I push the series?
>> (I'm fine waiting
>> a few more days if they intend to provide feedback; I
>> just wouldn't like
>> to wait in vain.)
>>
>> Thank you!
>> Laszlo
>>
>>>
>>>> -----Original Message-----
>>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>>> Sent: Thursday, February 15, 2018 10:37 AM
>>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>>>> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Gao,
>>>> Liming <liming.gao@intel.com>; Kinney, Michael D
>>>> <michael.d.kinney@intel.com>; Sean Brogan
>>>> <sean.brogan@microsoft.com>
>>>> Subject: [PATCH 0/4] MdePkg/BaseSafeIntLib: fix
>>>> undefined behavior in INT64 Sub/Add/Mult
>>>>
>>>> Repo:   https://github.com/lersek/edk2.git
>>>> Branch: signed_range_checks
>>>>
>>>> Based on the discussion starting at
>>>> <https://lists.01.org/pipermail/edk2-devel/2018-
>>>> February/021178.html>.
>>>>
>>>> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
>>>> Cc: Liming Gao <liming.gao@intel.com>
>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>>> Cc: Sean Brogan <sean.brogan@microsoft.com>
>>>>
>>>> Laszlo Ersek (4):
>>>>   MdePkg/BaseSafeIntLib: fix undefined behavior in
>>>> SafeInt64Sub()
>>>>   MdePkg/BaseSafeIntLib: fix undefined behavior in
>>>> SafeInt64Add()
>>>>   MdePkg/BaseSafeIntLib: clean up parentheses in
>>>> MIN_INT64_MAGNITUDE
>>>>   MdePkg/BaseSafeIntLib: fix undefined behavior in
>>>> SafeInt64Mult()
>>>>
>>>>  MdePkg/Library/BaseSafeIntLib/SafeIntLib.c | 110
>>>> ++++++++++++++++----
>>>>  1 file changed, 88 insertions(+), 22 deletions(-)
>>>>
>>>> --
>>>> 2.14.1.3.gb7cf6e02401b
>>>
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult

[Ard Biesheuvel]

On 21 February 2018 at 11:00, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/17/18 04:07, Kinney, Michael D wrote:
>> Laszlo,
>>
>> The tests for the SafeIntLib are in an edk2-staging
>> branch.
>>
>> https://github.com/tianocore/edk2-staging/tree/edk2-test
>>
>>
>> In this directory
>>
>> https://github.com/tianocore/edk2-staging/tree/edk2-test/MdePkgUnitTest/SafeIntLib
>>
>> Thanks for the very detailed analysis/comments in the patch.
>>
>> Let's wait till Tuesday next week for any additional feedback.
>
> Thanks everyone; series pushed as 44e6186eeadf..75505d161133.
>
> Cheers
> Laszlo
>

Thanks again Laszlo