classification
Title: Segfault when readline history is more then 2 * history size
Type: crash Stage: resolved
Components: Extension Modules Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Nir Soffer, berker.peksag, martin.panter, nirs, serhiy.storchaka, twouters, vstinner
Priority: normal Keywords:

Created on 2017-03-19 22:15 by nirs, last changed 2017-10-18 13:54 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 728 merged python-dev, 2017-03-19 22:27
PR 2618 closed berker.peksag, 2017-07-07 12:24
PR 2619 merged vstinner, 2017-07-07 12:46
PR 2621 merged nirs, 2017-07-07 17:13
PR 2633 merged nirs, 2017-07-08 16:00
PR 2636 merged nirs, 2017-07-08 23:22
PR 2637 merged nirs, 2017-07-08 23:28
Messages (24)
msg289865 - (view) Author: Nir Soffer (nirs) * Date: 2017-03-19 22:15
GNU readline let the user select limit the history size by setting:

$ cat ~/.inputrc 
set history-size 1000

So I cooked this test script:

$ cat history.py 
from __future__ import print_function
import readline

readline.read_history_file(".history")

print("current_history_length", readline.get_current_history_length())
print("history_length", readline.get_history_length())
print("history_get_item(1)", readline.get_history_item(1))
print("history_get_item(1000)", readline.get_history_item(1000))

input()

readline.write_history_file(".history")


And this history file generator:

$ cat make-history 
for i in range(2000):
    print("%04d" % i)


Generating .history file with 2000 entries:
$ python3 make-history > .history


Finally running the test script:

$ python3 history.py 
current_history_length 1000
history_length -1
history_get_item(1) None
history_get_item(1000) None
please crash
Segmentation fault (core dumped)

So we have few issues here:
- segfault
- history_get_item returns None for both 1 and 1000
  although we have 1000 items in history
- history_length is always wrong (-1), instead of
  the expected value (1000), set in .inputrc

Running with gdb we see:

$ gdb python3
GNU gdb (GDB) Fedora 7.12.1-46.fc25
Copyright (C) 2017 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-redhat-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from python3...Reading symbols from /usr/lib/debug/usr/libexec/system-python.debug...done.
done.

(gdb) run history.py
Starting program: /usr/bin/python3 history.py
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
current_history_length 1000
history_length -1
history_get_item(1) None
history_get_item(1000) None
crash?

Program received signal SIGSEGV, Segmentation fault.
0x00007fffeff60fab in call_readline (sys_stdin=<optimized out>, sys_stdout=<optimized out>, prompt=<optimized out>) at /usr/src/debug/Python-3.5.2/Modules/readline.c:1281
1281	            line = (const char *)history_get(length)->line;

(gdb) list
1276	            if (using_libedit_emulation) {
1277	                /* handle older 0-based or newer 1-based indexing */
1278	                line = (const char *)history_get(length + libedit_history_start - 1)->line;
1279	            } else
1280	#endif /* __APPLE__ */
1281	            line = (const char *)history_get(length)->line;
1282	        else
1283	            line = "";
1284	        if (strcmp(p, line))
1285	            add_history(p);


So we assume that history_get(length) returns non-null
when length > 0, but this assumption is not correct.

In 2 other usages in Modules/readline.c, we validate
that history_get() return value is not null before
using it.


If we change the .history contents to 1999 lines, we get:

$ python3 make-history | head -1999 > .history

$ python3 history.py
current_history_length 1000
history_length -1
history_get_item(1) None
history_get_item(1000) 0999
crash?

$ wc -l .history
1000 .history

$ head -1 .history
1000
$ tail -1 .history
crash?

So now it does not crash, but item 1 is still None.


Trying again with history file with 1000 entries:

$ python3 make-history | head -1000 > .history

$ python3 history.py
current_history_length 1000
history_length -1
history_get_item(1) 0000
history_get_item(1000) 0999
looks fine!

$ wc -l .history
1000 .history

