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.

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

Jump to bottom

Fix overflow check in PySequence_Tuple #71768

Closed
zhangyangyu opened this issue Jul 21, 2016 · 5 comments
Closed

Fix overflow check in PySequence_Tuple #71768

zhangyangyu opened this issue Jul 21, 2016 · 5 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@zhangyangyu
Copy link
Member

BPO 27581
Nosy @vadmium, @serhiy-storchaka, @zhangyangyu
Files
  • overflow_check_in_PySequence_Tuple.patch
    		•

    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 2016-07-26.02:12:13.822>
created_at = <Date 2016-07-21.05:36:48.146>
labels = ['interpreter-core', 'type-feature']
title = 'Fix overflow check in PySequence_Tuple'
updated_at = <Date 2016-07-26.02:12:13.821>
user = 'https://github.com/zhangyangyu'

    bugs.python.org fields:

    activity = <Date 2016-07-26.02:12:13.821>
actor = 'martin.panter'
assignee = 'none'
closed = True
closed_date = <Date 2016-07-26.02:12:13.822>
closer = 'martin.panter'
components = ['Interpreter Core']
creation = <Date 2016-07-21.05:36:48.146>
creator = 'xiang.zhang'
dependencies = []
files = ['43806']
hgrepos = []
issue_num = 27581
keywords = ['patch']
message_count = 5.0
messages = ['270909', '271062', '271076', '271133', '271222']
nosy_count = 4.0
nosy_names = ['python-dev', 'martin.panter', 'serhiy.storchaka', 'xiang.zhang']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue27581'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']
    @zhangyangyu
    Copy link
    Member Author

    Overflow check in PySequence_Tuple relies on undefined behaviour, fix it.

    @zhangyangyu zhangyangyu added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement labels Jul 21, 2016
    @vadmium
    Copy link
    Member

    vadmium commented Jul 23, 2016

    Hmm maybe this patch is okay. We are assuming size_t will fit more than PY_SSIZE_T_MAX.

    The alternatives I can think of would be equally ugly:

    /* Risks loss of precision, e.g. 64 bit integer from floating point */
    if (n < (Py_ssize_t)(PY_SSIZE_T_MAX / 1.25) - 10))

    /* PY_SSIZE_T_MAX * 4/5 - 10 without loss of precision or overflowing */
    if (n < PY_SSIZE_T_MAX / 5 * 4 + PY_SSIZE_T_MAX % 5 * 4 / 5 - 10)

    @zhangyangyu
    Copy link
    Member Author

    I'd prefer the size_t method. The others seems to make the logic not clear. I've seen some codes using size_t to do overflow checking, such as https://hg.python.org/cpython/file/tip/Python/bltinmodule.c#l1954. There are more if you use a simple grep. So I think the logic is okay.

    @vadmium
    Copy link
    Member

    vadmium commented Jul 24, 2016

    I don’t accept that the bltinmodule.c code is similar to your patch. It gets a size_t from calling strlen() on a string that potentially comes from outside Python, so it is definitely valid to check for PY_SSIZE_T_MAX.

    However I did find PyByteArray_Resize() (revision 1590c594550e), where this technique of calculating in size_t and then checking for overflow is used. And also in your favour is the definition in Include/pyport.h which currently guarantees size_t can store up to double PY_SSIZE_T_MAX:

    /* Largest positive value of type Py_ssize_t. */
#define PY_SSIZE_T_MAX ((Py_ssize_t)(((size_t)-1)>>1))

    So I am convinced there should be no real problem with your patch.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 25, 2016

    New changeset ad3762227655 by Martin Panter in branch '3.5':
    Issue bpo-27581: Don’t rely on overflow wrapping in PySequence_Tuple()
    https://hg.python.org/cpython/rev/ad3762227655

    New changeset 8f84942a0e40 by Martin Panter in branch 'default':
    Issue bpo-27581: Merge overflow fix from 3.5
    https://hg.python.org/cpython/rev/8f84942a0e40

    New changeset 55b6e51b878b by Martin Panter in branch '2.7':
    Issue bpo-27581: Don’t rely on overflow wrapping in PySequence_Tuple()
    https://hg.python.org/cpython/rev/55b6e51b878b

    @vadmium vadmium closed this as completed Jul 26, 2016
    @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
    Assignees
    No one assigned
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @vadmium @zhangyangyu