1. Patchew
  2. EDK2
  3. View series

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

Laszlo Ersek posted 4 patches 6 years, 2 months ago
Failed in applying to current master (apply log)
MdePkg/Library/BaseSafeIntLib/SafeIntLib.c | 110 ++++++++++++++++----
1 file changed, 88 insertions(+), 22 deletions(-)
[edk2] [PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult
Posted by Laszlo Ersek 6 years, 2 months ago 
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: [edk2] [PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult
Posted by Ard Biesheuvel 6 years, 2 months ago 
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult
Posted by Laszlo Ersek 6 years, 2 months ago 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult
Posted by Kinney, Michael D 6 years, 2 months ago 
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult
Posted by Laszlo Ersek 6 years, 2 months ago 
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: [edk2] [PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult
Posted by Kinney, Michael D 6 years, 2 months ago 
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
> >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult
Posted by Laszlo Ersek 6 years, 1 month ago 
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
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult
Posted by Ard Biesheuvel 6 years, 1 month ago 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.