$ head -1 history
head: cannot open 'history' for reading: No such file or directory

$ head -1 .history
0001

$ tail -1 .history
looks fine!


Finally trying with 1001 items:

$ python3 make-history | head -1001 > .history

$ python3 history.py
current_history_length 1000
history_length -1
history_get_item(1) None
history_get_item(1000) 0999

And item 1 is wrong.


I got same results with python 2.7, 3.5 and master
on fedora 25.

The root cause seems to be a readline bug when history
file is bigger than the history-size in .inputrc,
but I could not find yet readline library documentation,
so  I don't know if the issues is incorrect usage of the
readline apis, or bug in readline.
msg289881 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-20 09:33
The fix LGTM. Any chance to write a test? And please add an entry in Misc/NEWS.
msg289883 - (view) Author: Nir Soffer (nirs) * Date: 2017-03-20 11:26
Sure, I'll add news entry and tests.
msg290044 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2017-03-23 11:57
Gnu Readline comes includes its own documentation (e.g. /usr/share/info/history.info.gz on my computer). It is also at <https://cnswww.cns.cwru.edu/php/chet/readline/history.html>.

Perhaps the history_base value is relevant; see some of the comments starting at <https://bugs.python.org/issue6953#msg100466>.

It would be interesting to see if Apple Editline is affected. According to the comment in the get_history_item function, history_get might crash before returning the null pointer. Is there some other workaround that avoids calling history_get?
msg293981 - (view) Author: Nir Soffer (nirs) * Date: 2017-05-19 22:25
I think the issue can be solved in readline or in the code using it, but I don't have more time to dig into this, and I think that python should not crash in this case.

I don't have an environment to test Apple editline, so I cannot test this issue. The PR includes a test case now, running the new test on OS X will tell us if this is the case.
msg293988 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2017-05-20 01:34
I suspect the test won’t be effective with Editline (either fail, or silently pass without testing the bug). From memory Editline uses an ~/.editrc file, not INPUTRC, with different syntax and configuration.
msg294101 - (view) Author: Nir Soffer (nirs) * Date: 2017-05-21 12:50
This issue does not exist on OS X 10.11.6 (latest my old mac can install).

I tested using .editrc file:

$ cat ~/.editrc
history size 5

With history file with 10 items that crashes on Linux using GNU readline.

This settings is ignored, adding items to the history file without truncating it to 5 items.

I tested also truncating the size using readline.set_history_size(). It works correctly, but this means every application need to implement its own readline configuration, instead of reusing the system readline configuration.

So this bug is relevant only to GNU readline, need to skip this test when using libedit.
msg297863 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-07-07 06:10
New changeset fae8f4a9cb88a68eb14750cbb8ddf8740fd67b8b by Berker Peksag (Nir Soffer) in branch 'master':
bpo-29854: Fix segfault in call_readline() (GH-728)
https://github.com/python/cpython/commit/fae8f4a9cb88a68eb14750cbb8ddf8740fd67b8b
msg297876 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-07-07 09:44
The test fails on AMD64 FreeBSD 10.x Shared 3.x:

http://buildbot.python.org/all/builders/AMD64%20FreeBSD%2010.x%20Shared%203.x/builds/551/steps/test/logs/stdio

======================================================================
FAIL: test_history_size (test.test_readline.TestReadline)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/home/buildbot/python/3.x.koobs-freebsd10/build/Lib/test/test_readline.py", line 247, in test_history_size
    self.assertEqual(len(lines), history_size)
AssertionError: 21 != 10
msg297877 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-07-07 09:49
Similar failure on x86 Tiger 3.x.

Maybe we need to skip the test on old macOS and old FreeBSD versions? Maybe it's related to the libncurses version?

http://buildbot.python.org/all/builders/x86%20Tiger%203.x/builds/908/steps/test/logs/stdio

======================================================================
FAIL: test_history_size (test.test_readline.TestReadline)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/db3l/buildarea/3.x.bolen-tiger/build/Lib/test/test_readline.py", line 247, in test_history_size
    self.assertEqual(len(lines), history_size)
