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
Conversation
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.
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>
Modules/arraymodule.c
Outdated
@@ -3053,13 +3059,13 @@ array_modexec(PyObject *m) | |||
*p++ = (char)descr->typecode; | |||
} | |||
typecodes = PyUnicode_DecodeASCII(buffer, p - buffer, NULL); | |||
assert(typecodes != NULL); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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...
Thanks @mpaolini! |
Modules/arraymodule.c
Outdated
if (PyErr_Occurred()) { | ||
Py_DECREF(m); | ||
m = NULL; | ||
if (typecodes == NULL) { |
There was a problem hiding this comment.
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)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 777fc61
Thanks @mpaolini for the PR, and @serhiy-storchaka for merging it |
GH-17166 is a backport of this pull request to the 3.8 branch. |
Thanks @mpaolini! |
https://bugs.python.org/issue38677