gh-130824: Add tests for `NULL` in `PyLong_*AndOverflow` functions #130828

ZeroIntensity · 2025-03-04T11:30:36Z

It crashed before because the assertion didn't realize that overflow would be 0 and not -1 in SystemError cases.

Issue: PyLong_AsLongAndOverflow does not guarantee overflow is -1 when value is -1 #130824

serhiy-storchaka

LGTM. 👍

skirpichev

LGTM

Few nitpicks, feel free to ignore.

Lib/test/test_capi/test_long.py

Modules/_testlimitedcapi/long.c

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

serhiy-storchaka · 2025-03-04T15:44:15Z

Lib/test/test_capi/test_long.py

@@ -211,9 +211,9 @@ def check_long_asintandoverflow(self, func, min_val, max_val):

        self.assertEqual(func(min_val - 1), (-1, -1))
        self.assertEqual(func(max_val + 1), (-1, +1))
+        self.assertRaises(SystemError, func, None)

        # CRASHES func(1.0)


Does it really crash?

Good catch, it doesn't: "If any other exception occurs set *overflow to 0 and return -1 as usual." TypeError will be here.

serhiy-storchaka · 2025-03-04T15:48:26Z

Modules/_testlimitedcapi/long.c

@@ -625,7 +625,8 @@ pylong_aslongandoverflow(PyObject *module, PyObject *arg)
    int overflow = UNINITIALIZED_INT;
    long value = PyLong_AsLongAndOverflow(arg, &overflow);
    if (value == -1 && PyErr_Occurred()) {
-        assert(overflow == -1);
+        // overflow can be 0 if a seperate exception occurred
+        assert(overflow == -1 || overflow == 0);


Is not it always 0?

Not with OverflowError.

Yeah, OverflowError is -1 and then other exceptions (only SystemError?) are 0.

only SystemError?

No, TypeError too.

OverflowError is not raised. In case of an integer overflow, *overflow should be 1 or -1.

Isn't it always 0 here?

skirpichev

one grammar nitpick

Modules/_testlimitedcapi/long.c

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

miss-islington-app · 2025-03-05T08:59:06Z

Thanks @ZeroIntensity for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington-app · 2025-03-05T08:59:06Z

Thanks @ZeroIntensity for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington-app · 2025-03-05T08:59:09Z

Sorry, @ZeroIntensity and @encukou, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 90130807d9c5a55b2a64024f5dfbee4785b9a27c 3.12

…ons (pythonGH-130828) (cherry picked from commit 9013080) Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

bedevere-app · 2025-03-05T08:59:18Z

GH-130869 is a backport of this pull request to the 3.13 branch.

ZeroIntensity · 2025-03-05T10:37:12Z

@encukou Should I make the manual backport to 3.12? I'm not sure it's worth backporting to prevent conflicts if there's already a conflict.

serhiy-storchaka · 2025-03-05T10:41:49Z

It will help to backport future tests. It will help to ensure that no regressions be introduced in old versions.

See also #130871. You can include that change in the backports.

… functions (pythonGH-130828) (cherry picked from commit 9013080) Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

bedevere-app · 2025-03-05T10:55:19Z

GH-130876 is a backport of this pull request to the 3.12 branch.

encukou · 2025-03-05T10:55:49Z

Should I make the manual backport to 3.12?

Sorry, I did it before I saw your message

… functions (pythonGH-130828) (cherry picked from commit 9013080) Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

…ions (GH-130828) (GH-130869) (cherry picked from commit 9013080) Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

…GH-130828) (GH-130876) (cherry picked from commit 9013080) Co-authored-by: Peter Bierma <zintensitydev@gmail.com> Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

pythongh-130824: Add tests for NULL in PyLong_*AndOverflow functions

b15c219

ZeroIntensity added tests skip news labels Mar 4, 2025

ZeroIntensity requested a review from serhiy-storchaka March 4, 2025 11:30

bedevere-app bot added the awaiting review label Mar 4, 2025

bedevere-app bot mentioned this pull request Mar 4, 2025

PyLong_AsLongAndOverflow does not guarantee overflow is -1 when value is -1 #130824

Closed

serhiy-storchaka approved these changes Mar 4, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting review labels Mar 4, 2025

skirpichev approved these changes Mar 4, 2025

View reviewed changes

Lib/test/test_capi/test_long.py Outdated Show resolved Hide resolved

Modules/_testlimitedcapi/long.c Show resolved Hide resolved

Apply suggestions from code review

0fc0c21

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

serhiy-storchaka reviewed Mar 4, 2025

View reviewed changes

serhiy-storchaka self-requested a review March 4, 2025 15:50

skirpichev self-requested a review March 5, 2025 00:26

Add check for TypeError

46cd27f

skirpichev approved these changes Mar 5, 2025

View reviewed changes

Modules/_testlimitedcapi/long.c Outdated Show resolved Hide resolved

Modules/_testlimitedcapi/long.c Outdated Show resolved Hide resolved

Apply suggestions from code review

8bb2502

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>

encukou merged commit 9013080 into python:main Mar 5, 2025
41 checks passed

bedevere-app bot removed the awaiting merge label Mar 5, 2025

encukou added needs backport to 3.12 needs backport to 3.13 labels Mar 5, 2025

miss-islington-app bot assigned encukou Mar 5, 2025

bedevere-app bot removed the needs backport to 3.13 label Mar 5, 2025

ZeroIntensity deleted the add-pylong-null-checks branch March 5, 2025 10:24

bedevere-app bot removed the needs backport to 3.12 label Mar 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-130824: Add tests for `NULL` in `PyLong_*AndOverflow` functions #130828

gh-130824: Add tests for `NULL` in `PyLong_*AndOverflow` functions #130828

ZeroIntensity commented Mar 4, 2025 •

edited by bedevere-app bot

Loading

serhiy-storchaka left a comment

skirpichev left a comment

serhiy-storchaka Mar 4, 2025

skirpichev Mar 5, 2025

serhiy-storchaka Mar 4, 2025

skirpichev Mar 5, 2025

ZeroIntensity Mar 5, 2025

skirpichev Mar 5, 2025

serhiy-storchaka Mar 5, 2025

skirpichev left a comment

miss-islington-app bot commented Mar 5, 2025

miss-islington-app bot commented Mar 5, 2025

miss-islington-app bot commented Mar 5, 2025

bedevere-app bot commented Mar 5, 2025

ZeroIntensity commented Mar 5, 2025

serhiy-storchaka commented Mar 5, 2025

bedevere-app bot commented Mar 5, 2025

encukou commented Mar 5, 2025

gh-130824: Add tests for NULL in PyLong_*AndOverflow functions #130828

gh-130824: Add tests for NULL in PyLong_*AndOverflow functions #130828

Conversation

ZeroIntensity commented Mar 4, 2025 • edited by bedevere-app bot Loading

serhiy-storchaka left a comment

Choose a reason for hiding this comment

skirpichev left a comment

Choose a reason for hiding this comment

serhiy-storchaka Mar 4, 2025

Choose a reason for hiding this comment

skirpichev Mar 5, 2025

Choose a reason for hiding this comment

serhiy-storchaka Mar 4, 2025

Choose a reason for hiding this comment

skirpichev Mar 5, 2025

Choose a reason for hiding this comment

ZeroIntensity Mar 5, 2025

Choose a reason for hiding this comment

skirpichev Mar 5, 2025

Choose a reason for hiding this comment

serhiy-storchaka Mar 5, 2025

Choose a reason for hiding this comment

skirpichev left a comment

Choose a reason for hiding this comment

miss-islington-app bot commented Mar 5, 2025

miss-islington-app bot commented Mar 5, 2025

miss-islington-app bot commented Mar 5, 2025

bedevere-app bot commented Mar 5, 2025

ZeroIntensity commented Mar 5, 2025

serhiy-storchaka commented Mar 5, 2025

bedevere-app bot commented Mar 5, 2025

encukou commented Mar 5, 2025

gh-130824: Add tests for `NULL` in `PyLong_*AndOverflow` functions #130828

gh-130824: Add tests for `NULL` in `PyLong_*AndOverflow` functions #130828

ZeroIntensity commented Mar 4, 2025 •

edited by bedevere-app bot

Loading