Skip to content

bpo-41336: Fix the error handling in zoneinfo_new_instance() #21546

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

Merged
merged 1 commit into from
Jul 20, 2020
Merged

bpo-41336: Fix the error handling in zoneinfo_new_instance() #21546

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion Modules/_zoneinfo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,8 +224,14 @@ zoneinfo_new_instance(PyTypeObject *type, PyObject *key)
self = NULL;
cleanup:
if (file_obj != NULL) {
PyObject *exc, *val, *tb;
PyErr_Fetch(&exc, &val, &tb);
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of this?

Is the issue that there might be a live exception and that file_obj.close() may itself raise an exception, so we want to save the original one and chain it to the new one?

PyObject *tmp = PyObject_CallMethod(file_obj, "close", NULL);
Py_DECREF(tmp);
_PyErr_ChainExceptions(exc, val, tb);
if (tmp == NULL) {
Py_CLEAR(self);
Copy link
Member

Choose a reason for hiding this comment

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

Hm... this feels a little weird but I suppose it's acceptable.

At this point we've actually already succeeded in reading and constructing a ZoneInfo in self; if tmp is NULL that indicates that we've failed to close a file that we've opened for some reason, but we could still return self. In what situation would it matter that we failed to close the file handle? I would expect that failure to close the file handle would mean that it's already closed or something has happened to the file descriptor at the OS level — some sort of conflict around ownership of the resource that we could easily cede to whatever other process has taken ownership of it.

Is the idea here that the Ctrl + C here is manifesting as an error in the method call and will only bubble up the stack if we affirmatively raise it here? If so, might it make sense to clear anything derived from Exception and bubble up only errors here derived from BaseException but not Exception?

}
Py_XDECREF(tmp);
Py_DECREF(file_obj);
}
Py_DECREF(file_path);
Expand Down