Handle failure from PyDict_New or PyList_New #6999
Conversation
| @@ -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; | |||
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
My thinking was to copy behaviour is already used in Pillow in another location.
Lines 3819 to 3822 in 2b16494
Looking at CPython, I think that PyList_New already sets an error before returning. The same with PyDict_New.
There was a problem hiding this comment.
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.)
src/_imagingft.c
Outdated
| for (j = 0; j < i; j++) { | ||
| list_axis = PyList_GetItem(list_axes, j); | ||
| Py_DECREF(list_axis); | ||
| } | ||
| Py_DECREF(list_axes); |
There was a problem hiding this comment.
Feels like this duplicated cleaning code should be deduplicated, maybe..?
There was a problem hiding this comment.
Ok, I've pushed a commit to rearrange the code.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
Co-authored-by: Aarni Koskela <akx@iki.fi>
src/_imagingft.c
Outdated
| for (j = 0; j < i; j++) { | ||
| list_axis = PyList_GetItem(list_axes, j); | ||
| Py_DECREF(list_axis); | ||
| } | ||
| Py_DECREF(list_axes); |
src/_imagingft.c
Outdated
| for (j = 0; j < i; j++) { | ||
| list_axis = PyList_GetItem(list_axes, j); | ||
| Py_DECREF(list_axis); | ||
| } | ||
| Py_DECREF(list_axes); |
There was a problem hiding this comment.
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).
Co-authored-by: Ondrej Baranovič <3819630+nulano@users.noreply.github.com>
Co-authored-by: Ondrej Baranovič <3819630+nulano@users.noreply.github.com>
Two changes here.
PyDict_New()andPyList_New()may returnNULL. This addresses situations where that is not handled.