Undefined behavior in `Py_ABS()` macro with `LLONG_MIN`

[sethmlarson]

Bug report

Bug description:

When the value passed to Py_ABS() macro is LLONG_MIN (or any min value of a signed integer), the macro suffers from undefined behavior. This was detected in a failure to build the OSS-Fuzz test suite.

CPython versions tested on:

CPython main branch

Operating systems tested on:

No response

Linked PRs

• gh-141370: document undefined behavior of Py_ABS() #141439
• [3.14] gh-141370: document undefined behavior of Py_ABS() (GH-141439) #141454
• [3.13] gh-141370: document undefined behavior of Py_ABS() (GH-141439) #141455
• gh-141370: Fix undefined behavior when using Py_ABS() #141548
• [3.14] gh-141370: Fix undefined behavior when using Py_ABS() (GH-141548) #142301
• [3.13] gh-141370: Fix undefined behavior when using Py_ABS() (GH-141548) #142304

[skirpichev]

I don't think it's an issue. Same happens for abs/labs/llabs from the C stdlib, when the absolute value can't be represented in two’s complement:

$ cat a.c
#include <stdlib.h>
#include <limits.h>
#include <stdio.h>
int main()
{
    long long lmin = LLONG_MIN, lmax = LLONG_MAX;

    printf("%lld %lld %lld\n", lmin, llabs(lmin), lmax);
    return 0;
}
$ cc a.c && ./a.out
-9223372036854775808 -9223372036854775808 9223372036854775807

I suggest either close this or reclassify as a documentation issue. We could add remark like this: "If the result cannot be represented, the behavior is undefined."

[serhiy-storchaka]

This is partially a documentation issue, but we should also analyze every use of Py_ABS in the C code.

[skirpichev]

I see few instances:

1. float_as_integer_ratio_impl() in Objects/floatobject.c: here we are safe, because the exponent range will not hit INT_MIN (even for quadruple precision it fits into 16bit int).
2. _PyLong_GCD() in Objects/longobject.c: here we just can't have LONG_MIN or LLONG_MIN value in PyLong with <= 2 digits.
3. w_PyLong() in Python/marshal.c: here we cast result to unsigned type.
4. r_PyLong() in Python/marshal.c: here we are safe, because -SIZE32_MAX <= n <= SIZE32_MAX
5. _Py_strhex_impl() in Python/pystrhex.c: again cast to unsigned
6. pytime_divide() in Python/pytime.c: here is just integer division with divisor k>=2.

If you think all is ok with above, I'll reclassify this as a documentation issue.

$ git grep 'Py_ABS *('
Doc/c-api/intro.rst:.. c:macro:: Py_ABS(x)
Include/pymacro.h:#define Py_ABS(x) ((x) < 0 ? -(x) : (x))
Modules/_testcapimodule.c:    // Py_MIN(), Py_MAX(), Py_ABS()
Modules/_testcapimodule.c:    assert(Py_ABS(-5) == 5);
Objects/floatobject.c:    py_exponent = PyLong_FromLong(Py_ABS(exponent));
Objects/longobject.c:    x = Py_ABS(x);
Objects/longobject.c:    y = Py_ABS(y);
Python/marshal.c:        uint64_t abs_value = Py_ABS(long_export.value);
Python/marshal.c:    Py_ssize_t size = 1 + (Py_ABS(n) - 1) / marshal_ratio;
Python/marshal.c:    int shorts_in_top_digit = 1 + (Py_ABS(n) - 1) % marshal_ratio;
Python/pystrhex.c:    unsigned int abs_bytes_per_sep = Py_ABS(bytes_per_sep_group);
Python/pytime.c:        PyTime_t abs_r = Py_ABS(r);
Python/pytime.c:        if (abs_r > k / 2 || (abs_r == k / 2 && (Py_ABS(x) & 1))) {

[serhiy-storchaka]

Technically, negation of INT_MIN is an undefined behavior.

[serhiy-storchaka]

To avoid an undefined behavior we should handle the special case separately using unsigned arithmetic.

[skirpichev]

Technically, negation of INT_MIN is an undefined behavior.

Yes, as an example (though, I think it's only corner case; ditto for other signed integer types).

To avoid an undefined behavior we should handle the special case separately using unsigned arithmetic.

Probably, this is possible, but it will be too costly just for one special case.