AssertionError: 21 != 10
msg297879 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-07-07 10:24
> Similar failure on x86 Tiger 3.x.

This one is interesting. I thought we don't have OS X buildbots with readline installed.

I would prefer skipping the test based on readline version installed.

Side note: I think we should print readline, sqlite3 etc. versions in  https://github.com/python/cpython/blob/fae8f4a9cb88a68eb14750cbb8ddf8740fd67b8b/Lib/test/libregrtest/main.py#L421
msg297880 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-07-07 10:49
I've opened PR 2618 to print readline version and implementation in regrtest's display_header() method.
msg297881 - (view) Author: Nir Soffer (Nir Soffer) Date: 2017-07-07 11:54
The failures looks like libedit failures on OS X, where history size is ignored. The test is skipped if is_editline is set, we should probably skip on these platforms too.
msg297884 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-07-07 14:07
New changeset 1881befb905553618f1e7ad2cef8f6ff07e1b8ef by Victor Stinner in branch 'master':
bpo-29854: test_readline logs versions (#2619)
https://github.com/python/cpython/commit/1881befb905553618f1e7ad2cef8f6ff07e1b8ef
msg297887 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-07-07 14:53
AMD64 FreeBSD 9.x 3.x, test_history_size() fails:

readline version: 0x502
readline runtime version: 0x502
readline library version: '5.2'
use libedit emulation? False


AMD64 FreeBSD 10.x Shared 3.x, test_history_size() fails:

readline version: 0x502
readline runtime version: 0x502
readline library version: '5.2'
use libedit emulation? False


x86 Tiger 3.x, test_history_size() fails:

readline version: 0x501
readline runtime version: 0x501
readline library version: '5.1'
use libedit emulation? False

--

My Linux box, test_history_size() pass:

readline version: 0x603
readline runtime version: 0x603
readline library version: '6.3'
use libedit emulation? False
msg297895 - (view) Author: Nir Soffer (Nir Soffer) Date: 2017-07-07 16:12
So we have version 0x502 without libedit emulation succeeding on
FreeBSD 9.x, and failing on 10.x.

I think we are missing something, or maybe the libedit check is wrong.

We need results from all builders to do something with this. I think 
at least for now we want to see readline info from all builders, not only
for failed tests.

Maybe switch the failing test to run only on Linux for now?
msg297896 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-07-07 16:34
> So we have version 0x502 without libedit emulation succeeding on
> FreeBSD 9.x, and failing on 10.x.

test_history_size() fails on FreeBSD 9.x too:

======================================================================
FAIL: test_history_size (test.test_readline.TestReadline)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/home/buildbot/python/3.x.koobs-freebsd9/build/Lib/test/test_readline.py", line 263, in test_history_size
    self.assertEqual(len(lines), history_size)
AssertionError: 21 != 10

http://buildbot.python.org/all/builders/AMD64%20FreeBSD%209.x%203.x/builds/316/steps/test/logs/stdio
msg297897 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-07-07 16:49
According to https://cnswww.cns.cwru.edu/php/chet/readline/CHANGES, the history-size setting was added in readline 6.0:

    e.  A new user-settable variable, `history-size', allows setting the maximum
        number of entries in the history list.

The only thing we need to do is skip the test if readline version is older than 6.0.

We discussed this with Nir on IRC, and he will send another PR to tweak the test.
msg297955 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-07-08 14:34
New changeset aa6a4d6ed881f79c51fb91dd928ed9496737b420 by Berker Peksag (Nir Soffer) in branch 'master':
bpo-29854: Skip history-size test on older readline (GH-2621)
https://github.com/python/cpython/commit/aa6a4d6ed881f79c51fb91dd928ed9496737b420
msg297967 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-07-08 18:51
New changeset 04f77d4677e7508b6ec8de9d0331fdabbcd11d30 by Berker Peksag (Nir Soffer) in branch '3.6':
[3.6] bpo-29854: Fix segfault in call_readline() (GH-728)
https://github.com/python/cpython/commit/04f77d4677e7508b6ec8de9d0331fdabbcd11d30
msg297989 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-07-09 10:42
New changeset 68c3724651776f4ae90ed24d70cef6fd45bc7db5 by Berker Peksag (Nir Soffer) in branch '3.5':
[3.5] bpo-29854: Fix segfault in call_readline() (GH-728)
https://github.com/python/cpython/commit/68c3724651776f4ae90ed24d70cef6fd45bc7db5
msg298082 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-07-10 21:06
New changeset bfa4fe4f39dd8b5ce1a0b649cedd36857859081a by Berker Peksag (Nir Soffer) in branch '2.7':
[2.7] bpo-29854: Fix segfault in call_readline() (GH-728)
https://github.com/python/cpython/commit/bfa4fe4f39dd8b5ce1a0b649cedd36857859081a
msg298084 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-07-10 21:09
Ok, I think we can finally close this one :) Thank you, everyone!
msg304578 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-18 13:54
FYI I added the test.pythoninfo utility as a follow-up of this issue to log many informations to debug Python, not only the readline version: see bpo-30871.
History
Date User Action Args
2017-10-18 13:54:07vstinnersetmessages: + msg304578
2017-07-10 21:09:39berker.peksagsetstatus: open -> closed
resolution: fixed
messages: + msg298084

stage: backport needed -> resolved
2017-07-10 21:06:59berker.peksagsetmessages: + msg298082
2017-07-09 10:42:39berker.peksagsetmessages: + msg297989
2017-07-08 23:28:04nirssetpull_requests: + pull_request2702
2017-07-08 23:22:20nirssetpull_requests: + pull_request2701
2017-07-08 18:51:23berker.peksagsetmessages: + msg297967
2017-07-08 16:00:02nirssetpull_requests: + pull_request2698
2017-07-08 14:34:29berker.peksagsetmessages: + msg297955
2017-07-07 17:13:58nirssetpull_requests: + pull_request2687
2017-07-07 16:49:11berker.peksagsetmessages: + msg297897
2017-07-07 16:34:49berker.peksagsetmessages: + msg297896
2017-07-07 16:12:32Nir Soffersetmessages: + msg297895
2017-07-07 14:53:53vstinnersetmessages: + msg297887
2017-07-07 14:07:00vstinnersetmessages: + msg297884
2017-07-07 12:46:36vstinnersetpull_requests: + pull_request2685
2017-07-07 12:24:03berker.peksagsetpull_requests: + pull_request2684
2017-07-07 11:54:04Nir Soffersetnosy: + Nir Soffer
messages: + msg297881
2017-07-07 10:49:24berker.peksagsetmessages: + msg297880
2017-07-07 10:24:30berker.peksagsetmessages: + msg297879
2017-07-07 09:49:48vstinnersetmessages: + msg297877
2017-07-07 09:44:38vstinnersetnosy: + vstinner
messages: + msg297876
2017-07-07 06:11:59berker.peksagsetstage: patch review -> backport needed
2017-07-07 06:10:48berker.peksagsetnosy: + berker.peksag
messages: + msg297863
2017-05-21 12:50:06nirssetmessages: + msg294101
2017-05-20 01:34:44martin.pantersetmessages: + msg293988
2017-05-19 22:25:31nirssetmessages: + msg293981
2017-03-23 11:57:57martin.pantersetnosy: + martin.panter
messages: + msg290044
2017-03-20 11:26:30nirssetmessages: + msg289883
2017-03-20 09:33:28serhiy.storchakasetversions: + Python 3.6
nosy: + serhiy.storchaka, twouters

messages: + msg289881

type: crash
stage: patch review
2017-03-19 22:27:36python-devsetpull_requests: + pull_request643
2017-03-19 22:15:24nirscreate