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

Handle failure from PyDict_New or PyList_New #6999

Merged
merged 8 commits into from
Mar 31, 2023
Merged

Conversation

radarhere
Copy link
Member

@radarhere radarhere commented Mar 10, 2023

Two changes here.

  1. PyDict_New() and PyList_New() may return NULL. This addresses situations where that is not handled.
  2. Resolves Memory leaks when function returns directly on failure without decreasing the refcnt of a new reference (static analyzer reports) #6322, by decrementing reference counts. The first change adds another situation where this is needed, so these are in the same PR.

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

src/_imagingft.c Outdated
Comment on lines 1140 to 1144
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.

src/_imagingtk.c Outdated Show resolved Hide resolved
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Some of these new return NULLs aren't covered by tests. I guess it's not very easy to trigger them, and that's fine as defensive programming in this case.

src/_imagingft.c Outdated
Comment on lines 1140 to 1144
for (j = 0; j < i; j++) {
list_axis = PyList_GetItem(list_axes, j);
Py_DECREF(list_axis);
}
Py_DECREF(list_axes);
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?

src/_imaging.c Show resolved Hide resolved
src/_imaging.c Show resolved Hide resolved
src/_imagingft.c Outdated
Comment on lines 1140 to 1144
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.

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

src/_imagingft.c Show resolved Hide resolved
src/_imagingft.c Show resolved Hide resolved
src/_imagingft.c Show resolved Hide resolved
src/_imagingft.c Show resolved Hide resolved
src/_imagingft.c Show resolved Hide resolved
@hugovk hugovk merged commit f191666 into python-pillow:main Mar 31, 2023
@radarhere radarhere deleted the null branch March 31, 2023 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants