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

Test for integer overflow on Py_ssize_t: explicitly cast to size_t #66403

Closed
vstinner opened this issue Aug 15, 2014 · 16 comments
Closed

Test for integer overflow on Py_ssize_t: explicitly cast to size_t #66403

vstinner opened this issue Aug 15, 2014 · 16 comments
Assignees
@serhiy-storchaka
Labels
3.7 (EOL) end of life

Comments

@vstinner
Copy link
Member

BPO 22207
Nosy @gpshead, @vstinner, @serhiy-storchaka
PRs
  • bpo-22207: Add checks for possible integer overflows in unicodeobject.c. #2623
  • [3.6] bpo-22207: Add checks for possible integer overflows in unicodeobject.c. (GH-2623) #2658
  • [3.5] bpo-22207: Add checks for possible integer overflows in unicodeobject.c. (GH-2623) #2659
    		•
    Files
  • test_overflow_ssize_t.patch
  • overflow2.patch
  • unicode.patch
  • unicode_2.patch
  • issue22207_unicode_3.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 = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2017-07-11.04:44:29.605>
created_at = <Date 2014-08-15.22:03:19.694>
labels = ['3.7']
title = 'Test for integer overflow on Py_ssize_t: explicitly cast to size_t'
updated_at = <Date 2017-07-11.04:44:29.604>
user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2017-07-11.04:44:29.604>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2017-07-11.04:44:29.605>
closer = 'serhiy.storchaka'
components = []
creation = <Date 2014-08-15.22:03:19.694>
creator = 'vstinner'
dependencies = []
files = ['36381', '36382', '36402', '36451', '36757']
hgrepos = []
issue_num = 22207
keywords = ['patch']
message_count = 16.0
messages = ['225369', '225372', '225475', '225476', '225808', '225859', '227868', '227869', '227870', '227871', '227873', '297116', '297903', '298113', '298117', '298119']
nosy_count = 5.0
nosy_names = ['gregory.p.smith', 'vstinner', 'neologix', 'python-dev', 'serhiy.storchaka']
pr_nums = ['2623', '2658', '2659']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue22207'
versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']
    @vstinner
    Copy link
    Member Author

    Python contains a lot of tests like this one:

    if (length > PY_SSIZE_T_MAX / 4)
     return PyErr_NoMemory();

    where length type is Py_ssize_t.

    This test uses signed integers. There is usually a "assert(length > 0);" before.

    The issue bpo-22110 enabled more compiler warnings and there are now warnings when the test uses an unsigned number. Example:

    if (size > PY_SSIZE_T_MAX - PyBytesObject_SIZE) ...

    where PyBytesObject_SIZE is defined using offsetof() which returns a size_t.

    I propose to always cast Py_ssize_t length to size_t to avoid undefined behaviour (I never know if the compiler chooses signed or unsigned at the end) to ensure that the test also fail for negative number. For example, the following test must fail for negative size:

    if ((size_t)size > (size_t)PY_SSIZE_T_MAX - PyBytesObject_SIZE) ...

    Attached patch changes bytesobject.c, tupleobject.c and unicodeobject.c (and asdl.c). If the global approach is accepted, more files should be patched.

    @vstinner
    Copy link
    Member Author

    overflow2.patch: more changes for this issue (it doesn't replace my previous patch).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 17, 2014

    New changeset 97c70528b604 by Victor Stinner in branch 'default':
    Issue bpo-22207: Fix "comparison between signed and unsigned integers" warning in
    http://hg.python.org/cpython/rev/97c70528b604

    @vstinner
    Copy link
    Member Author

    I commited the first part.

    The remaining part is a long patch for unicodeobject.c.

    @serhiy-storchaka
    Copy link
    Member

    I think there are too many changes. Here is simpler patch which get rid of all warnings in Objects/unicodeobject.c on my computer (gcc 4.6.3, 32-bit Linux).

    @vstinner
    Copy link
    Member Author

    I think there are too many changes. Here is simpler patch which get rid of all warnings in Objects/unicodeobject.c on my computer (gcc 4.6.3, 32-bit Linux).

    The purpose of my patch is not to make the compiler quiet, but to ensure that negative lengths are handled correctly: Python must raise an error if a length is negative.

    For example, _PyUnicode_DecodeUnicodeInternal() doesn't check currently if the size is negative. I don't know exactly what happens if you pass a negative size.

    @serhiy-storchaka
    Copy link
    Member

    Gregory just has committed an equivalent of unicode_2.patch in a404bf4db6a6.

    @gpshead
    Copy link
    Member

    gpshead commented Sep 30, 2014

    modify it as you see fit, i hadn't noticed this issue; just the warnings.

    @serhiy-storchaka
    Copy link
    Member

    Yes, these warning annoyed me too. But Victor's patch is purposed to check
    input data of public C API functions.

    @serhiy-storchaka
    Copy link
    Member

    Here is original Victor's patch synchronized with the tip.

    @serhiy-storchaka
    Copy link
    Member

    I have reviewed the patch and found only few checks which looks good to me. Also I found two possible overflows (not fixed by the patch). All other changes looks redundant to me and just churn a code.

    @vstinner
    Copy link
    Member Author

    Ok, let's close the issue in that case ;-)

    @serhiy-storchaka
    Copy link
    Member

    I extracted the cases that really can happen in PR 2623.

    @serhiy-storchaka serhiy-storchaka added the 3.7 (EOL) end of life label Jul 7, 2017
    @serhiy-storchaka serhiy-storchaka self-assigned this Jul 7, 2017
    @serhiy-storchaka
    Copy link
    Member

    New changeset 64e461b by Serhiy Storchaka in branch 'master':
    bpo-22207: Add checks for possible integer overflows in unicodeobject.c. (bpo-2623)
    64e461b

    @serhiy-storchaka
    Copy link
    Member

    New changeset 82a9075 by Serhiy Storchaka in branch '3.6':
    [3.6] bpo-22207: Add checks for possible integer overflows in unicodeobject.c. (GH-2623) (bpo-2658)
    82a9075

    @serhiy-storchaka
    Copy link
    Member

    New changeset 44eb51e by Serhiy Storchaka in branch '3.5':
    [3.5] bpo-22207: Add checks for possible integer overflows in unicodeobject.c. (GH-2623) (bpo-2659)
    44eb51e

    @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

    @serhiy-storchaka serhiy-storchaka

    Labels
    3.7 (EOL) end of life
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @gpshead @vstinner @serhiy-storchaka