Skip to content

bpo-38677: Fix arraymodule error handling in module initialization #17039

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Nov 15, 2019

Conversation

mpaolini
Copy link
Contributor

@mpaolini mpaolini commented Nov 3, 2019

Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -3053,13 +3059,13 @@ array_modexec(PyObject *m)
*p++ = (char)descr->typecode;
}
typecodes = PyUnicode_DecodeASCII(buffer, p - buffer, NULL);
assert(typecodes != NULL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

@brandtbucher brandtbucher Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@mpaolini mpaolini Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
     }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awwww yes you are right!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@brandtbucher
Copy link
Member

Thanks @mpaolini!

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (PyErr_Occurred()) {
Py_DECREF(m);
m = NULL;
if (typecodes == NULL) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 777fc61

@miss-islington
Copy link
Contributor

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

@bedevere-bot
Copy link

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

miss-islington added a commit that referenced this pull request Nov 15, 2019
@brandtbucher
Copy link
Member

Thanks @mpaolini!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants