bpo-38677: Fix arraymodule error handling in module initialization

[mpaolini]

https://bugs.python.org/issue38677

[brandtbucher]

Thanks, this looks like a good change overall. The only things I would suggest are:

• Formatting: braces and 4-space indents, as per PEP 7.
• Refcount fixes: PyModule_AddObject is really tricky in regards to these.

See below.

[brandtbucher]

Why is this assert here, rather than checking for an error in the line above and handling it normally?

[mpaolini]

because the buffer is filled sith hardcoded chars set in this module, and the PyUnicode_DecodeASCII docs say Return NULL if an exception was raised by the codec. ... so I thought ...

well I checked PyUnicode_DecodeASCII and it can return null also in other cases (for instance when PyUnicodeNew fails.

Will fix this now

[mpaolini]

fixed now, please note that in this error case (and in the one above) there still are refleaks.

When the referencs are stolen by the PyModule_AddObject (in the ok paths), nothing decrefs those in the following error paths...

[brandtbucher]

Pretty sure those are cleaned up whenever the module is (the stolen references reside in its __dict__ now).

[mpaolini]

with this patch applied

diff --git a/Modules/arraymodule.c b/Modules/arraymodule.c
index 5b58658709..ec8dd3d942 100644
--- a/Modules/arraymodule.c
+++ b/Modules/arraymodule.c
@@ -3067,7 +3067,7 @@ array_modexec(PyObject *m)
         return -1;
     }
 
-    return 0;
+    return -1;
 }
 
 static PyModuleDef_Slot arrayslots[] = {

the following program leaks

import time
import gc
import os
print(os.getpid())
gc.disable()
for i in range(1000):
    try:
        import array
    except Exception:
        time.sleep(0.1)

with and without the Py_DECREF

[brandtbucher]

I'm not sure what you mean when you say "with and without the Py_DECREF"... does the same leak occur with:

-    return 0;
+    Py_DECREF(m);
+    return -1;

Either way, this looks good to me. This can be a question for the core reviewer.

[mpaolini]

I made this module to always fail on exec by returning -1

then, withor without the decrefs on error paths (see next patch), I get same leaking behaviour

diff --git a/Modules/arraymodule.c b/Modules/arraymodule.c
index ec8dd3d942..aacef20f20 100644
--- a/Modules/arraymodule.c
+++ b/Modules/arraymodule.c
@@ -3041,12 +3041,10 @@ array_modexec(PyObject *m)
 
     Py_INCREF((PyObject *)&Arraytype);
     if (PyModule_AddObject(m, "ArrayType", (PyObject *)&Arraytype) < 0) {
-        Py_DECREF((PyObject *)&Arraytype);
         return -1;
     }
     Py_INCREF((PyObject *)&Arraytype);
     if (PyModule_AddObject(m, "array", (PyObject *)&Arraytype) < 0) {
-        Py_DECREF((PyObject *)&Arraytype);
         return -1;
     }
 
@@ -3063,7 +3061,6 @@ array_modexec(PyObject *m)
         return -1;
     }
     if (PyModule_AddObject(m, "typecodes", typecodes) < 0) {
-        Py_DECREF(typecodes);
         return -1;
     }

[brandtbucher]

Well, you don't hit those branches, right? So I wouldn't expect a difference.

[mpaolini]

awwww yes you are right!

[mpaolini]

...anyways this seems to point to the fact that Py_CLEAR(module) is not needed on error...

[brandtbucher]

Thanks @mpaolini!

[serhiy-storchaka]

LGTM, but the check for typecodes is actually not needed.

[serhiy-storchaka]

It is not needed. PyModule_AddObject() checks the added object for NULL.

If you remove it, change also Py_DECREF(typecodes) to Py_XDECREF(typecodes).

[mpaolini]

done in 777fc61

[miss-islington]

Thanks @mpaolini for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

[bedevere-bot]

GH-17166 is a backport of this pull request to the 3.8 branch.

[brandtbucher]

Thanks @mpaolini!