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

[Laszlo Ersek <lersek@redhat.com>]

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