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

resource.prlimit(int, int, str) crashs #64390

Closed
vstinner opened this issue Jan 8, 2014 · 9 comments
Closed

resource.prlimit(int, int, str) crashs #64390

vstinner opened this issue Jan 8, 2014 · 9 comments
Assignees
Labels
3.7 (EOL) end of life extension-modules C modules in the Modules dir type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@vstinner
Copy link
Member

vstinner commented Jan 8, 2014

BPO 20191
Nosy @vstinner, @larryhastings, @vadmium, @serhiy-storchaka
PRs
  • [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
  • Files
  • prlimit_refcount.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 2016-12-19.06:09:45.414>
    created_at = <Date 2014-01-08.14:33:42.874>
    labels = ['extension-modules', '3.7', 'type-crash']
    title = 'resource.prlimit(int, int, str) crashs'
    updated_at = <Date 2017-03-31.16:36:17.305>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:17.305>
    actor = 'dstufft'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2016-12-19.06:09:45.414>
    closer = 'serhiy.storchaka'
    components = ['Extension Modules']
    creation = <Date 2014-01-08.14:33:42.874>
    creator = 'vstinner'
    dependencies = []
    files = ['45848']
    hgrepos = []
    issue_num = 20191
    keywords = ['patch']
    message_count = 9.0
    messages = ['207686', '207692', '208272', '282897', '282898', '282909', '283584', '283586', '283588']
    nosy_count = 5.0
    nosy_names = ['vstinner', 'larry', 'python-dev', 'martin.panter', 'serhiy.storchaka']
    pr_nums = ['552']
    priority = 'critical'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue20191'
    versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 8, 2014

    $ ./python -c 'import resource; resource.prlimit(-3, 11, "\udbff\udfff")'
    Erreur de segmentation (core dumped)

    The problem is a generic problem with PyArg_Parse functions and "(O)" format. With this format, the caller does not hold a reference to the object nor the tuple. If arbitrary Python code is executed before the object is used, the object pointer becomes a dangling pointer.

    resource.prlimit() uses:

        if (!PyArg_ParseTuple(args, _Py_PARSE_PID "i|(OO):prlimit",
                              &pid, &resource, &curobj, &maxobj))
            return NULL;

    In this issue, it's worse: the string is casted to a sequence, and each string character becomes a temporary substring of 1 character. The problem is that PyArg_ParseTuple() nor resource_prlimit() hold the reference, and so the curobj and maxobj are dangling pointer.

    Options:

    • raise an error if the second parameter is not a tuple: implement the check in prlimit() or i PyArg_ParseTuple()?
    • hold a reference to the sequence, to curobj and to maxobj instead of using borrowed references

    @vstinner vstinner added the type-crash A hard crash of the interpreter, possibly with a core dump label Jan 8, 2014
    @serhiy-storchaka
    Copy link
    Member

    Thank you for good example, Victor. See bpo-6083 for early discussion.

    As for options:

    • I afraid we can't raise an error if the second parameter is not a tuple right now. Rather we should first emit deprecation warning, and raise an error only several releases later.

    • We can't turn borrowed references into non-borrowed references, because it will cause reference leaks in existing code.

    So what we should to do:

    • Convert all codes in the stdlib to not use "(...)" in PyArg_ParseTuple(). This was mainly done in bpo-6083. Perhaps resource.prlimit() was added after this.

    • Deprecate this dangerous feature. Early is better. And emit a warning to all core developers.

    @vstinner
    Copy link
    Member Author

    There is another option: modify the function to use argument clinic and implement something in argument clinic to hold a reference on borrowed references "O" and hold a reference on "(...)" sequence.

    Holding a reference on borrowed references "O" would make Python more safer against a whole class of bugs.

    @vadmium
    Copy link
    Member

    vadmium commented Dec 11, 2016

    Revision 4bac47eb444c fixed setrlimit(). Perhaps those changes can just be applied again to prlimit().

    I’m not an Arg Clinic expert, but isn’t one of its purposes to imitate native Python function signatures? Since argument unpacking was dropped from Python 2 to 3, I doubt Arg Clinic would grow support for this. I think the tuple should be unpacked inside the implementation body, just as a native Py 3 function has to unpack tuple arguments.

    BTW I couldn’t get either Victor’s "\udbff" test case, nor Serhiy’s BadSequence test case to crash, but the original MyNum class from bpo-6083 does crash.

    @vadmium vadmium added the 3.7 (EOL) end of life label Dec 11, 2016
    @larryhastings
    Copy link
    Contributor

    Sorry, Argument Clinic doesn't support automatic tuple unpacking for arguments. It was almost never used, I don't think it was ever a good idea, and it would have made an already-too-complicated program even more complicated-er.

    @serhiy-storchaka
    Copy link
    Member

    Following patch applies to resource.prlimit() the same solution as to resource.setrlimit().

    @serhiy-storchaka serhiy-storchaka added the extension-modules C modules in the Modules dir label Dec 11, 2016
    @serhiy-storchaka serhiy-storchaka self-assigned this Dec 16, 2016
    @vadmium
    Copy link
    Member

    vadmium commented Dec 19, 2016

    Patch looks good to me. Although maybe you don’t need the IndexError check in the test. Won’t limit[key] already handle that for you (as long as key isn’t -1 etc).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 19, 2016

    New changeset dac72bc14c00 by Serhiy Storchaka in branch '3.5':
    Issue bpo-20191: Fixed a crash in resource.prlimit() when pass a sequence that
    https://hg.python.org/cpython/rev/dac72bc14c00

    New changeset 7bc2923a41b6 by Serhiy Storchaka in branch '3.6':
    Issue bpo-20191: Fixed a crash in resource.prlimit() when pass a sequence that
    https://hg.python.org/cpython/rev/7bc2923a41b6

    New changeset b4d2bff1c5f8 by Serhiy Storchaka in branch 'default':
    Issue bpo-20191: Fixed a crash in resource.prlimit() when pass a sequence that
    https://hg.python.org/cpython/rev/b4d2bff1c5f8

    @serhiy-storchaka
    Copy link
    Member

    Although maybe you don’t need the IndexError check in the test.

    Good point. Thank you for your review Martin!

    @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 extension-modules C modules in the Modules dir type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants