Skip to content
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.

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

undefined behaviour: signed integer overflow in threadmodule.c #77813

Closed
pitrou opened this issue May 24, 2018 · 9 comments
Closed

undefined behaviour: signed integer overflow in threadmodule.c #77813

pitrou opened this issue May 24, 2018 · 9 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@pitrou
Copy link
Member

pitrou commented May 24, 2018

BPO 33632
Nosy @pitrou, @vstinner, @vadmium, @pganssle, @ZackerySpytz, @hongweipeng
PRs
  • bpo-33632: Avoid signed integer overflow in the _thread module #12729
  • Superseder
  • bpo-41710: threading.Lock.acquire(timeout) should use sem_clockwait(CLOCK_MONOTONIC)
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2021-12-06.13:26:08.534>
    created_at = <Date 2018-05-24.08:39:55.148>
    labels = ['extension-modules', '3.8', 'type-bug', '3.7']
    title = 'undefined behaviour: signed integer overflow in threadmodule.c'
    updated_at = <Date 2021-12-06.13:26:08.533>
    user = 'https://github.com/pitrou'

    bugs.python.org fields:

    activity = <Date 2021-12-06.13:26:08.533>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-12-06.13:26:08.534>
    closer = 'vstinner'
    components = ['Extension Modules']
    creation = <Date 2018-05-24.08:39:55.148>
    creator = 'pitrou'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 33632
    keywords = ['patch']
    message_count = 9.0
    messages = ['317545', '317554', '317556', '339644', '340184', '340255', '340284', '407712', '407800']
    nosy_count = 6.0
    nosy_names = ['pitrou', 'vstinner', 'martin.panter', 'p-ganssle', 'ZackerySpytz', 'hongweipeng']
    pr_nums = ['12729']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = '41710'
    type = 'behavior'
    url = 'https://bugs.python.org/issue33632'
    versions = ['Python 3.7', 'Python 3.8']

    @pitrou
    Copy link
    Member Author

    pitrou commented May 24, 2018

    Modules/_threadmodule.c:52:47: runtime error: signed integer overflow: 2387971499048 + 9223372036000000000 cannot be represented in type 'long'

    @pitrou pitrou added stdlib Python modules in the Lib dir 3.7 (EOL) end of life 3.8 only security fixes type-bug An unexpected behavior, bug, or error labels May 24, 2018
    @vadmium
    Copy link
    Member

    vadmium commented May 24, 2018

    Looks like this is what my thread.patch was fixing in <https://bugs.python.org/issue1621#msg271057\>. You’re welcome to use my patch, but I won’t have time to work on it myself.

    @vstinner
    Copy link
    Member

    Modules/_threadmodule.c:52:47: runtime error: signed integer overflow: 2387971499048 + 9223372036000000000 cannot be represented in type 'long'

    How do you reproduce the issue? The thread module should limit the maximum timeout to PY_TIMEOUT_MAX. Maybe PY_TIMEOUT_MAX is too big?

    @ZackerySpytz
    Copy link
    Mannequin

    ZackerySpytz mannequin commented Apr 8, 2019

    I've created a PR based on Martin Panter's patch.

    @ZackerySpytz ZackerySpytz mannequin added extension-modules C modules in the Modules dir and removed stdlib Python modules in the Lib dir labels Apr 8, 2019
    @vadmium
    Copy link
    Member

    vadmium commented Apr 14, 2019

    Victor, if you run the test suite, one of the test cases should trigger the overflow. I used to compile with Undefined Behaviour Sanitizer to print messages when these errors occur; see <https://bugs.python.org/issue1621#msg271118\> for my setup at the time. I presume Antoine did something similar.

    I do not remember, but suspect the test case might be the following lines of “BaseLockTests.test_timeout” in Lib/test/lock_tests.py, testing a fraction of a second less than PY_TIMEOUT_MAX:

    # TIMEOUT_MAX is ok
    lock.acquire(timeout=TIMEOUT_MAX)

    Perhaps reducing PY_TIMEOUT_MAX by a few centuries would be one way to avoid the problem. In my patch I avoided the problem by rearranging the arithmetic, so that the timeout value is only compared and reduced, never added.

    @vstinner
    Copy link
    Member

    In short, a+b can overflow, but a-b cannot?

    @pganssle
    Copy link
    Member

    In short, a+b can overflow, but a-b cannot?

    I think it's more that by always checking the elapsed time against now() - starttime, you never need to represent the time at which the timeout should happen - which may be so far in the future that it causes a signed overflow.

    @hongweipeng
    Copy link
    Mannequin

    hongweipeng mannequin commented Dec 5, 2021

    I think PR #28674 has resolved this issue.

    @vstinner
    Copy link
    Member

    vstinner commented Dec 6, 2021

    I think PR #28674 has resolved this issue.

    You're right.

    _threadmodule.c now uses _PyDeadline_Init() which calls _PyTime_Add(), and _PyTime_Add() prevents integer overflows; Extract of its implementation:

    // Compute t1 + t2. Clamp to [_PyTime_MIN; _PyTime_MAX] on overflow.
    static inline int
    pytime_add(_PyTime_t *t1, _PyTime_t t2)
    {
        if (t2 > 0 && *t1 > _PyTime_MAX - t2) {
            *t1 = _PyTime_MAX;
            return -1;
        }
        else if (t2 < 0 && *t1 < _PyTime_MIN - t2) {
            *t1 = _PyTime_MIN;
            return -1;
        }
        else {
            *t1 += t2;
            return 0;
        }
    }

    @vstinner vstinner closed this as completed Dec 6, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants