Re: [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance

[Bret Barkelew <Bret.Barkelew@microsoft.com>]

Ah, yes, this makes more sense. It was difficult to review all your comments on my phone.

Thanks for clarifying!

- Bret

From: Laszlo Ersek<mailto:lersek@redhat.com>
Sent: Tuesday, February 13, 2018 9:29 AM
To: Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; Kinney, Michael D<mailto:michael.d.kinney@intel.com>; Sean Brogan<mailto:sean.brogan@microsoft.com>
Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Gao, Liming<mailto:liming.gao@intel.com>
Subject: Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance

On 02/13/18 17:56, Bret Barkelew wrote:
> In response to the original question, I would content that our goal
> should be "a". We should be allowing universal detection of errors
> without the caller having to carry this detection code itself.

OK.

The question is how the detection is implemented internally. Is that
implementation (in edk2) allowed to rely on behavior that the ISO C
standard leaves undefined, and -- consequently -- compilers might
exploit for code generation?

> The analog would be the safe string functions: if a buffer overflow
> occurs, they don't find a way to "fix" the operation, but they
> faithfully report an error.

Precisely.

Correspondingly, translated to the safe string functions, my question
becomes:

Is the implementation of the safe string functions allowed to employ
buffer overflow internally, and detect the overflow after the fact?

> As such, I believe from my review that these functions work as
> intended.

Please let me quote the function again, from
"MdePkg/Library/BaseSafeIntLib/SafeIntLib.c":

  3831  RETURN_STATUS
  3832  EFIAPI
  3833  SafeInt64Sub (
  3834    IN  INT64  Minuend,
  3835    IN  INT64  Subtrahend,
  3836    OUT INT64  *Result
  3837    )
  3838  {
  3839    RETURN_STATUS  Status;
  3840    INT64          SignedResult;
  3841
  3842    if (Result == NULL) {
  3843      return RETURN_INVALID_PARAMETER;
  3844    }
  3845
  3846    SignedResult = Minuend - Subtrahend;
  3847
  3848    //
  3849    // Subtracting a positive number from a positive number never overflows.
  3850    // Subtracting a negative number from a negative number never overflows.
  3851    // If you subtract a negative number from a positive number, you expect a positive result.
  3852    // If you subtract a positive number from a negative number, you expect a negative result.
  3853    // Overflow if inputs vary in sign and the output does not have the same sign as the first input.
  3854    //
  3855    if (((Minuend < 0) != (Subtrahend < 0)) &&
  3856        ((Minuend < 0) != (SignedResult < 0))) {
  3857      *Result = INT64_ERROR;
  3858      Status = RETURN_BUFFER_TOO_SMALL;
  3859    } else {
  3860      *Result = SignedResult;
  3861      Status = RETURN_SUCCESS;
  3862    }
  3863
  3864    return Status;
  3865  }

On line 3846, integer overflow is possible. If that happens, not only is
the resultant value of "SignedResult" undefined, but the behavior of the
rest of the function is undefined, according to ISO C.

In other words, just because we move the checking into a library
function, we cannot check *after the fact*. The subtraction on line 3846
can invoke undefined behavior *first*, and we check only afterwards,
starting on line 3855.

Here's an analogy. Various C compilers regularly equate "undefined
behavior" with "never happens" (which is a valid interpretation of the
ISO C standard for the compiler to make). For example, given the code

  int f(int *x)
  {
    *x = 3;
    if (x == NULL) {
      return 1;
    }
    return 0;
}

the compiler may compile f() to unconditionally return 0, such as:

  int f(int *x)
  {
    *x = 3;
    return 0;
  }

Because, the (x == NULL) branch would depend on undefined behavior
invoked by the assignment to *x. Given that the (*x = 3) assignment is
undefined for (x==NULL), according to ISO C, the subsequent (x == NULL)
check can be taken as constant false, and eliminated.

Similarly, a sufficiently smart compiler may assume that the subtraction
on line 3846 will never overflow. (Because, such an overflow would be
undefined behavior.) Consequently, it may deduce that the overflow
checks, *after the fact*, evaluate to constant false, and can be
eliminated. It might compile the function as in:

  {
    RETURN_STATUS  Status;
    INT64          SignedResult;

    if (Result == NULL) {
      return RETURN_INVALID_PARAMETER;
    }

    SignedResult = Minuend - Subtrahend;

    *Result = SignedResult;
    Status = RETURN_SUCCESS;

    return Status;
  }

I apoligize if I'm unclear; I really don't know how to put it better.
The subtraction on line 3846 runs the risk of undefined behavior, and
the checks starting on line 3855 are too late.

Thanks
Laszlo