readline.set_completer_delims() doesn't play well with others #61491

Activity

1. bfroehle commented on Feb 24, 2013

   

   bfroehlemannequin

   on Feb 24, 2013

   MannequinAuthor

   More actions

   The readline.set_completer_delims doesn't play well with others because
   it assumes that only it ever allocates or modifies the
   rl_completer_word_break_characters buffer. If other programs modify this
   value, for example changing it from heap allocated space to stack
   allocated space, the results can be catastrophic.

   To remind you, the function essentially works as:

       set_completer_delims(PyObject *self, PyObject *args)
       {
           // ...
           free((void*) rl_completer_word_break_characters;
           rl_completer_word_break_characters = strdup(break_chars);
           // ...
       }

   where break_chars is the user provided string.

   Take, for example, R as another programs which changes the readline
   completer strings. When an embedded R instance is initialized (say, using
   r2py) something similar to the following takes place::

       static void
       set_rl_completer_word_break_characters(const char *new)
       {
           static char[201] buffer;
           strncpy(buffer, new, 200);
           rl_completer_word_break_characters = buffer;
       }
   
       static void
       initialize_embedded_R(...)
       {
           // ...
           set_rl_completer_word_break_characters(...);
       }

   As you might expect the next trip through readline.set_completer_delims
   after initializing R will be catastrophic when we attempt to free a stack
   allocate buffer.

   I think we should consider modifying the readline.set_completer_delims
   to store the allocated buffers in the module state::

       set_completer_delims(PyObject *self, PyObject *args)
       {
           // ...
           free(_readlinestate_global->break_chars);
           rl_completer_word_break_characters = strdup(break_chars);
           _readlinestate_global->break_chars = rl_completer_word_break_characters;
           // ...
       }

   This would prevent the segfault and memory leaks, and would render weird
   hacks (like https://bitbucket.org/lgautier/rpy2/commits/408bae913653 in
   the r2py code) unnecessary.
2. bfroehle

   added

   extension-modulesC modules in the Modules dirC modules in the Modules dir

   type-crashA hard crash of the interpreter, possibly with a core dumpA hard crash of the interpreter, possibly with a core dump

   on Feb 24, 2013
3. pitrou commented on May 4, 2013

   

   pitrou

   on May 4, 2013

   Member

   More actions

   Thanks for reporting this. Do you want to contribute a proper patch? You'll find instructions at http://docs.python.org/devguide/
4. bfroehle commented on May 5, 2013

   

   bfroehlemannequin

   on May 5, 2013

   MannequinAuthor

   More actions

   Patch attached. I implemented this by adding a 'static char *' which
   holds the memory we allocate. I did not use the PyState machinery.
5. python-dev commented on May 6, 2013

   

   python-devmannequin

   on May 6, 2013

   Mannequin

   More actions

   New changeset 55c7295aca6c by Antoine Pitrou in branch '2.7':
   Issue bpo-17289: The readline module now plays nicer with external modules or applications changing the rl_completer_word_break_characters global variable.
   http://hg.python.org/cpython/rev/55c7295aca6c
6. python-dev commented on May 6, 2013

   

   python-devmannequin

   on May 6, 2013

   Mannequin

   More actions

   New changeset df0afd3ebb70 by Antoine Pitrou in branch '3.3':
   Issue bpo-17289: The readline module now plays nicer with external modules or applications changing the rl_completer_word_break_characters global variable.
   http://hg.python.org/cpython/rev/df0afd3ebb70

   New changeset 0f65426009e2 by Antoine Pitrou in branch 'default':
   Issue bpo-17289: The readline module now plays nicer with external modules or applications changing the rl_completer_word_break_characters global variable.
   http://hg.python.org/cpython/rev/0f65426009e2
7. pitrou commented on May 6, 2013

   

   pitrou

   on May 6, 2013

   Member

   More actions

   Thanks for the patch! I made a variable name a bit shorter and also added some error checking on the strdup() result.
8. pitrou

   closed this as completedon May 6, 2013
9. ezio-melotti

   transferred this issue fromon Apr 10, 2022