Skip to content

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

Merged
serhiy-storchaka merged 1 commit into
python:masterfrom
ZackerySpytz:bpo-41336-zoneinfo-exception-handling
Jul 20, 2020
Merged

bpo-41336: Fix the error handling in zoneinfo_new_instance()#21546
serhiy-storchaka merged 1 commit into
python:masterfrom
ZackerySpytz:bpo-41336-zoneinfo-exception-handling

Conversation

@ZackerySpytz

@ZackerySpytz ZackerySpytz commented Jul 19, 2020

Copy link
Copy Markdown
Contributor

Do not call PyObject_CallMethod() with a live exception (like
KeyboardInterrupt).

https://bugs.python.org/issue41336

@ZackerySpytz

Copy link
Copy Markdown
Contributor Author

I believe this is a "skip news" PR.

@tirkarthi
tirkarthi requested a review from pganssle July 19, 2020 12:03
@ZackerySpytz
ZackerySpytz force-pushed the bpo-41336-zoneinfo-exception-handling branch from 054080e to 441c715 Compare July 19, 2020 14:20
Comment thread Modules/_zoneinfo.c Outdated
Do not call PyObject_CallMethod() with a live exception (like
KeyboardInterrupt).
@ZackerySpytz
ZackerySpytz force-pushed the bpo-41336-zoneinfo-exception-handling branch from 441c715 to dd39080 Compare July 20, 2020 10:45

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@serhiy-storchaka serhiy-storchaka added needs backport to 3.9 type-bug An unexpected behavior, bug, or error labels Jul 20, 2020
@serhiy-storchaka
serhiy-storchaka merged commit eca2549 into python:master Jul 20, 2020
@miss-islington

Copy link
Copy Markdown
Contributor

Thanks @ZackerySpytz for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 20, 2020
…H-21546)

Do not call PyObject_CallMethod() with a live exception (like
KeyboardInterrupt).
(cherry picked from commit eca2549)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
@bedevere-bot

Copy link
Copy Markdown

GH-21563 is a backport of this pull request to the 3.9 branch.

miss-islington added a commit that referenced this pull request Jul 20, 2020
Do not call PyObject_CallMethod() with a live exception (like
KeyboardInterrupt).
(cherry picked from commit eca2549)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
Comment thread Modules/_zoneinfo.c
cleanup:
if (file_obj != NULL) {
PyObject *exc, *val, *tb;
PyErr_Fetch(&exc, &val, &tb);

Copy link
Copy Markdown
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?

Comment thread Modules/_zoneinfo.c
Py_DECREF(tmp);
_PyErr_ChainExceptions(exc, val, tb);
if (tmp == NULL) {
Py_CLEAR(self);

Copy link
Copy Markdown
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?

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Aug 4, 2020
…H-21546)

Do not call PyObject_CallMethod() with a live exception (like
KeyboardInterrupt).
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Aug 20, 2020
…H-21546)

Do not call PyObject_CallMethod() with a live exception (like
KeyboardInterrupt).
xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
…H-21546)

Do not call PyObject_CallMethod() with a live exception (like
KeyboardInterrupt).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip news type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants