Skip to content
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

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

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

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.

Modules/arraymodule.c Outdated Show resolved Hide resolved
Modules/arraymodule.c Outdated Show resolved Hide resolved
Modules/arraymodule.c Outdated Show resolved Hide resolved
Modules/arraymodule.c Outdated Show resolved Hide resolved
Modules/arraymodule.c Outdated Show resolved Hide resolved
Modules/arraymodule.c Outdated Show resolved Hide resolved
mpaolini and others added 5 commits Nov 4, 2019
Co-Authored-By: Brandt Bucher <brandtbucher@gmail.com>
Co-Authored-By: Brandt Bucher <brandtbucher@gmail.com>
Co-Authored-By: Brandt Bucher <brandtbucher@gmail.com>
Co-Authored-By: Brandt Bucher <brandtbucher@gmail.com>
@@ -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

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

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

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.

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

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

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

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

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

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

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

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

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

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.

awwww yes you are right!

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.

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

@brandtbucher
Copy link
Member

brandtbucher commented Nov 4, 2019

Thanks @mpaolini!

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

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

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

@serhiy-storchaka serhiy-storchaka Nov 14, 2019

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

@mpaolini mpaolini Nov 14, 2019

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

miss-islington commented Nov 15, 2019

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

bedevere-bot commented Nov 15, 2019

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
…H-17039)

(cherry picked from commit b44ffc8)

Co-authored-by: Marco Paolini <mpaolini@users.noreply.github.com>
@brandtbucher
Copy link
Member

brandtbucher commented Nov 15, 2019

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.

None yet

8 participants