# [PATCH] readline extension bug fixes and enhancements - Thread: https://externals.io/message/59084 - Original archive: https://news-web.php.net/php.internals/59084 - Started by Osama Abu Elsorour on 2012-03-20T19:06:45+00:00 - Messages: 4 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #59084 — Osama Abu Elsorour — 2012-03-20T19:06:45+00:00 URL: https://externals.io/message/59084#59084 All, I was recently involved in a project that relied heavily on readline to provide console text input capabilities. However I soon noticed that the current readline extension has a serious bug and is lacking some important functionality. This patch applies only to PHP compiled with libreadline (and don't have an effect if compiled with libeditline). It adds/modifies/fixes the following: - Fixes memory corruption caused by directly setting rl_line_buffer to new buffer and not using rl_replace_line(). - Fixes a bad combination of preprocessor #if that would cause `readline_on_new_line()` implementation not to compile when using libreadline only. - Adds support for rl_bind_key function by exposing: function readline_bind_key_function($key, $callback_func) where: $key: Key code to bind to. $callback_func: A callback function in the form: function callback($key, $count) where $key and $count are the same parameters passed from rl_bind_key() Setting $callback_func to null will cause the binding to be removed for $key. - Modifies the behavior of `readline_info()` to allow for setting the readline properties "point" and "end" (it used to only read them but not set them). Patch below: --- php-5.4.0/ext/readline/readline.c 2012-01-01 15:15:04.000000000 +0200 +++ php-5.4.0-patched/ext/readline/readline.c 2012-03-19 09:09:27.388174973 +0200 @@ -58,9 +58,13 @@ PHP_FUNCTION(readline_callback_read_char PHP_FUNCTION(readline_callback_handler_remove); PHP_FUNCTION(readline_redisplay); PHP_FUNCTION(readline_on_new_line); +PHP_FUNCTION(readline_bind_key_function); static zval *_prepped_callback = NULL; +#endif +#if HAVE_LIBREADLINE +static HashTable _bind_key_functions_ht; #endif static zval *_readline_completion = NULL; @@ -121,10 +125,19 @@ ZEND_END_ARG_INFO() ZEND_BEGIN_ARG_INFO(arginfo_readline_redisplay, 0) ZEND_END_ARG_INFO() +#endif +#if HAVE_RL_ON_NEW_LINE | HAVE_LIBREADLINE ZEND_BEGIN_ARG_INFO(arginfo_readline_on_new_line, 0) ZEND_END_ARG_INFO() #endif + +#if HAVE_LIBREADLINE +ZEND_BEGIN_ARG_INFO_EX(arginfo_readline_bind_key_function, 0, 0, 2) + ZEND_ARG_INFO(0, key) + ZEND_ARG_INFO(0, funcname) +ZEND_END_ARG_INFO() +#endif /* }}} */ /* {{{ module stuff */ @@ -145,9 +158,12 @@ static const zend_function_entry php_rea PHP_FE(readline_callback_handler_remove, arginfo_readline_callback_handler_remove) PHP_FE(readline_redisplay, arginfo_readline_redisplay) #endif -#if HAVE_RL_ON_NEW_LINE +#if HAVE_RL_ON_NEW_LINE | HAVE_LIBREADLINE PHP_FE(readline_on_new_line, arginfo_readline_on_new_line) #endif +#if HAVE_LIBREADLINE + PHP_FE(readline_bind_key_function, arginfo_readline_bind_key_function) +#endif PHP_FE_END }; @@ -170,6 +186,9 @@ ZEND_GET_MODULE(readline) PHP_MINIT_FUNCTION(readline) { +#if HAVE_LIBREADLINE + zend_hash_init(&_bind_key_functions_ht, 255, NULL, ZVAL_PTR_DTOR, 1); +#endif using_history(); return PHP_MINIT(cli_readline)(INIT_FUNC_ARGS_PASSTHRU); } @@ -193,6 +212,9 @@ PHP_RSHUTDOWN_FUNCTION(readline) } #endif +#if HAVE_LIBREADLINE + zend_hash_destroy(&_bind_key_functions_ht); +#endif return SUCCESS; } @@ -265,12 +287,20 @@ PHP_FUNCTION(readline_info) if (value) { /* XXX if (rl_line_buffer) free(rl_line_buffer); */ convert_to_string_ex(value); - rl_line_buffer = strdup(Z_STRVAL_PP(value)); + rl_replace_line(Z_STRVAL_PP(value), 1); } RETVAL_STRING(SAFE_STRING(oldstr),1); } else if (!strcasecmp(what, "point")) { + if (value) { + convert_to_long_ex(value); + rl_point = Z_LVAL_PP(value); + } RETVAL_LONG(rl_point); } else if (!strcasecmp(what, "end")) { + if (value) { + convert_to_long_ex(value); + rl_end = Z_LVAL_PP(value); + } RETVAL_LONG(rl_end); #ifdef HAVE_LIBREADLINE } else if (!strcasecmp(what, "mark")) { @@ -621,7 +651,7 @@ PHP_FUNCTION(readline_redisplay) #endif -#if HAVE_RL_ON_NEW_LINE +#if HAVE_RL_ON_NEW_LINE | HAVE_LIBREADLINE /* {{{ proto void readline_on_new_line(void) Inform readline that the cursor has moved to a new line */ PHP_FUNCTION(readline_on_new_line) @@ -632,6 +662,72 @@ PHP_FUNCTION(readline_on_new_line) #endif +/* {{{ proto bool readline_bind_key_function(string funcname) + Readline rl_bind_key */ + +static int _readline_bind_key_cb(int count, int key) +{ + zval *params[2]; + TSRMLS_FETCH(); + + params[0]=_readline_long_zval(key); + params[1]=_readline_long_zval(count); + + zval **_callback_func = NULL; + long _key = key; + int r = zend_hash_find(&_bind_key_functions_ht, (char *) &_key, sizeof(_key), (void **)&_callback_func); + if (r == -1) + return -1; + + int _return_int = 0; + zval *_callback_return = NULL; + MAKE_STD_ZVAL(_callback_return); + + if (call_user_function(CG(function_table), NULL, *_callback_func, _callback_return, 2, params TSRMLS_CC) == SUCCESS) { + _return_int = _callback_return->value.lval; + zval_dtor(_callback_return); + FREE_ZVAL(_callback_return); + } + + return _return_int; +} + +PHP_FUNCTION(readline_bind_key_function) +{ + long key; + zval *arg = NULL; + char *name = NULL; + + if (FAILURE == zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "l!z", &key, &arg)) { + RETURN_FALSE; + } + + if (Z_TYPE_P(arg) == IS_NULL) { + zend_hash_del(&_bind_key_functions_ht, (char *)&key, sizeof(key)); + rl_bind_key(key, rl_insert); + } + else { + if (!zend_is_callable(arg, 0, &name TSRMLS_CC)) { + php_error_docref(NULL TSRMLS_CC, E_WARNING, "%s is not callable", name); + efree(name); + RETURN_FALSE; + } + efree(name); + + zval *_temp_callback = NULL; + MAKE_STD_ZVAL(_temp_callback); + *_temp_callback = *arg; + zval_copy_ctor(_temp_callback); + + zend_hash_add(&_bind_key_functions_ht, (char *)&key, sizeof(key), &_temp_callback, sizeof(zval *), NULL); + + rl_bind_key(key, &_readline_bind_key_cb); + } + + RETURN_TRUE; +} + +/* }}} */ #endif /* HAVE_LIBREADLINE */ --- ## #59086 — Christopher Jones — 2012-03-20T19:17:08+00:00 URL: https://externals.io/message/59084#59086 · In reply to: #59084 > All, > > I was recently involved in a project that relied heavily on readline to provide console text input capabilities. However I soon noticed that the current readline extension has a serious bug and is lacking some important functionality. > > This patch applies only to PHP compiled with libreadline (and don't have an effect if compiled with libeditline). It adds/modifies/fixes the following: > - Fixes memory corruption caused by directly setting rl_line_buffer to new buffer and not using rl_replace_line(). > - Fixes a bad combination of preprocessor #if that would cause `readline_on_new_line()` implementation not to compile when using libreadline only. > - Adds support for rl_bind_key function by exposing: > function readline_bind_key_function($key, $callback_func) > where: > $key: Key code to bind to. > $callback_func: A callback function in the form: > function callback($key, $count) where $key and $count are the same parameters passed from rl_bind_key() > Setting $callback_func to null will cause the binding to be removed for $key. > - Modifies the behavior of `readline_info()` to allow for setting the readline properties "point" and "end" (it used to only read them but not set them). > > Patch below: To avoid your patch getting lost in the mail archives, what about trying out the new github integration?: https://wiki.php.net/vcs/gitworkflow#workflow_for_external_contributors Or log a bug (https://bugs.php.net/) and attach the patch. Chris -- Email: christopher.jones@oracle.com Tel: +1 650 506 8630 Blog: http://blogs.oracle.com/opal/ --- ## #59089 — johannes@schlueters.de — 2012-03-20T19:43:59+00:00 URL: https://externals.io/message/59084#59089 · In reply to: #59084 Hi, > All, > > I was recently involved in a project that relied heavily on readline > to provide console text input capabilities. However I soon noticed > that the current readline extension has a serious bug and is lacking > some important functionality. Have you read Nikita's and my comments on your mail from last week? johannes --- ## #59111 — Osama Abu Elsorour — 2012-03-21T08:03:46+00:00 URL: https://externals.io/message/59084#59111 · In reply to: #59089 Hello > > Have you read Nikita's and my comments on your mail from last week? > I just did. There was a problem with my mailing list subscription (wasn't confirmed) so I thought it didn't go through. But the good thing is that I had ported it to 5.4 and found the memory corruption bug. Here are my answers to your comments: > A) for licensing reasons we should try to keep as few readline only things as possible in there (gpl vs. php license) I understand the issue of licensing but the extension already has readline-specific functionality so I thought why not. However if this is going to be a problem then I can take out all readline-sepcific functions into a separate module altogether and keep this one for editline functions only. > B) thread safty isn't an issue for readline but you still should do the init and deinit in rinit/rshutdown not minit/mshutdown, probably also do only bind_key_functions =NULL in rinit and init the hashtable on demand later. Roger that. > C) don't compare the r result of zend_hash_find and others against -1 or such but use SUCCESS/FAILURE Roger that. > D) i don't really like new features in 5.3 anymore at this stage This last patch is ported to 5.4. > E) please log a bug and attach the patch so we can track this Roger that. After I clear out which direction to take based on your answers. And as for Nikita's comments: > May I ask where you got this pattern for copying zvals from? Is this > kind of code shown in some tutorial / manual / etc? Such code is used > all other the place, instead of the MAKE_COPY_ZVAL macro and I wonder > why. Doing manual copying has a good chance of leaking memory. Just > from glancing I'd say that your code will leak if arg has a refcount > > 1 (but I could be wrong) as it does not PINIT the zval after it was > copied. Two points here: A) The new patch does not use this at all because it really isn't needed B) I got this from the same readline.c file lines 521 and 574 and 579. But I will change it accordingly