bpo-29854: Fix segfault in call_readline() by nirs · Pull Request #728 · python/cpython (original) (raw)
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 our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation43 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
If history-length is set in .inputrc, and the history file is double the
history size (or more), history_get(N) returns NULL, and python
segfaults. Fix that by checking for NULL return value.
It seems that the root cause is incorrect handling of bigger history in
readline, but python should not segfault even if readline returns
unexpected value.
Hello, and thanks for your contribution!
I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).
Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:
- If you don't have an account on b.p.o, please create one
- Make sure your GitHub username is listed in "Your Details" at b.p.o
- If you have not already done so, please sign the PSF contributor agreement. The "bugs.python.org username " requested by the form is the "Login name" field under "Your Details".
- If you just signed the CLA, please wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
- Reply here saying you have completed the above steps
Thanks again to your contribution and we look forward to looking at it!
bpo-29854: Fix segtault in call_readline() bpo-29854: Fix segfault in call_readline()
I signed the CLA few years ago, but my github user name was missing in bpo.
| line = (const char *)history_get(length)->line; |
|---|
| else |
| hist_ent = history_get(length); |
| line = (const char *)hist_ent ? hist_ent->line : ""; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you cast hist_ent? The original cast was added in Git revision 2525dc8, though I have doubts about the reason [avoiding a compiler warning when assigning (char *) to (const char *)]. It is better to avoid casts if possible; they can mask real errors and warnings.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cast is not needed. A compiler shouldn't emit a warning when assigning char * to const char *.
And more, I have doubts about the priority of the cast and the trinary operator. Does the above code is equivalent to (const char *)(hist_ent ? hist_ent->line : "") or to ((const char *)hist_ent) ? hist_ent->line : ""?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the correct cast would be:
hist_ent ? (const char *)hist_ent->line : "";
But the cast is probably not needed, I kept it only to minimize changes which are not required to fix this issue.
| else |
|---|
| hist_ent = history_get(length); |
| line = (const char *)hist_ent ? hist_ent->line : ""; |
| } else |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} and else should be on separate lines.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to keep the current code style of this module, see for example write_history_file, read_history_file, read_init_file, setup_readline.
Changes in version 2:
- Add failing test (serhiy-storchaka)
- Remove unneeded cast in readline.c (vadmium)
Version 3 adds the missing NEWS entry.
| import readline |
|---|
| import sys |
| history_file = "{}" |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be safer to use ascii or {!a} (repr or {!r} on Python 2), in case the path has special characters; e.g. double quotes are allowed on Unix, Windows user profiles may be non-ASCII. Or you could change directory to the temp directory in the child process, or pass it as a CLI argument.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path is a temporary path, so it should not have quotes. I think a better way would be to use command line arguments or environment variables instead.
| script = """ |
| import readline |
| import sys |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover, will remove.
| inputrc = os.path.join(temp_dir, "inputrc") |
|---|
| with io.open(inputrc, "wb") as f: |
| f.write(b"set history-size %d\n" % history_size) |
| env = os.environ.copy() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be safer and clearer to make the copy with env = dict(os.environ). Os.environ.copy is undocumented.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, will update in the next version.
| self.assertEqual(len(lines), history_size) |
|---|
| self.assertEqual(lines[-1].strip(), b"last input") |
| finally: |
| shutil.rmtree(temp_dir) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use self.addCleanup(shutil.rmtree, temp_dir) instead of this try..finally block?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or use test.support.temp_dir() or test.support.temp_cwd().
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, will change.
| temp_dir = tempfile.mkdtemp() |
|---|
| try: |
| inputrc = os.path.join(temp_dir, "inputrc") |
| with io.open(inputrc, "wb") as f: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you use io.open() to make backporting to 2.7 easier?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is a significant difference between Python 2 builtin open() and io.open() here.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, code that works in both python 3 and 2 is my intent.
| readline.read_history_file(history_file) |
|---|
| input() |
| readline.write_history_file(history_file) |
| """.format(history_file) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: I'd rather use an f-string here.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f-string does not work on python 2, creating extra work :-)
| @@ -322,6 +322,9 @@ Core and Builtins |
|---|
| Extension Modules |
| ----------------- |
| - bpo-29854: Fix segfault in readline when using readline's history-size |
| option. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add "Patch by Nir Soffer." (and add two spaces after the full stop)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
Changes in version 4:
- Avoid undocumented os.environ.copy (vadmium)
- Remove unneeded import (vadmium)
- Pass configuraion to child process using environment variables instead of formatting (vadmium, berkerpeksag)
- Use addCleanup instead of try-finally (berkerpeksag)
- Adding missing "Patch by" to news entry (berkerpeksag)
Version 4 is ready for 25 days, it would be nice if someone can take a look :-)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last version of the PR looks good to me, thank you!
I still prefer to use open() instead of io.open() (we are likely get a merge conflict in Misc/NEWS file when we backport it to 2.7 so we will need to do manual edit anyway and I don't think 2.7 is important to write slightly less idiomatic code in Python 3), but I'm going to left the ultimate decision to Martin and Serhiy.
LGTM if use builtin open().
Changes in version 5:
- Use builtin open instead of io.open (io.open is required for backporting to 2.7).
| temp_dir = tempfile.mkdtemp() |
|---|
| try: |
| inputrc = os.path.join(temp_dir, "inputrc") |
| with io.open(inputrc, "wb") as f: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is a significant difference between Python 2 builtin open() and io.open() here.
| try: |
|---|
| inputrc = os.path.join(temp_dir, "inputrc") |
| with io.open(inputrc, "wb") as f: |
| f.write(b"set history-size %d\n" % history_size) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the first time I see using bytes formatting in tests! Since bytes formatting is supported in Python 3.5 this is okay.
| self.assertEqual(len(lines), history_size) |
|---|
| self.assertEqual(lines[-1].strip(), b"last input") |
| finally: |
| shutil.rmtree(temp_dir) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or use test.support.temp_dir() or test.support.temp_cwd().
@serhiy-storchaka, I saw your comment about using test.support.temp_dir after I uploaded version 5.
See nirs@45931f5 using it. I can squash it into the tests patch or send another pull request later if you like.
@nirs, sorry, I added my comments few days ago, but forgot to publish them. When I approved the PR, they became visible.
I still think that it would be better to use test.support.temp_dir() (it is more robust for testing on Windows), but this is not critical, and I approved the PR in any case.
I think we need to move the NEWS entry into Misc/NEWS.d/next/Library now. You can use blurb to create a NEWS file.
I liked nirs@45931f5. Could you integrate it into this PR?
I can do the merging if Serhiy is busy with other PRs. Thanks! (and sorry for taking too long to merge this.)
@serhiy-storchaka are you waiting for another change?
No, I just waited for other reviewers in the case they have other comments. I left this on Berker.
This enable testing custom readline configuration using the INPUTRC environment variable, or passing arguments to the child process in a clean way.
readline segfaults on input() if the number of items in the history file is equal or more to history size * 2.
This issue affects only GNU readline. When using libedit emulation system history size option does not work.
If history-length is set in .inputrc, and the history file is double the history size (or more), history_get(N) returns NULL, and python segfaults. Fix that by checking for NULL return value.
It seems that the root cause is incorrect handling of bigger history in readline, but python should not segfault even if readline returns unexpected value.
nirs added a commit to nirs/cpython that referenced this pull request
If history-length is set in .inputrc, and the history file is double the history size (or more), history_get(N) returns NULL, and python segfaults. Fix that by checking for NULL return value.
It seems that the root cause is incorrect handling of bigger history in readline, but Python should not segfault even if readline returns unexpected value.
This issue affects only GNU readline. When using libedit emulation system history size option does not work.
berkerpeksag pushed a commit that referenced this pull request
If history-length is set in .inputrc, and the history file is double the history size (or more), history_get(N) returns NULL, and python segfaults. Fix that by checking for NULL return value.
It seems that the root cause is incorrect handling of bigger history in readline, but Python should not segfault even if readline returns unexpected value.
This issue affects only GNU readline. When using libedit emulation system history size option does not work.
nirs added a commit to nirs/cpython that referenced this pull request
If history-length is set in .inputrc, and the history file is double the history size (or more), history_get(N) returns NULL, and python segfaults. Fix that by checking for NULL return value.
It seems that the root cause is incorrect handling of bigger history in readline, but Python should not segfault even if readline returns unexpected value.
This issue affects only GNU readline. When using libedit emulation system history size option does not work.
nirs added a commit to nirs/cpython that referenced this pull request
If history-length is set in .inputrc, and the history file is double the history size (or more), history_get(N) returns NULL, and python segfaults. Fix that by checking for NULL return value.
It seems that the root cause is incorrect handling of bigger history in readline, but Python should not segfault even if readline returns unexpected value.
This issue affects only GNU readline. When using libedit emulation system history size option does not work.
This is a backport of the actual fix from master without the test, since the test depends on new run_pty() helper which is not available in 2.7.
berkerpeksag pushed a commit that referenced this pull request
If history-length is set in .inputrc, and the history file is double the history size (or more), history_get(N) returns NULL, and python segfaults. Fix that by checking for NULL return value.
It seems that the root cause is incorrect handling of bigger history in readline, but Python should not segfault even if readline returns unexpected value.
This issue affects only GNU readline. When using libedit emulation system history size option does not work.
berkerpeksag pushed a commit that referenced this pull request
If history-length is set in .inputrc, and the history file is double the history size (or more), history_get(N) returns NULL, and python segfaults. Fix that by checking for NULL return value.
It seems that the root cause is incorrect handling of bigger history in readline, but Python should not segfault even if readline returns unexpected value.
This issue affects only GNU readline. When using libedit emulation system history size option does not work.
This is a backport of the actual fix from master without the test, since the test depends on new run_pty() helper which is not available in 2.7.
Labels
An unexpected behavior, bug, or error