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
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
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.
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)
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
___
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
___
___
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
___
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
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.
--
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
___
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
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
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
___
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
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
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,
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
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
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) {
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
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
___
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,
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 */
23 matches
Mail list logo
