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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
