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.

Sign up for GitHub

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

Jump to bottom

Handle failure from PyDict_New or PyList_New #6999

Merged
merged 8 commits into from
Mar 31, 2023
Merged

Handle failure from PyDict_New or PyList_New #6999

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
929dbba
Handle failure from PyDict_New or PyList_New
radarhere Mar 10, 2023
b3d7823
Decrement reference count
radarhere Mar 11, 2023
c7d4d1f
Fixed typo
radarhere Mar 18, 2023
dfeed0e
Group decrementing reference counts for previous axes in font_getvaraxes
radarhere Mar 20, 2023
dc79a09
Merge branch 'main' into null
radarhere Mar 22, 2023
0ea1184
Free additional variables
radarhere Mar 29, 2023
7632d8d
Do not DECREF individual list items, reverting grouping
radarhere Mar 30, 2023
448ab0a
Call FT_Done_MM_Var when returning early
radarhere Mar 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/_imaging.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1249,6 +1249,9 @@ _histogram(ImagingObject *self, PyObject *args) {

/* Build an integer list containing the histogram */
list = PyList_New(h->bands * 256);
if (list == NULL) {
return NULL;
radarhere marked this conversation as resolved.
Show resolved Hide resolved
}
for (i = 0; i < h->bands * 256; i++) {
PyObject *item;
item = PyLong_FromLong(h->histogram[i]);
Expand Down Expand Up @@ -2154,6 +2157,9 @@ _getcolors(ImagingObject *self, PyObject *args) {
Py_INCREF(out);
} else {
out = PyList_New(colors);
if (out == NULL) {
return NULL;
radarhere marked this conversation as resolved.
Show resolved Hide resolved
}
for (i = 0; i < colors; i++) {
ImagingColorItem *v = &items[i];
PyObject *item = Py_BuildValue(
Expand Down Expand Up @@ -4325,6 +4331,7 @@ PyInit__imaging(void) {
m = PyModule_Create(&module_def);

if (setup_module(m) < 0) {
Py_DECREF(m);
return NULL;
}

Expand Down
62 changes: 43 additions & 19 deletions src/_imagingft.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1082,11 +1082,15 @@ font_getvarnames(FontObject *self) {

num_namedstyles = master->num_namedstyles;
list_names = PyList_New(num_namedstyles);
if (list_names == NULL) {
return NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

geterror, used for error handling elsewhere here, calls PyErr_SetString before returning NULL; maybe these should have something too instead of just returning NULL..? (Repeated for other bare return NULL;s in this PR.)

Copy link
Member Author

Choose a reason for hiding this comment

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

My thinking was to copy behaviour is already used in Pillow in another location.

Pillow/src/_imaging.c

Lines 3819 to 3822 in 2b16494

d = PyDict_New();
if (!d) {
return NULL;
}

Looking at CPython, I think that PyList_New already sets an error before returning. The same with PyDict_New.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, great, so it seems 👍

(I'd personally probably add a comment that says something like // Error set by PyDict_New on the return line, but that's fine.)

radarhere marked this conversation as resolved.
Show resolved Hide resolved
}

name_count = FT_Get_Sfnt_Name_Count(self->face);
for (i = 0; i < name_count; i++) {
error = FT_Get_Sfnt_Name(self->face, i, &name);
if (error) {
Py_DECREF(list_names);
radarhere marked this conversation as resolved.
Show resolved Hide resolved
return geterror(error);
}

Expand All @@ -1110,7 +1114,7 @@ font_getvarnames(FontObject *self) {

static PyObject *
font_getvaraxes(FontObject *self) {
int error;
int error, failed = 0;
FT_UInt i, j, num_axis, name_count;
FT_MM_Var *master;
FT_Var_Axis axis;
Expand All @@ -1125,34 +1129,54 @@ font_getvaraxes(FontObject *self) {
name_count = FT_Get_Sfnt_Name_Count(self->face);

list_axes = PyList_New(num_axis);
if (list_axes == NULL) {
return NULL;
radarhere marked this conversation as resolved.
Show resolved Hide resolved
}
for (i = 0; i < num_axis; i++) {
axis = master->axis[i];

list_axis = PyDict_New();
PyObject *minimum = PyLong_FromLong(axis.minimum / 65536);
PyDict_SetItemString(list_axis, "minimum", minimum ? minimum : Py_None);
Py_XDECREF(minimum);
if (list_axis == NULL) {
failed = 1;
} else {
PyObject *minimum = PyLong_FromLong(axis.minimum / 65536);
PyDict_SetItemString(list_axis, "minimum", minimum ? minimum : Py_None);
Py_XDECREF(minimum);

PyObject *def = PyLong_FromLong(axis.def / 65536);
PyDict_SetItemString(list_axis, "default", def ? def : Py_None);
Py_XDECREF(def);

PyObject *def = PyLong_FromLong(axis.def / 65536);
PyDict_SetItemString(list_axis, "default", def ? def : Py_None);
Py_XDECREF(def);
PyObject *maximum = PyLong_FromLong(axis.maximum / 65536);
PyDict_SetItemString(list_axis, "maximum", maximum ? maximum : Py_None);
Py_XDECREF(maximum);

PyObject *maximum = PyLong_FromLong(axis.maximum / 65536);
PyDict_SetItemString(list_axis, "maximum", maximum ? maximum : Py_None);
Py_XDECREF(maximum);
for (j = 0; j < name_count; j++) {
error = FT_Get_Sfnt_Name(self->face, j, &name);
if (error) {
Py_DECREF(list_axis);
failed = 1;
break;
}

for (j = 0; j < name_count; j++) {
error = FT_Get_Sfnt_Name(self->face, j, &name);
if (name.name_id == axis.strid) {
axis_name = Py_BuildValue("y#", name.string, name.string_len);
PyDict_SetItemString(list_axis, "name", axis_name ? axis_name : Py_None);
Py_XDECREF(axis_name);
break;
}
}
}
if (failed) {
for (j = 0; j < i; j++) {
list_axis = PyList_GetItem(list_axes, j);
Py_DECREF(list_axis);
}
Py_DECREF(list_axes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like this duplicated cleaning code should be deduplicated, maybe..?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I've pushed a commit to rearrange the code.

Copy link
Member

Choose a reason for hiding this comment

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

@akx How's it look now?

Copy link
Contributor

Choose a reason for hiding this comment

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

This loop looks suspicious to me. PyList_GetItem returns a borrowed reference, so the decref will cause it to be freed. However, the list is expected to also free its contents when it's reference count is decrefed later: https://github.com/python/cpython/blob/e375bff03736f809fbc234010c087ef9d7e0d384/Objects/listobject.c#L356

So I would expect this loop to cause each list_axis to be double freed (except the last, which has not been added to the list yet).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I've applied the suggestion from @nulano, which means that the loop was de-duplicated is no longer present. So I've actually undone the rearranging of the code as it is no longer necessary.

if (error) {
return geterror(error);
}

if (name.name_id == axis.strid) {
axis_name = Py_BuildValue("y#", name.string, name.string_len);
PyDict_SetItemString(list_axis, "name", axis_name ? axis_name : Py_None);
Py_XDECREF(axis_name);
break;
}
return NULL;
}

PyList_SetItem(list_axes, i, list_axis);
Expand Down
12 changes: 11 additions & 1 deletion src/_imagingmorph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,20 +136,26 @@ match(PyObject *self, PyObject *args) {
int row_idx, col_idx;
UINT8 **inrows;
PyObject *ret = PyList_New(0);
if (ret == NULL) {
return NULL;
}

if (!PyArg_ParseTuple(args, "On", &py_lut, &i0)) {
Py_DECREF(ret);
PyErr_SetString(PyExc_RuntimeError, "Argument parsing problem");
return NULL;
}

if (!PyBytes_Check(py_lut)) {
Py_DECREF(ret);
PyErr_SetString(PyExc_RuntimeError, "The morphology LUT is not a bytes object");
return NULL;
}

lut_len = PyBytes_Size(py_lut);

if (lut_len < LUT_SIZE) {
Py_DECREF(ret);
PyErr_SetString(PyExc_RuntimeError, "The morphology LUT has the wrong size");
return NULL;
}
Expand All @@ -158,6 +164,7 @@ match(PyObject *self, PyObject *args) {
imgin = (Imaging)i0;

if (imgin->type != IMAGING_TYPE_UINT8 || imgin->bands != 1) {
Py_DECREF(ret);
PyErr_SetString(PyExc_RuntimeError, "Unsupported image type");
return NULL;
}
Expand Down Expand Up @@ -214,10 +221,13 @@ get_on_pixels(PyObject *self, PyObject *args) {
int row_idx, col_idx;
int width, height;
PyObject *ret = PyList_New(0);
if (ret == NULL) {
return NULL;
}

if (!PyArg_ParseTuple(args, "n", &i0)) {
Py_DECREF(ret);
PyErr_SetString(PyExc_RuntimeError, "Argument parsing problem");

return NULL;
}
img = (Imaging)i0;
Expand Down
6 changes: 5 additions & 1 deletion src/_imagingtk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,5 +58,9 @@ PyInit__imagingtk(void) {
};
PyObject *m;
m = PyModule_Create(&module_def);
return (load_tkinter_funcs() == 0) ? m : NULL;
if (load_tkinter_funcs() != 0) {
Py_DECREF(m);
return NULL;
}
return m;
}
1 change: 1 addition & 0 deletions src/_webp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -990,6 +990,7 @@ PyInit__webp(void) {

m = PyModule_Create(&module_def);
if (setup_module(m) < 0) {
Py_DECREF(m);
return NULL;
}

Expand Down
6 changes: 6 additions & 0 deletions src/path.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,9 @@ path_tolist(PyPathObject *self, PyObject *args) {

if (flat) {
list = PyList_New(self->count * 2);
if (list == NULL) {
return NULL;
}
for (i = 0; i < self->count * 2; i++) {
PyObject *item;
item = PyFloat_FromDouble(self->xy[i]);
Expand All @@ -449,6 +452,9 @@ path_tolist(PyPathObject *self, PyObject *args) {
}
} else {
list = PyList_New(self->count);
if (list == NULL) {
return NULL;
}
for (i = 0; i < self->count; i++) {
PyObject *item;
item = Py_BuildValue("dd", self->xy[i + i], self->xy[i + i + 1]);
Expand Down