classification
Title: Memory leak in readline.get_current_history_length
Type: resource usage Stage:
Components: Library (Lib) Versions: Python 3.3, Python 3.2, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Alexander.Belopolsky, mark.dickinson, pitrou, ronaldoussoren, stefanholek, yaneurabeya, zvezdan
Priority: normal Keywords: patch

Created on 2010-03-05 10:27 by stefanholek, last changed 2011-11-29 06:24 by ezio.melotti.

Files
File name Uploaded Description Edit
issue8065.diff Alexander.Belopolsky, 2010-03-05 17:06 patch against revision 78631 review
issue8065-tests.diff Alexander.Belopolsky, 2010-03-05 17:59 patch against revision 78631 review
Messages (12)
msg100462 - (view) Author: Stefan Holek (stefanholek) Date: 2010-03-05 10:27
The history state must be freed, like so:

diff --git a/rl/readline.c b/rl/readline.c
index a676892..2394dca 100644
--- a/rl/readline.c
+++ b/rl/readline.c
@@ -576,9 +576,12 @@ static PyObject *
 get_current_history_length(PyObject *self, PyObject *noarg)
 {
        HISTORY_STATE *hist_st;
+       PyObject *length;
 
        hist_st = history_get_history_state();
-       return PyInt_FromLong(hist_st ? (long) hist_st->length : (long) 0);
+       length = PyInt_FromLong(hist_st ? (long) hist_st->length : (long) 0);
+       free(hist_st);
+       return length;
 }
 
 PyDoc_STRVAR(doc_get_current_history_length,
msg100492 - (view) Author: Alexander Belopolsky (Alexander.Belopolsky) Date: 2010-03-05 17:06
Note the following comment elsewhere in Modules/readline.c:

           /* the history docs don't say so, but the address of state                                                                   
              changes each time history_get_history_state is called                                                                      
              which makes me think it's freshly malloc'd memory...                                                                       
              on the other hand, the address of the last line stays the                                                                  
              same as long as history isn't extended, so it appears to                                                                   
              be malloc'd but managed by the history package... */
           free(state);


It is indeed not documented that state is malloced, but the implementation of history_get_history_state () is as follows:

/* Return the current HISTORY_STATE of the history. */
HISTORY_STATE *
history_get_history_state ()
{
  HISTORY_STATE *state;

  state = (HISTORY_STATE *)xmalloc (sizeof (HISTORY_STATE));
  state->entries = the_history;
  state->offset = history_offset;
  state->length = history_length;
  state->size = history_size;
  state->flags = 0;
  if (history_stifled)
    state->flags |= HS_STIFLED;

  return (state);
}

xmalloc () is an error checking wrapper around malloc, so free () can be used to deallocate the memory.

On the other hand it seems wasteful to request full state in a function that only needs history_length which is directly exported by the readline library.  I am attaching a patch that reads history_length directly.
msg100495 - (view) Author: Alexander Belopolsky (Alexander.Belopolsky) Date: 2010-03-05 17:59
Not directly related to the issue at hand, but I've noticed that while readline.get_current_history_length() is tested in the unittests, readline.get_history_length() is not.  Attached patch adds tests for reading and writing history files.

Also, I find it confusing that readline module static variable _history_length is named almost the same as readline library exported variable history_length.  Maybe _history_length could be renamed to max_history_file_length.
msg100553 - (view) Author: Alexander Belopolsky (Alexander.Belopolsky) Date: 2010-03-07 00:00
It appears that the tests that I attached fail when libedit is used.  This is clearly due to bugs in libedits readline emulation:

1. read_history does not update history_length

2. history_truncate_file does not preserver the history cookie ("_HiStOrY_V2_").

Does anyone know where libedit bugs should be reported?
msg100566 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2010-03-07 07:22
On 7 Mar, 2010, at 1:02, Alexander Belopolsky wrote:
> 
> 
> Does anyone know where libedit bugs should be reported?

Apple's bugreporter. (bugreport.apple.com, you need an ADC account to report bugs).
msg100764 - (view) Author: Alexander Belopolsky (Alexander.Belopolsky) Date: 2010-03-09 22:06
I my experience, reporting bugs in open source components of OSX to bugreport.apple.com is a waste of time.   Such reports are largely ignored  and they are not visible to upstream developers.

I believe the upstream for libedit is NetBSD, http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libedit, but I cannot find their bug tracker.
msg100766 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2010-03-09 22:15
Without filing a bug Apple won't know that something is wrong and they will definitly not fix the issue. If you file an issue and post the radar number I'll ping the Python maintainer inside Apple.  There's little change that this will be fixed, but you never know.
msg100768 - (view) Author: Alexander Belopolsky (Alexander.Belopolsky) Date: 2010-03-09 22:49
I submitted two bug reports:

7734961
libedit history_truncate_file () fails to preserve magic line
Mac OS X
Other Bug
09-Mar-2010 05:48 PM
Open

7734839
libedit read_history() does not update history_length
Mac OS X
Other Bug
09-Mar-2010 05:39 PM
Open
msg112367 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-08-01 18:57
See also issue 9450 (leaks in readline.replace_history_item and readline.remove_history_item).
msg112631 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-08-03 16:57
I've fixed this leak in r83670 through r83672.  It's still using the old, inefficient method (get the state, read the length, free the state), because without good tests I don't want to disturb things too much.  In particular, it's not clear whether libedit keeps the history_length variable properly updated.

There are still the remaining issues of:

 - better testing for the readline module, and
 - attempting to work around libedit bugs.

Perhaps those should become separate issues, though?
msg134340 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-04-24 19:37
For the record, I tried Alexander's tests on the buildbots and it failed on OS X Leopard:
http://www.python.org/dev/buildbot/all/builders/AMD64%20Leopard%20custom/builds/12/steps/test/logs/stdio
but succeeded on OS X Tiger:
http://www.python.org/dev/buildbot/all/builders/x86%20Tiger%20custom/builds/18/steps/test/logs/stdio
msg145320 - (view) Author: Garrett Cooper (yaneurabeya) Date: 2011-10-11 00:59
As a note for future reference, FreeBSD/NetBSD/OpenBSD doesn't use the term "bug", but instead uses the term "problem report" (the NetBSD website says "bug" though BTW).

The PR system for NetBSD can be accessed here: http://www.netbsd.org/cgi-bin/sendpr.cgi?gndb=netbsd .
History
Date User Action Args
2011-11-29 06:24:40ezio.melottisetversions: - Python 3.1
2011-10-11 00:59:08yaneurabeyasetnosy: + yaneurabeya
messages: + msg145320
2011-04-24 19:37:39pitrousetnosy: + pitrou

messages: + msg134340
versions: + Python 3.3
2010-08-03 16:57:08mark.dickinsonsetmessages: + msg112631
versions: + Python 3.1, Python 2.7, Python 3.2
2010-08-01 18:57:02mark.dickinsonsetnosy: + mark.dickinson
messages: + msg112367
2010-03-09 22:49:52Alexander.Belopolskysetmessages: + msg100768
2010-03-09 22:15:02ronaldoussorensetmessages: + msg100766
2010-03-09 22:14:41ronaldoussorensetfiles: - smime.p7s
2010-03-09 22:06:45Alexander.Belopolskysetmessages: + msg100764
2010-03-08 22:24:10zvezdansetnosy: + zvezdan
2010-03-07 07:22:51ronaldoussorensetfiles: + smime.p7s

messages: + msg100566
2010-03-07 00:00:45Alexander.Belopolskysetnosy: + ronaldoussoren
messages: + msg100553
2010-03-05 17:59:41Alexander.Belopolskysetfiles: + issue8065-tests.diff

messages: + msg100495
2010-03-05 17:06:38Alexander.Belopolskysetfiles: + issue8065.diff

nosy: + Alexander.Belopolsky
messages: + msg100492

keywords: + patch
2010-03-05 10:27:40stefanholekcreate