
This issue trackerhas been migrated toGitHub, and is currentlyread-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.
Created on2014-01-08 14:33 byvstinner, last changed2022-04-11 14:57 byadmin. This issue is nowclosed.
| Files | ||||
|---|---|---|---|---|
| File name | Uploaded | Description | Edit | |
| prlimit_refcount.patch | serhiy.storchaka,2016-12-11 07:43 | review | ||
| Pull Requests | |||
|---|---|---|---|
| URL | Status | Linked | Edit |
| PR 552 | closed | dstufft,2017-03-31 16:36 | |
| Messages (9) | |||
|---|---|---|---|
| msg207686 -(view) | Author: STINNER Victor (vstinner)*![]() | Date: 2014-01-08 14:33 | |
$ ./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 | |||
| msg207692 -(view) | Author: Serhiy Storchaka (serhiy.storchaka)*![]() | Date: 2014-01-08 16:20 | |
Thank you for good example, Victor. Seeissue6083 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 inissue6083. Perhaps resource.prlimit() was added after this.* Deprecate this dangerous feature. Early is better. And emit a warning to all core developers. | |||
| msg208272 -(view) | Author: STINNER Victor (vstinner)*![]() | Date: 2014-01-16 11:06 | |
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. | |||
| msg282897 -(view) | Author: Martin Panter (martin.panter)*![]() | Date: 2016-12-11 03:14 | |
Revision4bac47eb444c 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 fromIssue 6083 does crash. | |||
| msg282898 -(view) | Author: Larry Hastings (larry)*![]() | Date: 2016-12-11 03:17 | |
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. | |||
| msg282909 -(view) | Author: Serhiy Storchaka (serhiy.storchaka)*![]() | Date: 2016-12-11 07:43 | |
Following patch applies to resource.prlimit() the same solution as to resource.setrlimit(). | |||
| msg283584 -(view) | Author: Martin Panter (martin.panter)*![]() | Date: 2016-12-19 05:39 | |
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). | |||
| msg283586 -(view) | Author: Roundup Robot (python-dev)![]() | Date: 2016-12-19 06:07 | |
New changesetdac72bc14c00 by Serhiy Storchaka in branch '3.5':Issue#20191: Fixed a crash in resource.prlimit() when pass a sequence thathttps://hg.python.org/cpython/rev/dac72bc14c00New changeset7bc2923a41b6 by Serhiy Storchaka in branch '3.6':Issue#20191: Fixed a crash in resource.prlimit() when pass a sequence thathttps://hg.python.org/cpython/rev/7bc2923a41b6New changesetb4d2bff1c5f8 by Serhiy Storchaka in branch 'default':Issue#20191: Fixed a crash in resource.prlimit() when pass a sequence thathttps://hg.python.org/cpython/rev/b4d2bff1c5f8 | |||
| msg283588 -(view) | Author: Serhiy Storchaka (serhiy.storchaka)*![]() | Date: 2016-12-19 06:09 | |
> Although maybe you don’t need the IndexError check in the test.Good point. Thank you for your review Martin! | |||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022-04-11 14:57:56 | admin | set | github: 64390 |
| 2017-03-31 16:36:17 | dstufft | set | pull_requests: +pull_request912 |
| 2016-12-19 06:09:45 | serhiy.storchaka | set | status: open -> closed resolution: fixed messages: +msg283588 stage: patch review -> resolved |
| 2016-12-19 06:07:53 | python-dev | set | nosy: +python-dev messages: +msg283586 |
| 2016-12-19 05:39:53 | martin.panter | set | messages: +msg283584 |
| 2016-12-16 12:22:43 | serhiy.storchaka | set | assignee:serhiy.storchaka |
| 2016-12-11 07:43:45 | serhiy.storchaka | set | files: +prlimit_refcount.patch messages: +msg282909 components: + Extension Modules keywords: +patch stage: needs patch -> patch review |
| 2016-12-11 03:17:23 | larry | set | messages: +msg282898 |
| 2016-12-11 03:14:20 | martin.panter | set | stage: needs patch |
| 2016-12-11 03:14:05 | martin.panter | set | nosy: +martin.panter messages: +msg282897 versions: + Python 3.5, Python 3.6, Python 3.7, - Python 3.4 |
| 2016-12-11 03:13:20 | martin.panter | link | issue6083 dependencies |
| 2014-01-16 11:06:38 | vstinner | set | nosy: +larry messages: +msg208272 |
| 2014-01-16 10:55:31 | serhiy.storchaka | set | priority: normal -> critical |
| 2014-01-08 16:20:05 | serhiy.storchaka | set | messages: +msg207692 |
| 2014-01-08 14:33:42 | vstinner | create | |