Skip to content

gh-141370: Fix undefined behavior when using Py_ABS() #141548

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

gh-141370: Fix undefined behavior when using Py_ABS() #141548

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1acabab
gh-141370: Fix undefined behavior when using Py_ABS()
serhiy-storchaka Nov 14, 2025
445bcc8
Merge branch 'main' into Py_ABC-UB
skirpichev Nov 15, 2025
887853d
Merge branch 'main' into Py_ABC-UB
serhiy-storchaka Nov 17, 2025
df24c4f
Use more efficient implementation.
serhiy-storchaka Nov 17, 2025
a584478
Merge remote-tracking branch 'refs/remotes/origin/Py_ABC-UB' into Py_…
serhiy-storchaka Nov 17, 2025
b292561
Try other code. Add a comment.
serhiy-storchaka Nov 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions Lib/test/test_bytes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,17 @@ def test_hex_separator_basics(self):
self.assertEqual(three_bytes.hex(':', 2), 'b9:01ef')
self.assertEqual(three_bytes.hex(':', 1), 'b9:01:ef')
self.assertEqual(three_bytes.hex('*', -2), 'b901*ef')
self.assertEqual(three_bytes.hex(sep=':', bytes_per_sep=2), 'b9:01ef')
self.assertEqual(three_bytes.hex(sep='*', bytes_per_sep=-2), 'b901*ef')
for bytes_per_sep in 3, -3, 2**31-1, -(2**31-1):
with self.subTest(bytes_per_sep=bytes_per_sep):
self.assertEqual(three_bytes.hex(':', bytes_per_sep), 'b901ef')
for bytes_per_sep in 2**31, -2**31, 2**1000, -2**1000:
with self.subTest(bytes_per_sep=bytes_per_sep):
try:
self.assertEqual(three_bytes.hex(':', bytes_per_sep), 'b901ef')
except OverflowError:
pass

value = b'{s\005\000\000\000worldi\002\000\000\000s\005\000\000\000helloi\001\000\000\0000'
self.assertEqual(value.hex('.', 8), '7b7305000000776f.726c646902000000.730500000068656c.6c6f690100000030')
Expand Down
5 changes: 5 additions & 0 deletions Lib/test/test_marshal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ def test_ints(self):
for expected in (-n, n):
self.helper(expected)
n = n >> 1
n = 1 << 100
while n:
for expected in (-n, -n+1, n-1, n):
self.helper(expected)
n = n >> 1

def test_int64(self):
# Simulate int marshaling with TYPE_INT64.
Expand Down
19 changes: 19 additions & 0 deletions Lib/test/test_memoryview.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,25 @@ def test_memoryview_hex(self):
m2 = m1[::-1]
self.assertEqual(m2.hex(), '30' * 200000)

def test_memoryview_hex_separator(self):
x = bytes(range(97, 102))
m1 = memoryview(x)
m2 = m1[::-1]
self.assertEqual(m2.hex(':'), '65:64:63:62:61')
self.assertEqual(m2.hex(':', 2), '65:6463:6261')
self.assertEqual(m2.hex(':', -2), '6564:6362:61')
self.assertEqual(m2.hex(sep=':', bytes_per_sep=2), '65:6463:6261')
self.assertEqual(m2.hex(sep=':', bytes_per_sep=-2), '6564:6362:61')
for bytes_per_sep in 5, -5, 2**31-1, -(2**31-1):
with self.subTest(bytes_per_sep=bytes_per_sep):
self.assertEqual(m2.hex(':', bytes_per_sep), '6564636261')
for bytes_per_sep in 2**31, -2**31, 2**1000, -2**1000:
with self.subTest(bytes_per_sep=bytes_per_sep):
try:
self.assertEqual(m2.hex(':', bytes_per_sep), '6564636261')
except OverflowError:
pass

def test_copy(self):
m = memoryview(b'abc')
with self.assertRaises(TypeError):
Expand Down
4 changes: 3 additions & 1 deletion Python/marshal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,9 @@ w_PyLong(const PyLongObject *ob, char flag, WFILE *p)
}
if (!long_export.digits) {
int8_t sign = long_export.value < 0 ? -1 : 1;
uint64_t abs_value = Py_ABS(long_export.value);
uint64_t abs_value = long_export.value < -INT64_MAX
? (uint64_t)INT64_MAX + (uint64_t)-(long_export.value + INT64_MAX)
: (uint64_t)Py_ABS(long_export.value);
Copy link
Contributor

@skirpichev skirpichev Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we assume two's complement representation of integers and use simpler workaround? As this fix span several places, I would prefer if it could be refactored to a separate macro.

I.e. something like:

#define My_ABS(x, MAX) \
    ((x) < 0 ? ((x) >= -MAX ? -(x) : (U##MAX >> 1) + 1) : (x))

Nowadays two's complement is only case permitted by the C23 and is a de-facto standard. Do you have some system in mind on which we should care? I'm pretty sure all Tier 1-3 platforms fit to this picture.

Copy link
Member Author

@serhiy-storchaka serhiy-storchaka Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All Tier 1-3 platforms perhaps fine with the current code. We change the code because we cannot be sure that it work on all exotic platforms and that future versions of the C compiler will not interpret an undefined behavior in interesting way.

The current gcc does not generate additions and substractions for this PR. It generates something more smart, although not so smart as for Py_ABS(). Although for your proposition it generates more complex code.

Details 

#include <limits.h>

#define Py_ABS(x) ((x) < 0 ? -(x) : (x))
#define My_ABS(x, MAX) \
    ((x) < 0 ? ((x) >= -MAX ? -(x) : (U##MAX >> 1) + 1) : (x))

unsigned int intabs0(int x) {
    return (unsigned int)Py_ABS(x);
}

unsigned int intabs(int x) {
    return x < -INT_MAX
        ? (unsigned int)INT_MAX + (unsigned int)-(x + INT_MAX)
        : (unsigned int)Py_ABS(x);
}

unsigned int intabs2(int x) {
    return My_ABS(x, INT_MAX);
}

unsigned long longabs0(long x) {
    return (unsigned long)Py_ABS(x);
}

unsigned long longabs(long x) {
    return x < -LONG_MAX
        ? (unsigned long)LONG_MAX + (unsigned long)-(x + LONG_MAX)
        : (unsigned long)Py_ABS(x);
}

unsigned long longabs2(long x) {
    return My_ABS(x, LONG_MAX);
}

Anyway, the performance of this code is not critical (if there is any difference).

Copy link
Contributor

@skirpichev skirpichev Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All Tier 1-3 platforms perhaps fine with the current code.

(Yes, and I suspect tests might be redundant in fact.)

Although for your proposition it generates more complex code.

A different version:

#define __GMP_CAST(type, expr) ((type) (expr))
#define NEG_CAST(T,x) (- (__GMP_CAST (T, (x) + 1) - 1))
#define ABS_CAST(T,x) ((x) >= 0 ? __GMP_CAST (T, x) : NEG_CAST (T, x))

I found same approach in the GNU GMP, so just copied NIH code here.

Anyway, the performance of this code is not critical (if there is any difference).

Your solution looks ok for me. But in any case we should factor it to some macro.

Copy link
Member Author

@serhiy-storchaka serhiy-storchaka Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this version gcc generates exactly the same code as for the current code. But it is now free from undefined behavior.

uint64_t d = abs_value;
long l = 0;

Expand Down
5 changes: 3 additions & 2 deletions Python/pystrhex.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,9 @@ static PyObject *_Py_strhex_impl(const char* argbuf, const Py_ssize_t arglen,
else {
bytes_per_sep_group = 0;
}

unsigned int abs_bytes_per_sep = Py_ABS(bytes_per_sep_group);
unsigned int abs_bytes_per_sep = (bytes_per_sep_group < -INT_MAX)
? (unsigned int)INT_MAX + (unsigned int)-(bytes_per_sep_group + INT_MAX)
: (unsigned int)Py_ABS(bytes_per_sep_group);
Py_ssize_t resultlen = 0;
if (bytes_per_sep_group && arglen > 0) {
/* How many sep characters we'll be inserting. */
Expand Down
Loading