[issue20434] Process crashes if not enough memory to import module
Eric Snow added the comment: I was going to say we should consider changing the API of _PyBytes_Resize() and _PyString_Resize(). However, having looked at the two functions, I guess it makes sense. Looking at the patch, I'd argue that we still need to set the string to NULL in the error case. Only in the out-of-memory case do the two functions change it to NULL for you. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
Kristján Valur Jónsson added the comment: I would also advocate for a better api, that leaves it up to the caller what to do, much like realloc() does. A convenience macro that frees the block on error could then be provided. But this is 2.7 and we don't change stuff there :) Can you elaborate on your second comment? Is there some place where I forgot to clear the object? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
Eric Snow added the comment: For example, in the patch binascii_b2a_uu() in Modules/binascii.c no longer sets rv to NULL even though in one of the _PyString_Resize() error cases rv is not automatically set to NULL. And simply setting rv to NULL would be backward-incompatible as well. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
Kristján Valur Jónsson added the comment:
This is _PyString_Resize(). I don't immediatlly see an error case where the
string isn't freed:
int
_PyString_Resize(PyObject **pv, Py_ssize_t newsize)
{
register PyObject *v;
register PyStringObject *sv;
v = *pv;
if (!PyString_Check(v) || Py_REFCNT(v) != 1 || newsize 0 ||
PyString_CHECK_INTERNED(v)) {
*pv = 0;
Py_DECREF(v);
PyErr_BadInternalCall();
return -1;
}
/* XXX UNREF/NEWREF interface should be more symmetrical */
_Py_DEC_REFTOTAL;
_Py_ForgetReference(v);
*pv = (PyObject *)
PyObject_REALLOC((char *)v, PyStringObject_SIZE + newsize);
if (*pv == NULL) {
PyObject_Del(v);
PyErr_NoMemory();
return -1;
}
_Py_NewReference(*pv);
sv = (PyStringObject *) *pv;
Py_SIZE(sv) = newsize;
sv-ob_sval[newsize] = '\0';
sv-ob_shash = -1; /* invalidate cached hash value */
return 0;
}
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20434
___
___
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
Eric Snow added the comment: Yeah, I missed the *pv = 0; line in the first error case. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
Kristján Valur Jónsson added the comment: Ok, are we good to go then? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
Eric Snow added the comment: Okay from me, but Victor has a better idea of the string APIs. :) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
STINNER Victor added the comment: I'm busy right now with Pycon (I organize a sprint to port OpenStack to Python 3). I will try to review the patch later this week. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
Kristján Valur Jónsson added the comment: Sure, there was at least one case in the patch, where the string resize was consider optional, and the code tried to recover if it didn't succeed. But I don't think we should be trying to change apis, even internal ones in python 2.7. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
Kristján Valur Jónsson added the comment: Could someone please review this patch? I'd like to see it committed asap. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
Eric Snow added the comment: I don't see a review link. Looks like your patch wasn't against tip (at attach-time) or you used the --git flag in diff. Having the patch in the review tool would be really would be really helpful. I'll take a look otherwise, but won't be able to immediately regardless. -- nosy: +eric.snow ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
Kristján Valur Jónsson added the comment: Ok, retrying without the --git flag (I thought that was recommended, it was once...) -- Added file: http://bugs.python.org/file34784/string_resize.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
Changes by Kristján Valur Jónsson krist...@ccpgames.com: Removed file: http://bugs.python.org/file34779/string_resize.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
Andrew Svetlov added the comment: Looks like the issue has gone after 3.4 Anf it's still present for 2.7. If somebody like to make a patch for 2.7 - you are welcome! -- versions: -Python 3.3 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
Kristján Valur Jónsson added the comment: Here we are. There were a lot of places where this was being incorrectly done. And some places where this was being considered a recoverable error, which it isn't because the source is freed. Which sort of supports my opinion that this is bad general api design, but perhaps a good conveneice function for limited use. -- Added file: http://bugs.python.org/file34779/string_resize.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
Kristján Valur Jónsson added the comment: These are very unusual semantics. The convention in the python api is that functions are refernece-invariant when there are errors. i.e. if a function fails or not does not change the caller's reference passing assumptions. For example, Py_BuildValue(N, myobject); takes care to always steal the reference of myobject, even when Py_BuildValue fails. Thi tehe case of _PyBytes_Resize(), the caller owns the (single) reference to the operand, and owns the reference to it (or a new one) on success. It is highly unusual that the case of failure causes it to no longer own this reference. Python 3 should have taken the opportunity to remove remove this unusual inheritance from _PyString_Resize() -- nosy: +kristjan.jonsson ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
STINNER Victor added the comment: Python 3 should have taken the opportunity to remove remove this unusual inheritance from _PyString_Resize() It's not so unusual. PyUnicode_InternInPlace() replaces also a pointer to a object in the caller for example. There are many other functions taking PyObject** parameters: PyUnicode_Append(), PyDict_Next(), PyEval_EvalCodeEx(), PyErr_Fetch(), etc. Anyway, it's too late to change such major API, so it's not very useful the discuss this theorical change :-) And the function is well documented: http://docs.python.org/dev/c-api/bytes.html#_PyBytes_Resize If the reallocation fails, the original bytes object at *bytes is deallocated, *bytes is set to NULL, a memory exception is set, and -1 is returned. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
Kristján Valur Jónsson added the comment: I'm not talking about the PyObject** argument, Victor. I'm talking about reference counting semantics. It is a rule that reference counting semantics should be the same over a function call whether that function raised an exception or not. The this function effectively steals a reference in case of error. The caller owns the reference to the argument (passed by ref) if it succeeds, but if id doesn't, then he doesn't own it anymore. Reference counting invariance with errors is, as I mentioned, observed with e.g. the 'N' argument to Py_BuildValue(), which is defined to steal a reference and does so even if the call fails. This behaviour is observed by other reference-stealing functions, such as PyTuple_SetItem(). Similarly, functions who don't steal a reference, i.e. take their own, will not change that behaviour if they error. If you don't want to think about this in terms of reference counting semantics, think about it in terms of the fact that in case of error, most functions leave everything as it was. PyList_Append(), if it fails, leaves everything as it was. This function does not. In case of failure, it will, as a convenience to the caller, release the original object. It is equivalent to () realloc() freeing its operand if it cannot succeed. It is precisely these 'unusual' exceptions from established semantics that cause these kind of programming errors. Originally, this was probably designed as a convenience to the programmer for the handful of places that the function (_PyString_Resize) was used. But this decision forces every new user of this function (and its descendents) to be acutely aware of its unusual error behaviour. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
New submission from Vladimir Kerimov:
In the file _io\fileio.c in function
static PyObject *
fileio_readall(fileio *self)
this code is incorrect and crashes the process of Python:
if (_PyBytes_Resize(result, newsize) 0) {
if (total == 0) {
Py_DECREF(result);
return NULL;
}
PyErr_Clear();
break;
}
In the call of _PyBytes_Resize there the result variable passed by reference
and changes value to NULL-pointer when function fails and return 0. So on the
line Py_DECREF(result); the Python process crashes.
--
components: Interpreter Core
messages: 209624
nosy: asvetlov, qualab
priority: normal
severity: normal
status: open
title: Process crashes if not enough memory to import module
type: crash
versions: Python 3.3
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20434
___
___
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
Andrew Svetlov added the comment: Bug can be reproduced if _PyBytes_Resize fails with out-of-memory error than NULL object decrefed. Buggy modules are _io, binascii and zlib. 3.4 hasn't the problem. Patch for 3.3 is attached. Fix goes to mimic 3.4 -- (replace Py_DECREF with Py_CLEAR), while for _PyBytes_Resize that does not make sense, object has been set to NULL inside _PyBytes_Resize on fail. Also 2.7 has the same issue for _PyString_Resize calls. -- components: +Extension Modules -Interpreter Core keywords: +patch versions: +Python 2.7 Added file: http://bugs.python.org/file33791/issue20434.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
Changes by Andrew Svetlov andrew.svet...@gmail.com: -- keywords: +needs review stage: - patch review ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
STINNER Victor added the comment: Ah yes, the bytes object is set to NULL. In fact, you don't need to call Py_CLEAR(retval) in case of error, because retval is already NULL. Could you please update your patch to just do nothing on retval in case of error please? 3.4 hasn't the problem. Yes, I played some weeks with pyfailmalloc, a new tool for Python 3.4 to inject random MemoryError errors. I fixed many other different issues. -- nosy: +haypo ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue20434] Process crashes if not enough memory to import module
Serhiy Storchaka added the comment: There are other places where Py_DECREF is called after failed _PyBytes_Resize(). For example in PyBytes_FromObject(): if (_PyBytes_Resize(new, size) 0) goto error; ... error: /* Error handling when new != NULL */ Py_XDECREF(it); Py_DECREF(new); return NULL; -- nosy: +serhiy.storchaka ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20434 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
