[issue26871] Change weird behavior of PyModule_AddObject()

[Petr Viktorin]

Petr Viktorin  added the comment:

Since https://github.com/python/cpython/pull/23122, there is 
PyModule_AddObjectRef doesn't steal a reference.
PyModule_AddObject is discouraged in the docs, but not formally deprecated. (As 
Stefan notes, the leaks are single references in case of a module 
initialization failure -- the cost of using the API incorrectly is very small; 
requiring everyone to change their code because it was possible to misuse it is 
overkill.)

See https://docs.python.org/3.10/c-api/module.html#c.PyModule_AddObjectRef

I think this is enough to close the issue.

--
nosy: +petr.viktorin
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26871] Change weird behavior of PyModule_AddObject()

[Stefan Krah]

Stefan Krah added the comment:

Serhiy, I'm sorry that I overreacted here. You're doing great work for
Python -- we just happen to disagree on this one particular issue.

I think there were some proponents on python-dev, perhaps they'll show
up and discuss the details.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26871] Change weird behavior of PyModule_AddObject()

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Removed changes in _decimal.c and deprecation. Used Py_XDECREF in 
_PyModule_AddObject().

--
Added file: http://bugs.python.org/file42639/pymodule_addobject_2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26871] Change weird behavior of PyModule_AddObject()

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

> It seems that the patch also introduces a segfault if PyLong_FromSsize_t() 
> returns NULL.

Good catch Stefan! Py_XDECREF ahould be used instead of Py_DECREF in 
_PyModule_AddObject().

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26871] Change weird behavior of PyModule_AddObject()

[Stefan Krah]

Stefan Krah added the comment:

It seems that the patch also introduces a segfault if PyLong_FromSsize_t() 
returns NULL.

--
nosy: +skrah

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26871] Change weird behavior of PyModule_AddObject()

[Stefan Krah]

Changes by Stefan Krah :


--
nosy:  -skrah

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26871] Change weird behavior of PyModule_AddObject()

[Stefan Krah]

Stefan Krah added the comment:

Your definition of correctness is very odd. The "leaks" you are talking
about are single references in case of a module initialization failure,
in which case you are likely to have much bigger problems.


Please take _decimal out of this patch, it's a waste of time.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26871] Change weird behavior of PyModule_AddObject()

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Idiomatic code is

if (PyModule_AddObject(module, "name", create_new_object()) < 0)
goto error;

If you already have a reference and need to use it later:

obj = create_new_object();
... /* use obj */
Py_INCREF();
if (PyModule_AddObject(module, "name", create_new_object()) < 0)
goto error;
... /* use obj */
Py_DECREF(obj);

error:
Py_XDECREF(obj);

Many current code use above idioms, but it doesn't work as expected.

It is almost impossible to write correct code with current behavior. And 
_decimal.c is not an exception, it has leaks.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26871] Change weird behavior of PyModule_AddObject()

[Stefan Krah]

Stefan Krah added the comment:

I think the current behavior is good for error handling by goto,
which is common for module initialization.

Please don't change this.

--
nosy: +skrah

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26871] Change weird behavior of PyModule_AddObject()

[Serhiy Storchaka]

New submission from Serhiy Storchaka:

PyModule_AddObject() has weird and counterintuitive behavior. It steals a 
reference only on success. The caller is responsible to decref it on error. 
This behavior was not documented and inconsistent with the behavior of other 
functions stealing a reference (PyList_SetItem() and PyTuple_SetItem()). Seems 
most developers don't use this function correctly, since only in few places in 
the stdlib a reference is decrefed explicitly after PyModule_AddObject() 
failure.

This weird behavior was first reported in issue1782, and changing it was 
proposed. Related bugs in PyModule_AddIntConstant() and 
PyModule_AddStringConstant() were fixed, but the behavior of 
PyModule_AddObject() was not changed and not documented.

This issue is opened for gradual changing the behavior of PyModule_AddObject(). 
Proposed patch introduces new macros PY_MODULE_ADDOBJECT_CLEAN that controls 
the behavior of PyModule_AddObject() as PY_SSIZE_T_CLEAN controls the behavior 
of PyArg_Parse* functions. If the macro is defined before including "Python.h", 
PyModule_AddObject() steals a reference unconditionally.  Otherwise it steals a 
reference only on success, and the caller is responsible for decref'ing it on 
error (current behavior). This needs minimal changes to source code if 
PyModule_AddObject() was used incorrectly (i.e. as documented), and keep the 
code that explicitly decref a reference after PyModule_AddObject() working 
correctly.

Use of PyModule_AddObject() without defining PY_MODULE_ADDOBJECT_CLEAN is 
declared deprecated (or we can defer this to 3.7). In the distant future (after 
dropping the support of 2.7) the old behavior will be dropped.

See also a discussion on Python-Dev: 
http://comments.gmane.org/gmane.comp.python.devel/157545 .

--
components: Extension Modules, Interpreter Core
files: pymodule_addobject.patch
keywords: patch
messages: 264384
nosy: serhiy.storchaka
priority: normal
severity: normal
stage: patch review
status: open
title: Change weird behavior of PyModule_AddObject()
type: enhancement
versions: Python 3.6
Added file: http://bugs.python.org/file42632/pymodule_addobject.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com