Re: [PATCH] [readline] Fix double free in _rl_scxt_dispose

[Andrew Burgess]

Tom de Vries <tdevries@suse.de> writes:

> Consider the following scenario.  We start gdb in TUI mode:
> ...
> $ gdb -q -tui
> ...
> and type ^R which gives us the reverse-isearch prompt in the cmd window:
> ...
> (reverse-i-search)`':
> ...
> and then type "foo", right-arrow-key, and ^C.
>
> In TUI mode, gdb uses a custom rl_getc_function tui_getc.
>
> When pressing the right-arrow-key, tui_getc:
> - attempts to scroll the TUI src window, without any effect, and
> - returns 0.
>
> The intention of returning 0 is mentioned here in tui_dispatch_ctrl_char:
> ...
>   /* We intercepted the control character, so return 0 (which readline
>      will interpret as a no-op).  */
>   return 0;
> ...
>
> However, after this 0 is returned by the rl_read_key () call in
> _rl_search_getchar, _rl_read_mbstring is called, which incorrectly interprets
> 0 as the first part of an utf-8 multibyte char, and tries to read the next
> char.
>
> In this state, the ^C takes effect and we run into a double free because
> _rl_isearch_cleanup is called twice.
>
> Both these issues need fixing independently, though after fixing the first we
> no longer trigger the second.
>
> The first issue is caused by the subtle difference between:
> - a char array containing 0 chars, which is zero-terminated, and
> - a char array containing 1 char, which is zero.
>
> In mbrtowc terms, this is the difference between:
> ...
>   mbrtowc (&wc, "", 0, &ps);
> ...
> which returns -2, and:
> ...
>   mbrtowc (&wc, "", 1, &ps);
> ...
> which returns 0.
>
> Note that _rl_read_mbstring calls _rl_get_char_len without passing it an
> explicit length parameter, and consequently it cannot distinguish between the
> two, and defaults to the "0 chars" choice.
>
> Note that the same problem doesn't exist in _rl_read_mbchar.
>
> Fix this by defaulting to the "1 char" choice in _rl_get_char_len:
> ...
> -  if (_rl_utf8locale && l > 0 && UTF8_SINGLEBYTE(*src))
> +  if (_rl_utf8locale && l >= 0 && UTF8_SINGLEBYTE(*src))
> ...
>
> The second problem happens when the call to _rl_search_getchar in
> _rl_isearch_callback returns.  At that point _rl_isearch_cleanup has already
> been called from the signal handler, but we proceed regardless, using a cxt
> pointer that has been freed.
>
> Fix this by checking for "RL_ISSTATE (RL_STATE_ISEARCH)" after the call to
> _rl_search_getchar:
> ...
>    c = _rl_search_getchar (cxt);
> +  if (!RL_ISSTATE (RL_STATE_ISEARCH))
> +    return 1;
> ...
>
> Tested on x86_64-linux.
>
> PR tui/30056
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30056
> ---
>  readline/readline/isearch.c | 3 +++
>  readline/readline/mbutil.c  | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)

Have you posted this to the readline list?  I think it would be best if
we at least posted patches like this upstream before we merge them.

Thanks,
Andrew



>
> diff --git a/readline/readline/isearch.c b/readline/readline/isearch.c
> index 080ba3cbb9c..941078f790e 100644
> --- a/readline/readline/isearch.c
> +++ b/readline/readline/isearch.c
> @@ -882,6 +882,9 @@ _rl_isearch_callback (_rl_search_cxt *cxt)
>    int c, r;
>  
>    c = _rl_search_getchar (cxt);
> +  if (!RL_ISSTATE (RL_STATE_ISEARCH))
> +    return 1;
> +
>    /* We might want to handle EOF here */
>    r = _rl_isearch_dispatch (cxt, cxt->lastc);
>  
> diff --git a/readline/readline/mbutil.c b/readline/readline/mbutil.c
> index dc62b4cc24d..7da3ff17bb5 100644
> --- a/readline/readline/mbutil.c
> +++ b/readline/readline/mbutil.c
> @@ -363,7 +363,7 @@ _rl_get_char_len (char *src, mbstate_t *ps)
>  
>    /* Look at no more than MB_CUR_MAX characters */
>    l = (size_t)strlen (src);
> -  if (_rl_utf8locale && l > 0 && UTF8_SINGLEBYTE(*src))
> +  if (_rl_utf8locale && l >= 0 && UTF8_SINGLEBYTE(*src))
>      tmp = (*src != 0) ? 1 : 0;
>    else
>      {
>
> base-commit: 9196be90bd9572bd09fad63d7e0b2fa199738b90
> -- 
> 2.35.3