bpo-38037: Fix reference counters in signal module

[ghost]

Only 3.8 and 3.9 branches are affected.

https://bugs.python.org/issue38037

[brandtbucher]

I'm not familiar with this file, but it looks like these Py_XINCREF calls should be made after the calls to PyModule_AddObject, correct?

If PyModule_AddObject fails, the additional reference isn't stolen... so it isn't needed.

[ghost]

I'm not familiar with this file, but it looks like these Py_XINCREF calls should be made after the calls to PyModule_AddObject, correct?

If PyModule_AddObject fails, the additional reference isn't stolen... so it isn't needed.

Search PyModule_AddObject in CPython code.
It seems always Py_INCREF first, then call PyModule_AddObject.

If Py_INCREF after calls, maybe the object is already deallocated inside PyModule_AddObject function?
https://github.com/python/cpython/blob/3.8/Python/modsupport.c#L654

[ghost]

No need needs backport to 3.7 label, only 3.8 and 3.9 branches are affected.

This is a regression caused by commit 9541bd3 (22 Apr 2019)

[brandtbucher]

If Py_INCREF after calls, maybe the object is already deallocated inside PyModule_AddObject function?
https://github.com/python/cpython/blob/3.8/Python/modsupport.c#L654

Ah, yes. I at least think that we should Py_XDECREF in the failure condition (before goto finally;) though, right?

[brandtbucher]

The failure looks like it's due to https://bugs.python.org/issue34037, unrelated. Closing and reopening the PR should trigger a rebuild!

[ghost]

Ah, yes. I at least think that we should Py_XDECREF in the failure condition (before goto finally;) though, right?

Search in CPython code, some sites don't check PyModule_AddObject's return value, some sites don't do Py_DECREF if PyModule_AddObject fails. Looks less normative.

I did all in the last commit.

[vstinner]

LGTM, but I have a few minor comments.

@nanjekyejoannah: Would you mind to also review this change? I would prefer to have a second reviewer.

[brandtbucher]

@vstinner It looks like the idiom of calling PyModule_AddObject without Py_DECREF'ing on the error condition (or even checking for it at all) has spread quite a bit since https://bugs.python.org/issue26868 was first reported. I'll prepare a PR to fix the other call sites!

I don't want other contributors to think that this is the correct way to use it.

[ghost]

Comments addressed.

[ghost]

If you don't mind, I want to reuse PyDict_SetItemString().
It keeps the code neat, and saves a pair of Py_INCREF()/Py_DECREF().

compare

    DefaultHandler = PyLong_FromVoidPtr((void *)SIG_DFL);
    if (!DefaultHandler ||
        PyDict_SetItemString(d, "SIG_DFL", DefaultHandler) < 0) {
        goto finally;
    }

to

    DefaultHandler = PyLong_FromVoidPtr((void *)SIG_DFL);
    if (!DefaultHandler) {
        goto finally;
    }
    Py_INCREF(DefaultHandler);
    if (PyModule_AddObject(m, "SIG_DFL", DefaultHandler)) {
        Py_DECREF(DefaultHandler);
        goto finally;
    }

[nanjekyejoannah]

If you don't mind, I want to reuse PyDict_SetItemString().
It keeps the code neat, and saves a pair of Py_INCREF()/Py_DECREF().

compare

    DefaultHandler = PyLong_FromVoidPtr((void *)SIG_DFL);
    if (!DefaultHandler ||
        PyDict_SetItemString(d, "SIG_DFL", DefaultHandler) < 0) {
        goto finally;
    }

to

    DefaultHandler = PyLong_FromVoidPtr((void *)SIG_DFL);
    if (!DefaultHandler) {
        goto finally;
    }
    Py_INCREF(DefaultHandler);
    if (PyModule_AddObject(m, "SIG_DFL", DefaultHandler)) {
        Py_DECREF(DefaultHandler);
        goto finally;
    }

Follow the discussion here : https://bugs.python.org/issue24011 on replacing PyDict_SetItemString() calls with PyModule_AddIntMacro() . By the time I made this change, all the PyModule_AddIntMacro() changes had been made save for this in this commit 6782b14

[nanjekyejoannah]

Moving this to a comment : Follow the discussion here : https://bugs.python.org/issue24011 on replacing PyDict_SetItemString() calls with PyModule_AddIntMacro() . By the time I made this change, all the PyModule_AddIntMacro() changes had been made save for this in this commit 6782b14

[ghost]

I still prefer to use PyDict_SetItemString().

IMO it's suitable for this situation, PyModule_AddObject() is more suitable for adding an object that doesn't need to be held by other variable.

I have another problem.
In issue24011's signalmodule.patch by Christian Heimes, there is a change:

+  finally:
     if (PyErr_Occurred()) {
         Py_DECREF(m);
         m = NULL;
     }
 
-  finally:
     return m;
 }

But this change has not been adopted, is it intentional or negligent? @tiran

[vstinner]

I don't think that PyModule_AddIntMacro() can be used in this PR. I like the removal of "x" variable, and reuse DefaultHandler, IgnoreHandler and ItimerError.

[vstinner]

@vstinner It looks like the idiom of calling PyModule_AddObject without Py_DECREF'ing on the error condition (or even checking for it at all) has spread quite a bit since https://bugs.python.org/issue26868 was first reported. I'll prepare a PR to fix the other call sites! I don't want other contributors to think that this is the correct way to use it.

I suggest you to open a new issue at bugs.python.org for that. Before fixing the usage, I suggest to enhance the documentation. Maybe add a short code example:
https://docs.python.org/dev/c-api/module.html?highlight=pymodule_addobject#c.PyModule_AddObject

[vstinner]

@nanjekyejoannah: PyModule_AddObject() has terrible API, it's easy to misuse it...

[nanjekyejoannah]

@vstinner @animalize agreed.

[vstinner]

@vstinner @animalize agreed.

If the PR looks good to you, would you mind to use GitHub UI to approve the PR?

[vstinner]

@animalize: If you want to use PyDict_SetItemString(), please open a separated PR so I can compare the two approaches. Using PyDict_SetItemString() sounds like a good idea: internally, PyModule_AddObject calls PyDict_SetItemString, but in PyInit__signal() we already have have "d" (module dictionary).

[nanjekyejoannah]

In terms of fixing the current regression, this LGTM.

[ghost]

@animalize: If you want to use PyDict_SetItemString(), please open a separated PR so I can compare the two approaches. Using PyDict_SetItemString() sounds like a good idea: internally, PyModule_AddObject calls PyDict_SetItemString, but in PyInit__signal() we already have have "d" (module dictionary).

Please see this PR, it uses PyDict_SetItemString: #15753

[vstinner]

I merged PR #15753 which is simpler than this PR.

[ghost]

Thanks for your reviews!

[brandtbucher]

I suggest you to open a new issue at bugs.python.org for that. Before fixing the usage, I suggest to enhance the documentation. Maybe add a short code example:
https://docs.python.org/dev/c-api/module.html?highlight=pymodule_addobject#c.PyModule_AddObject

I'll open an issue about live code changes. I've already got a doc patch at #15725!