-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
resource.prlimit(int, int, str) crashs #64390
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
Comments
$ ./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:
|
Thank you for good example, Victor. See bpo-6083 for early discussion. As for options:
So what we should to do:
|
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. |
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. |
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. |
Following patch applies to resource.prlimit() the same solution as to resource.setrlimit(). |
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). |
New changeset dac72bc14c00 by Serhiy Storchaka in branch '3.5': New changeset 7bc2923a41b6 by Serhiy Storchaka in branch '3.6': New changeset b4d2bff1c5f8 by Serhiy Storchaka in branch 'default': |
Good point. Thank you for your review Martin! |
Misc/NEWS
so that it is managed by towncrier #552Note: 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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: