Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-134119: Fix crash from calling next() on exhausted template iterator#134120
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Misc/NEWS.d/next/Core_and_Builtins/2025-05-16-20-59-12.gh-issue-134119.w8expI.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -23,6 +23,9 @@ templateiter_next(PyObject *op) | |||
| if (self->from_strings) { | |||
| item = PyIter_Next(self->stringsiter); | |||
| self->from_strings = 0; | |||
| if (item == NULL) { | |||
| return NULL; | |||
| } | |||
| if (PyUnicode_GET_LENGTH(item) == 0) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe it is worth to add condition here like
if (!item || PyUnicode_GET_LENGTH(item) == 0) { Py_XSETREF(item, PyItem_Next(self->interpolationsiter)); self->from_strings = 1;}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think that would potentially make an API call with an exception set, which is not allowed, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, good point. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good!
| @@ -23,6 +23,9 @@ templateiter_next(PyObject *op) | |||
| if (self->from_strings) { | |||
| item = PyIter_Next(self->stringsiter); | |||
| self->from_strings = 0; | |||
| if (item == NULL) { | |||
| return NULL; | |||
| } | |||
| if (PyUnicode_GET_LENGTH(item) == 0) { | |||
| Py_SETREF(item, PyIter_Next(self->interpolationsiter)); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should the result ofPyIter_Next(self->interpolationsiter) be checked forNULL too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That case just falls through to returningNULL safely, so it's probably fine as-is.
(The fall-through is intentional: since there is always one morestring thaninterpolation, the firstNULL returned by the iterator will always be here.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Py_SETREF is safe if it assigns to NULL, the behavior should be correct in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good to me as soon as@picnixz's feedback has been addressed.
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
fc7f4c3 intopython:mainUh oh!
There was an error while loading.Please reload this page.
Thanks@JelleZijlstra for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14. |
…iterator (pythonGH-134120)(cherry picked from commitfc7f4c3)Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
GH-134153 is a backport of this pull request to the3.14 branch. |
Uh oh!
There was an error while loading.Please reload this page.