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

gh-128198: Add missing error checks for usages of PyIter_Next #128199

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

gh-128198: Add missing error checks for usages of PyIter_Next #128199

wants to merge 4 commits into from
+30 −3

Conversation

WolframAlph
Copy link
Contributor

@WolframAlph WolframAlph commented Dec 23, 2024 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Dec 23, 2024
Open
@WolframAlph WolframAlph force-pushed the WolframAlph/fix-pyiternext branch from 1c58e95 to ca54407 Compare December 23, 2024 16:32
@WolframAlph WolframAlph force-pushed the WolframAlph/fix-pyiternext branch from ca54407 to c6d319f Compare December 23, 2024 16:33
@rhettinger rhettinger removed their request for review December 23, 2024 18:01
ZeroIntensity
ZeroIntensity reviewed Dec 23, 2024
View reviewed changes
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

In 3.14, we have a new PyIter_NextItem for exactly this problem. I think it's worth migrating to that in this PR instead of trying to adapt to the bad design choices of PyIter_Next.

@WolframAlph
Copy link
Contributor Author

👍 Will check it out and update the PR

@WolframAlph
Copy link
Contributor Author

@ZeroIntensity I can see PyIter_NextItem being used only in couple places. Should I rename the issue to indicate migration from old PyIter_Next to new PyIter_NextItem api and work on replacing old one in the entire codebase?

@ZeroIntensity
Copy link
Member

I don't think we need to replace PyIter_Next throughout the entire codebase (though it is probably inevitable), just change it for the cases that are problematic.

Should I rename the issue to indicate migration from old PyIter_Next

Nah, just update the PR title.

@WolframAlph
Copy link
Contributor Author

WolframAlph commented Dec 24, 2024 •
edited
Loading

@ZeroIntensity so if this is inevitable (otherwise why would new PyIter_NextItem be implemented) and I can see ~63 instances of PyIter_Next usage in the codebase, I would like to spend time on migrating to the new version in this PR. Or at least those instances that do not require significant rewrite and don't introduce more complexity. Any objections from your side? Or if this is still too much for a single PR, maybe create top level issue to track migration and add separate issues for migration in each file where usages occur?

picnixz
picnixz requested changes Dec 24, 2024
View reviewed changes
Modules/_collectionsmodule.c
@@ -1667,10 +1667,13 @@ deque_richcompare(PyObject *v, PyObject *w, int op)
if (it2 == NULL)
goto done;
for (;;) {
x = PyIter_Next(it1);
if (x == NULL && PyErr_Occurred())
if ((x = PyIter_Next(it1)) == NULL && PyErr_Occurred()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is equivalent to what was before. Please revert.

Modules/_collectionsmodule.c
y = PyIter_Next(it2);
}
if ((y = PyIter_Next(it2)) == NULL && PyErr_Occurred()) {
Py_XDECREF(x);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is already handled by lines 1677 + 1689 + 1691. PLease revert.

Modules/_asynciomodule.c

/* Check if loop ended because of exception in PyIter_Next */
if (PyErr_Occurred()) {
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.

You need to decref the tasks as well.

@picnixz picnixz changed the title gh-128198: Add missing error checks for PyIter_Next gh-128198: Add missing error checks for usages of PyIter_Next Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZeroIntensity ZeroIntensity ZeroIntensity left review comments

@picnixz picnixz picnixz requested changes

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner

@willingc willingc Awaiting requested review from willingc willingc is a code owner

Assignees
No one assigned
Labels
awaiting core review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@WolframAlph @ZeroIntensity @picnixz