GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||
GeezerVison is showing github.com — gh-151126: Check `strdup` result in readline `on_completion` by KowalskiThomas · Pull Request #156299 · python/cpython Conversation
| |||||||||||||||||||||||
| @@ -1272,6 +1272,10 @@ on_completion(const char *text, int state) | |||
| goto error; | |||
| result = strdup(PyBytes_AS_STRING(encoded)); | |||
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 this should actually change to _PyMem_Strdup while we're here
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
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.
Will do depending on whether we just close the PR or not!
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| result = strdup(PyBytes_AS_STRING(encoded)); | ||
| Py_DECREF(encoded); | ||
| if (result == NULL) { | ||
| PyErr_NoMemory(); |
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 don't think this works right as the goto err will clear the memory error set
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
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.
🤔 You're right, indeed.
Looking a bit more at it, I'm starting to wonder if the lack of checking strdup result is an oversight or on purpose, since the function ignores all errors that can happen and doesn't seem to ever return / raise them.
So I'm not quite sure what I should do here. It seems like actually bubbling the exception all the way up to input would require "setting the exception" somewhere, so that the actual "reading loop" can detect it and abort/actually raise the exception.
Alternatively, it might make sense to just explicitly ignore the result of strdup (since at this point it's likely the user impact is none or near none), and if strdup it's also likely things will blow up not much later...
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
|
Seems like this really is an unlikely edge case and there's no straightforward fix to it (see this comment) so I will just close the PR. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
2 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.