Add Py_REPLACE and Py_XREPLACE macros
The Py_CLEAR macros is used as safe alternative for following unsafe idiomatic code: Py_XDECREF(ptr); ptr = NULL; But other unsafe idiomatic code is widely used in the sources: Py_XDECREF(ptr); ptr = new_value; Every occurrence of such code is potential bug for same reasons as for Py_CLEAR. It was offered [1] to introduce new macros Py_REPLACE and Py_XREPLACE for safe replace with Py_DECREF and Py_XDECREF respectively. Automatically generated patch contains about 50 replaces [2]. [1] http://bugs.python.org/issue16447 [2] http://bugs.python.org/issue20440
Antoine already proposed similar macros [1] [2]. But now we have about 50 potential bugs which can be fixed with these macros. [1] https://mail.python.org/pipermail/python-dev/2008-May/079862.html [2] http://bugs.python.org/issue20440
29.01.14 21:10, Serhiy Storchaka написав(ла):
Antoine already proposed similar macros [1] [2]. But now we have about 50 potential bugs which can be fixed with these macros.
[1] https://mail.python.org/pipermail/python-dev/2008-May/079862.html [2] http://bugs.python.org/issue20440
29.01.14 20:24, Serhiy Storchaka написав(ла):
The Py_CLEAR macros is used as safe alternative for following unsafe idiomatic code:
Py_XDECREF(ptr); ptr = NULL;
But other unsafe idiomatic code is widely used in the sources:
Py_XDECREF(ptr); ptr = new_value;
Every occurrence of such code is potential bug for same reasons as for Py_CLEAR.
It was offered [1] to introduce new macros Py_REPLACE and Py_XREPLACE for safe replace with Py_DECREF and Py_XDECREF respectively. Automatically generated patch contains about 50 replaces [2].
[1] http://bugs.python.org/issue16447 [2] http://bugs.python.org/issue20440
There are objections to these patches. Raymond against backporting the patch unless some known bugs are being fixed [1]. But it fixes at least one bug that caused a crash. And I suspect that there may be other bugs, just we still have no reproducers. Even if we don't know how to reproduce the bug, the current code looks obviously wrong. Also porting the patch will make the sync easier. Note that the patches were automatically generated, which reduces the possibility of errors. I just slightly corrected formatting, remove unused variables and fixed one error. Martin's objections are that the macros do add to the learning curve and his expects that Py_REPLACE(op, op2) does an INCREF on op2, but it does not [2]. Antoine's original Py_(X)SETREF macros did INCREF and seems this was one of their flaw, because in most cases INCREF is not needed. Alternative names Py_(X)ASSIGN were suggested to break connotation of an INCREF [3]. As for learning curve, I think that it would be better to have standard macros for code that can be written (and often are written) incorrectly. And even already correct code can be written with these macros in more short and clear way [4]. Greg shared Martin's expectation and suggested to revive this thread. [1] http://bugs.python.org/issue20440#msg209701 [2] http://bugs.python.org/issue20440#msg209894 [3] http://bugs.python.org/issue20440#msg210447 [4] http://bugs.python.org/issue3081#msg102645
On 16 February 2014 02:52, Serhiy Storchaka <storchaka@gmail.com> wrote:
There are objections to these patches. Raymond against backporting the patch unless some known bugs are being fixed [1]. But it fixes at least one bug that caused a crash. And I suspect that there may be other bugs, just we still have no reproducers. Even if we don't know how to reproduce the bug, the current code looks obviously wrong. Also porting the patch will make the sync easier. Note that the patches were automatically generated, which reduces the possibility of errors. I just slightly corrected formatting, remove unused variables and fixed one error.
It's also likely than many of these crashes could only be reproduced through incorrect usage of the C API. Py_CLEAR/Py_XCLEAR was relatively simple, as there's only one pointer involved. As Martin noted, Py_REPLACE is more complex, as there's the question of whether or not the refcount for the RHS gets incremented. Rather than using a completely different name, perhaps we can leverage "Py_CLEAR" to make it more obvious that this operation is "like Py_CLEAR and for the same reason, just setting the pointer to something other than NULL". For example: Py_CLEAR_AND_SET Py_XCLEAR_AND_SET Such that Py_CLEAR and Py_XCLEAR are equivalent to: Py_CLEAR_AND_SET(target, NULL); Py_XCLEAR_AND_SET(target, NULL); (Note that existing occurrences of SET in C API macros like PyList_SET_ITEM and PyTuple_SET_ITEM also assume that any required reference count adjustments are handled externally). While the name does suggest the macro will expand to: Py_CLEAR(target); target = value; which isn't quite accurate, it's close enough that people should still be able to understand what the operation does. Explicitly pointing out in that docs that Py_CLEAR(target) is functionally equivalent to Py_CLEAR_AND_SET(target, NULL) should help correct the misapprehension. On the question of updating 3.4+ only vs also updating 3.3, I'm inclined towards fixing it systematically in all currently supported branches, as it's a low risk mechanical change. On the other hand, we still have other crash bugs with known reproducers (in Lib/test/crashers), so I'm also OK with leaving 3.3 alone. (Assuming we can agree on a name, I definitely think it's worth asking Larry for permission to make the late C API addition for 3.4, though) Regards, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
16.02.14 02:05, Nick Coghlan написав(ла):
It's also likely than many of these crashes could only be reproduced through incorrect usage of the C API.
Rather through queer or malicious usage of Python classes with strange code in __del__.
For example:
Py_CLEAR_AND_SET Py_XCLEAR_AND_SET
Such that Py_CLEAR and Py_XCLEAR are equivalent to:
There is no Py_XCLEAR. Py_CLEAR itself checks its argument for NULL (as Py_XDECREF and Py_XINCREF). And these names looks too cumbersome to me.
While the name does suggest the macro will expand to:
Py_CLEAR(target); target = value;
which isn't quite accurate, it's close enough that people should still be able to understand what the operation does.
This is not just inaccurate, this is wrong. Py_REPLACE first assigns new value and then DECREF old value. So it rather can be named as Py_SET_AND_DECREF, but of course this name looks ugly and confusing.
On 17 Feb 2014 06:12, "Serhiy Storchaka" <storchaka@gmail.com> wrote:
16.02.14 02:05, Nick Coghlan написав(ла):
It's also likely than many of these crashes could only be reproduced through incorrect usage of the C API.
Rather through queer or malicious usage of Python classes with strange
code in __del__. This change doesn't fix any of the known crashers in Lib/test/crashers, though - I applied the patch locally and checked.
For example:
Py_CLEAR_AND_SET Py_XCLEAR_AND_SET
Such that Py_CLEAR and Py_XCLEAR are equivalent to:
There is no Py_XCLEAR. Py_CLEAR itself checks its argument for NULL (as Py_XDECREF and Py_XINCREF). And these names looks too cumbersome to me.
While the name does suggest the macro will expand to:
Py_CLEAR(target); target = value;
which isn't quite accurate, it's close enough that people should still be able to understand what the operation does.
This is not just inaccurate, this is wrong. Py_REPLACE first assigns new value and then DECREF old value.
The point is that people already know what Py_CLEAR does. This operation is like Py_CLEAR (the old reference is only removed *after* the pointer has been updated), except that the value it is being replaced with can be something other than NULL. If the replacement value *is* NULL, then the new operation is *exactly* equivalent to Py_CLEAR. Operations that do related things should ideally have related names. The point of my deliberately erroneous expansion is that it's an error a reader can make while still correctly understanding the *logic* of the code, even though they're missing a subtlety of the mechanics.
So it rather can be named as Py_SET_AND_DECREF, but of course this name looks ugly and confusing.
An explicit name like Py_SET_AND_DECREF would also be reasonable. It's substantially less confusing than Py_REPLACE, as it is less ambiguous about whether or not the refcount on the new value is adjusted. Cheers, Nick.
_______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe:
https://mail.python.org/mailman/options/python-dev/ncoghlan%40gmail.com
17.02.14 01:27, Nick Coghlan написав(ла):
This change doesn't fix any of the known crashers in Lib/test/crashers, though - I applied the patch locally and checked.
It fixes other crasher (http://bugs.python.org/issue20440#msg209713).
The point is that people already know what Py_CLEAR does. This operation is like Py_CLEAR (the old reference is only removed *after* the pointer has been updated), except that the value it is being replaced with can be something other than NULL. If the replacement value *is* NULL, then the new operation is *exactly* equivalent to Py_CLEAR.
Operations that do related things should ideally have related names. The point of my deliberately erroneous expansion is that it's an error a reader can make while still correctly understanding the *logic* of the code, even though they're missing a subtlety of the mechanics.
Py_CLEAR and Py_DECREF have no related names. I think that the clarity and briefness are important. I assume that these macros will be widely used (they allow existing code to write shorter), perhaps even more than Py_CLEAR. Therefore people will know what they do.
An explicit name like Py_SET_AND_DECREF would also be reasonable. It's substantially less confusing than Py_REPLACE, as it is less ambiguous about whether or not the refcount on the new value is adjusted.
I agree if it will satisfy Martin, although this name looks ugly to me.
participants (2)
-
Nick Coghlan -
Serhiy Storchaka