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

Possible integer overflow of PyLong_AsLong() results #60193

Closed
serhiy-storchaka opened this issue Sep 20, 2012 · 31 comments
Closed

Possible integer overflow of PyLong_AsLong() results #60193

serhiy-storchaka opened this issue Sep 20, 2012 · 31 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@serhiy-storchaka
Copy link
Member

BPO 15989
Nosy @jcea, @mdickinson, @vstinner, @ericvsmith, @rbtcollins, @bitdancer, @asvetlov, @serhiy-storchaka
Files
  • long_aslong_overflow.patch
  • long_aslong_overflow-3.3_2.patch
  • long_aslong_overflow-3.2.patch
  • long_aslong_overflow-2.7.patch
  • long_aslong_overflow_tests.patch
  • long_aslong_overflow-2.7_3.patch
  • long_aslong_overflow-3.2_3.patch
  • long_aslong_overflow-3.3_3.patch
  • long_aslong_overflow-3.4_3.patch
  • long_aslong_overflow_2-3.4.patch: Uncommitted remnants without tests
  • 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 2015-09-06.18:28:50.834>
    created_at = <Date 2012-09-20.19:01:45.390>
    labels = ['interpreter-core', 'type-bug']
    title = 'Possible integer overflow of PyLong_AsLong() results'
    updated_at = <Date 2015-09-06.20:31:06.150>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2015-09-06.20:31:06.150>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2015-09-06.18:28:50.834>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2012-09-20.19:01:45.390>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = ['27246', '27268', '27401', '27402', '27632', '28658', '28659', '28660', '28661', '32893']
    hgrepos = []
    issue_num = 15989
    keywords = ['patch']
    message_count = 31.0
    messages = ['170832', '170847', '170850', '170903', '170904', '171042', '171046', '171078', '171886', '173383', '179492', '179991', '180005', '180239', '180240', '180245', '180250', '180252', '180264', '180916', '204746', '229621', '229696', '247373', '247374', '248339', '250002', '250003', '250009', '250017', '250018']
    nosy_count = 10.0
    nosy_names = ['jcea', 'mark.dickinson', 'vstinner', 'eric.smith', 'rbcollins', 'Arfrever', 'r.david.murray', 'asvetlov', 'python-dev', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue15989'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    @serhiy-storchaka
    Copy link
    Member Author

    There are several places where the result of PyLong_AsLong() assigned to variable of type int. It can cause unknown issues on platforms with sizeof(int) != sizeof(long). All 140 cases of PyLong_AsLong() usage should be checked.

    @serhiy-storchaka serhiy-storchaka self-assigned this Sep 20, 2012
    @serhiy-storchaka serhiy-storchaka added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Sep 20, 2012
    @mdickinson
    Copy link
    Member

    Getting a C int out of a Python int is currently a bit awkward. What do you think about adding a PyLong_AsInt counterpart to PyLong_AsLong? (The alternative is to use PyLong_AsLong and repeat the same overflow detection code in many places.)

    @mdickinson
    Copy link
    Member

    In the Objects subdirectory (which is all I've looked at so far), I see issues in:

    • fileobject.c (PyObject_AsFileDescriptor)

    • structseq.c (PyLong_AsLong return value used as a Py_ssize_t; probably safe, but it would be better to use PyLong_AsSsize_t).

    • unicodeobject.c (one place where result assigned to something of type ssize_t, one where result assigned to something of type int).

    @serhiy-storchaka
    Copy link
    Member Author

    Here is a patch (for 3.3). I added private _PyLong_AsInt and where possible I
    have tried to use the appropriate function.

    @serhiy-storchaka
    Copy link
    Member Author

    I was assigned this to itself to show that I working on the issue. Now I free up place for someone with committing privileges.

    @serhiy-storchaka serhiy-storchaka removed their assignment Sep 21, 2012
    @mdickinson
    Copy link
    Member

    Looks good, in general.

    _PyLong_AsInt should return -1 on overflow in all cases; at the moment, it looks like it doesn't do that for values that overflow an int but not a long.

    @mdickinson
    Copy link
    Member

    I added some comments on Rietveld.

    Apart from the bug in _PyLong_AsInt mentioned above, the patch mostly looks good. But there are many changes, some of which will have user-visible effects, and I think it's somewhat risky to make all these changes in bugfix releases (which now includes Python 3.3).

    Perhaps there could be a smaller patch aimed at the bugfix releases? Ideally, each fix there should be backed by a regression test.

    @serhiy-storchaka
    Copy link
    Member Author

    Thank you for review and for found bugs. I again checked the patch, corrected
    the errors and dubious places. Also I corrected the error in
    Modules/grpmodule.c (in other places gid_t parsed as signed long) and the
    possible behaviour change in Modules/_io/_iomodule.c, reversed the changes in
    Modules/pyexpat.c. If some changes cause you have doubts, you can cherry-pick
    only the most obvious fixes.

    Ideally, each fix there should be backed by a regression test.

    Unfortunately I have only platforms where sizeof(int) == sizeof(long) ==
    sizeof(size_t).

    @serhiy-storchaka
    Copy link
    Member Author

    Added patches for 2.7 and 3.2.

    @serhiy-storchaka
    Copy link
    Member Author

    Here is a tests for most of fixed overflows. Some errors are difficult or impossible to reproduce.

    @serhiy-storchaka serhiy-storchaka self-assigned this Dec 29, 2012
    @serhiy-storchaka
    Copy link
    Member Author

    Here are minimized patches. All included changes have tests (i.e. overflow bugs
    are observable). I drop grp module changes even with tests, because there are
    other issues for this (bpo-2005, bpo-4591). New tests for fcntl added (they
    test PyObject_AsFileDescriptor).

    If patches look good in general, I going first commit to 3.4, and if this will
    not break any buildbot, then I'll commit the rest patches. After that I'll
    continue with not included changes (if I can write tests for them).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 14, 2013

    New changeset 13e2e44db99d by Serhiy Storchaka in branch 'default':
    Issue bpo-15989: Fix several occurrences of integer overflow
    http://hg.python.org/cpython/rev/13e2e44db99d

    @serhiy-storchaka
    Copy link
    Member Author

    Changeset 525407d89277: Fix test_socket broken in previous commit (changeset 13e2e44db99d).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 19, 2013

    New changeset 974ace29ee2d by Serhiy Storchaka in branch '3.2':
    Issue bpo-15989: Fix several occurrences of integer overflow
    http://hg.python.org/cpython/rev/974ace29ee2d

    New changeset 8f10c9eae183 by Serhiy Storchaka in branch '3.3':
    Issue bpo-15989: Fix several occurrences of integer overflow
    http://hg.python.org/cpython/rev/8f10c9eae183

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 19, 2013

    New changeset d544873d62e9 by Serhiy Storchaka in branch '2.7':
    Issue bpo-15989: Fix several occurrences of integer overflow
    http://hg.python.org/cpython/rev/d544873d62e9

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Jan 19, 2013

    Several 2.7 buildbots are failing.

    Unfortunately I have only platforms where sizeof(int) == sizeof(long) ==
    sizeof(size_t).

    You can use your cpython ssh key to login to all snakebite buildbot
    machines. They are tagged with [SB].

    http://mail.python.org/pipermail/python-dev/2012-September/121651.html

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 19, 2013

    New changeset a78ebf9aed06 by Serhiy Storchaka in branch '2.7':
    Ensure that width and precision in string formatting test have type int, not long.
    http://hg.python.org/cpython/rev/a78ebf9aed06

    @serhiy-storchaka
    Copy link
    Member Author

    Thank you for point, Stefan. And thanks Trent for his project.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 19, 2013

    New changeset ee93a89b4e0f by Serhiy Storchaka in branch '2.7':
    Issue bpo-15989: Fix possible integer overflow in str formatting as in unicode formatting.
    http://hg.python.org/cpython/rev/ee93a89b4e0f

    @serhiy-storchaka
    Copy link
    Member Author

    Sqlite module part extracted to bpo-17073.

    @serhiy-storchaka
    Copy link
    Member Author

    Here are remnants of initial patch which were not committed. There are no tests.

    @bitdancer
    Copy link
    Member

    Are the fixes in the final patch waiting for someone to write tests, or is the intent to commit them without tests after a final review because tests are too difficult to write?

    @serhiy-storchaka
    Copy link
    Member Author

    It would be good to write tests, but for some tests it is difficult (if possible). At least I did not see ways how to do this.

    @rbtcollins
    Copy link
    Member

    @serhiy - I'm a little confused about the state of this patch. It seems like you need more review?

    @serhiy-storchaka
    Copy link
    Member Author

    Yes, it would be good if other's pair of eyes will look on the patch.

    @rbtcollins
    Copy link
    Member

    It looks fine to me, for whatever thats worth. I think you should commit it.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 6, 2015

    New changeset d51a82f68a70 by Serhiy Storchaka in branch 'default':
    Issue bpo-15989: Fixed some scarcely probable integer overflows.
    https://hg.python.org/cpython/rev/d51a82f68a70

    @serhiy-storchaka
    Copy link
    Member Author

    One of changes to Modules/_io/_iomodule.c is no longer actual since bpo-21679.

    Change to Modules/selectmodule.c is no longer actual since bpo-23485 (f54bc2c52dfd).

    The rest changes were committed to 3.6 only.

    @ericvsmith
    Copy link
    Member

    Isn't Python-ast.c a generated file?

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 6, 2015

    New changeset e53df7955192 by Serhiy Storchaka in branch 'default':
    Make asdl_c.py to generate Python-ast.c changed in issue bpo-15989.
    https://hg.python.org/cpython/rev/e53df7955192

    @serhiy-storchaka
    Copy link
    Member Author

    Isn't Python-ast.c a generated file?

    Good catch Eric.

    @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
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants