bpo-29854: Fix segfault in call_readline() #728

nirs · 2017-03-19T22:27:35Z

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.

the-knights-who-say-ni · 2017-03-19T22:27:36Z

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!

mention-bot · 2017-03-19T22:27:37Z

@nirs, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vadmium, @loewis and @ronaldoussoren to be potential reviewers.

nirs · 2017-03-19T22:33:04Z

I signed the CLA few years ago, but my github user name was missing in bpo.

vadmium · 2017-03-23T10:45:23Z

Modules/readline.c

-            line = (const char *)history_get(length)->line;
-        else
+                hist_ent = history_get(length);
+            line = (const char *)hist_ent ? hist_ent->line : "";


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.

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 : ""?

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.

serhiy-storchaka · 2017-03-23T11:16:17Z

Modules/readline.c

-        else
+                hist_ent = history_get(length);
+            line = (const char *)hist_ent ? hist_ent->line : "";
+        } else


} and else should be on separate lines.

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.

nirs · 2017-05-19T20:21:27Z

Changes in version 2:

Add failing test (serhiy-storchaka)
Remove unneeded cast in readline.c (vadmium)

nirs · 2017-05-19T22:32:52Z

Version 3 adds the missing NEWS entry.

vadmium · 2017-05-20T00:59:57Z

Lib/test/test_readline.py

+import readline
+import sys
+
+history_file = "{}"


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.

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.

vadmium · 2017-05-20T01:02:03Z

Lib/test/test_readline.py

+
+            script = """
+import readline
+import sys


Is this needed?

Leftover, will remove.

vadmium · 2017-05-20T01:16:41Z

Lib/test/test_readline.py

+            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()


I think it would be safer and clearer to make the copy with env = dict(os.environ). Os.environ.copy is undocumented.

I agree, will update in the next version.

berkerpeksag · 2017-05-20T13:08:06Z

Lib/test/test_readline.py

+            self.assertEqual(len(lines), history_size)
+            self.assertEqual(lines[-1].strip(), b"last input")
+        finally:
+            shutil.rmtree(temp_dir)


Can't we use self.addCleanup(shutil.rmtree, temp_dir) instead of this try..finally block?

Or use test.support.temp_dir() or test.support.temp_cwd().

Good idea, will change.

berkerpeksag · 2017-05-20T13:09:40Z

Lib/test/test_readline.py

+        temp_dir = tempfile.mkdtemp()
+        try:
+            inputrc = os.path.join(temp_dir, "inputrc")
+            with io.open(inputrc, "wb") as f:


Did you use io.open() to make backporting to 2.7 easier?

I don't think there is a significant difference between Python 2 builtin open() and io.open() here.

Yes, code that works in both python 3 and 2 is my intent.

berkerpeksag · 2017-05-20T13:10:37Z

Lib/test/test_readline.py

+readline.read_history_file(history_file)
+input()
+readline.write_history_file(history_file)
+""".format(history_file)


Style nit: I'd rather use an f-string here.

f-string does not work on python 2, creating extra work :-)

berkerpeksag · 2017-05-20T13:14:20Z

Misc/NEWS

@@ -322,6 +322,9 @@ Core and Builtins
 Extension Modules
 -----------------

+- bpo-29854: Fix segfault in readline when using readline's history-size
+  option.


Please add "Patch by Nir Soffer." (and add two spaces after the full stop)

nirs · 2017-05-21T15:38:42Z

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)

nirs · 2017-06-15T19:21:36Z

Version 4 is ready for 25 days, it would be nice if someone can take a look :-)

berkerpeksag

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.

serhiy-storchaka · 2017-06-16T05:54:51Z

LGTM if use builtin open().

nirs · 2017-06-16T18:43:12Z

Changes in version 5:

Use builtin open instead of io.open (io.open is required for backporting to 2.7).

serhiy-storchaka · 2017-05-20T13:22:44Z

Lib/test/test_readline.py

+        temp_dir = tempfile.mkdtemp()
+        try:
+            inputrc = os.path.join(temp_dir, "inputrc")
+            with io.open(inputrc, "wb") as f:


I don't think there is a significant difference between Python 2 builtin open() and io.open() here.

serhiy-storchaka · 2017-05-20T13:49:42Z

Lib/test/test_readline.py

+        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)


This is the first time I see using bytes formatting in tests! Since bytes formatting is supported in Python 3.5 this is okay.

serhiy-storchaka · 2017-05-20T13:55:33Z

Lib/test/test_readline.py

+            self.assertEqual(len(lines), history_size)
+            self.assertEqual(lines[-1].strip(), b"last input")
+        finally:
+            shutil.rmtree(temp_dir)


Or use test.support.temp_dir() or test.support.temp_cwd().

nirs · 2017-06-16T21:23:26Z

@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.

serhiy-storchaka · 2017-06-17T04:42:56Z

@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.

nirs · 2017-07-02T15:14:17Z

@serhiy-storchaka are you waiting for another change?

berkerpeksag · 2017-07-03T04:56:55Z

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 · 2017-07-03T05:14:19Z

@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.

nirs · 2017-07-04T09:31:21Z

@berkerpeksag, ok, I will squash nirs/cpython@45931f5 into the test patch, and update the news item as you suggest.

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.

nirs · 2017-07-06T23:11:30Z

@berkerpeksag are you sure blurb is ready? see python/core-workflow#153

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 · 2017-07-06T23:24:23Z

Changes in version 3:

Integrate nirs/cpython@45931f5 into the tests patch (berkerpeksag)
move the NEWS entry into Misc/NEWS.d/next/Library (berkerpeksag)

@berkerpeksag, @serhiy-storchaka please review.

berkerpeksag · 2017-07-07T06:10:54Z

Thanks!

nirs · 2017-07-07T11:24:44Z

@berkerpeksag, @serhiy-storchaka, @vadmium thanks for reviewing!

Do you want me to backport this to older versions?

berkerpeksag · 2017-07-07T11:36:53Z

@nirs it would be better to fix the test failures reported at https://bugs.python.org/issue29854#msg297876 and https://bugs.python.org/issue29854#msg297877 before doing the backports. Do you have some time to take a look at those failures?

nirs · 2017-07-07T11:46:51Z

@berkerpeksag sure, I'll take a look.

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.

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.

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.

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.

the-knights-who-say-ni added the CLA not signed label Mar 19, 2017

nirs force-pushed the readline-segfault branch from 08fa519 to 9f071fd Compare March 19, 2017 22:28

nirs changed the title ~~bpo-29854: Fix segtault in call_readline()~~ bpo-29854: Fix segfault in call_readline() Mar 19, 2017

Mariatta removed the CLA not signed label Mar 19, 2017

the-knights-who-say-ni added the CLA signed label Mar 19, 2017

serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 2.7 labels Mar 20, 2017

vadmium reviewed Mar 23, 2017

View reviewed changes

serhiy-storchaka reviewed Mar 23, 2017

View reviewed changes

nirs force-pushed the readline-segfault branch from 9f071fd to 6861217 Compare May 19, 2017 20:17

nirs force-pushed the readline-segfault branch from 6861217 to 2811487 Compare May 19, 2017 22:32

vadmium reviewed May 20, 2017

View reviewed changes

berkerpeksag reviewed May 20, 2017

View reviewed changes

nirs force-pushed the readline-segfault branch from 2811487 to a67438c Compare May 21, 2017 15:34

berkerpeksag reviewed Jun 16, 2017

View reviewed changes

nirs force-pushed the readline-segfault branch from a67438c to d2f511d Compare June 16, 2017 18:41

serhiy-storchaka approved these changes Jun 16, 2017

View reviewed changes

nirs added 2 commits July 7, 2017 01:56

Allow setting environment for readline tests

61cd9a1

This enable testing custom readline configuration using the INPUTRC environment variable, or passing arguments to the child process in a clean way.

bpo-29854: Add failing test that crash readline

e6ea330

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.

nirs force-pushed the readline-segfault branch from d2f511d to 9fcda61 Compare July 6, 2017 23:20

berkerpeksag approved these changes Jul 7, 2017

View reviewed changes

berkerpeksag merged commit fae8f4a into python:master Jul 7, 2017

nirs deleted the readline-segfault branch July 7, 2017 13:56

Mariatta removed needs backport to 2.7 labels Sep 7, 2017

bpo-29854: Fix segfault in call_readline() #728

bpo-29854: Fix segfault in call_readline() #728

Conversation

nirs commented Mar 19, 2017

the-knights-who-say-ni commented Mar 19, 2017

mention-bot commented Mar 19, 2017

nirs commented Mar 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirs commented May 19, 2017

nirs commented May 19, 2017

vadmium May 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirs commented May 21, 2017

nirs commented Jun 15, 2017

berkerpeksag left a comment

Choose a reason for hiding this comment

serhiy-storchaka commented Jun 16, 2017

nirs commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirs commented Jun 16, 2017

serhiy-storchaka commented Jun 17, 2017

nirs commented Jul 2, 2017

berkerpeksag commented Jul 3, 2017

serhiy-storchaka commented Jul 3, 2017

nirs commented Jul 4, 2017

nirs commented Jul 6, 2017

nirs commented Jul 6, 2017

berkerpeksag commented Jul 7, 2017

nirs commented Jul 7, 2017

berkerpeksag commented Jul 7, 2017

nirs commented Jul 7, 2017

vadmium May 20, 2017 •

edited

Loading