[Bug tui/30056] New: double free when using reverse-search for a previous command and Ctrl-C

[Bug tui/30056] New: double free when using reverse-search for a previous command and Ctrl-C

[etesta at undo dot io]

https://sourceware.org/bugzilla/show_bug.cgi?id=30056

            Bug ID: 30056
           Summary: double free when using reverse-search for a previous
                    command and Ctrl-C
           Product: gdb
           Version: 12.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: tui
          Assignee: unassigned at sourceware dot org
          Reporter: etesta at undo dot io
  Target Milestone: ---

The sequence of actions to load a program: "gdb my_file" and then

1) (gdb) start
2) (gdb) layout src
3) Ctrl-r - type anything
4) right arrow
5) Ctrl-C

the following message appears on the screen (tested with 9.2, 10.2 and 12.1):

(gdb) free(): double free detected in tcache 2


                                             Fatal signal:

                                                            Segmentation fault
(core dumped)

A bit of analysis showed the following:

Before the signal is delivered gdb is in __libc_read() as part of the isearch()
stack:

#0  __libc_read (nbytes=1, buf=0x7ffcca50d2f0, fd=0) at
../sysdeps/unix/sysv/linux/read.c:26
#1  __libc_read (fd=0, buf=buf@entry=0x7ffcca50d2f0, nbytes=nbytes@entry=1) at
../sysdeps/unix/sysv/linux/read.c:24
#2  0x00007f9a63a1fdf7 in fifo_push (sp=0xd5d5c00) at
.././ncurses-6.3/ncurses/base/lib_getch.c:349
#3  kgetch (forever=<optimised out>, sp=0xd5d5c00) at
.././ncurses-6.3/ncurses/base/lib_getch.c:733
#4  _nc_wgetch (win=win@entry=0xd36af90, result=result@entry=0x7ffcca50d3cc,
use_meta=<optimised out>) at .././ncurses-6.3/ncurses/base/lib_getch.c:564
#5  0x00007f9a63a207d7 in wgetch (win=win@entry=0xd36af90) at
.././ncurses-6.3/ncurses/base/lib_getch.c:694
#6  0x0000000000947735 in gdb_wgetch (win=0xd36af90) at
.././gdb-10.2/gdb/tui/tui-io.c:681
#7  tui_getc_1 (fp=<optimised out>) at .././gdb-10.2/gdb/tui/tui-io.c:960
#8  tui_getc (fp=<optimised out>) at .././gdb-10.2/gdb/tui/tui-io.c:1039
#9  0x00000000009a3f45 in rl_read_key () at
../.././gdb-10.2/readline/readline/input.c:495
#10 rl_read_key () at ../.././gdb-10.2/readline/readline/input.c:455
#11 0x00000000009a408d in _rl_read_mbstring (first=<optimised out>,
mb=mb@entry=0x3f967b0 "", mlen=mlen@entry=16) at
../.././gdb-10.2/readline/readline/input.c:686
#12 0x000000000099a259 in _rl_search_getchar (cxt=0x3f96740) at
../.././gdb-10.2/readline/readline/isearch.c:314
#13 0x000000000099b1d9 in _rl_isearch_callback (cxt=0x3f96740) at
../.././gdb-10.2/readline/readline/isearch.c:823
#14 0x00000000009a4546 in rl_callback_read_char () at
../.././gdb-10.2/readline/readline/callback.c:164
#15 0x00000000006b563e in gdb_rl_callback_read_char_wrapper_noexcept () at
.././gdb-10.2/gdb/event-top.c:177
#16 0x00000000006b660e in gdb_rl_callback_read_char_wrapper
(client_data=<optimised out>) at .././gdb-10.2/gdb/event-top.c:194
#17 0x00000000006b5580 in stdin_event_handler (error=<optimised out>,
client_data=0x3f27170) at .././gdb-10.2/gdb/event-top.c:516
#18 0x0000000000dc21d5 in gdb_wait_for_event (block=block@entry=1) at
.././gdb-10.2/gdbsupport/event-loop.cc:673
#19 0x0000000000dc22ad in gdb_wait_for_event (block=1) at
.././gdb-10.2/gdbsupport/event-loop.cc:569
#20 gdb_do_one_event () at .././gdb-10.2/gdbsupport/event-loop.cc:215
#21 0x00000000007709e5 in start_event_loop () at .././gdb-10.2/gdb/main.c:356



The first free happens in _rl_scxt_dispose() as part of the following call
stack:

#0  0x000000000099a1d2 in _rl_scxt_dispose (cxt=cxt@entry=0x3f96740,
flags=flags@entry=0) at ../.././gdb-10.2/readline/readline/isearch.c:128
#1  0x000000000099aeb9 in _rl_isearch_cleanup (cxt=0x3f96740, r=r@entry=0) at
../.././gdb-10.2/readline/readline/isearch.c:768
#2  0x00000000009a4816 in rl_callback_sigcleanup () at
../.././gdb-10.2/readline/readline/callback.c:343
#3  0x00000000009a144f in _rl_handle_signal (sig=2) at
../.././gdb-10.2/readline/readline/signals.c:218
#4  0x00000000009a1504 in _rl_signal_handler (sig=<optimised out>) at
../.././gdb-10.2/readline/readline/signals.c:158
#5  0x00000000009a3eea in rl_read_key () at
../.././gdb-10.2/readline/readline/input.c:497
#6  rl_read_key () at ../.././gdb-10.2/readline/readline/input.c:455
#7  0x00000000009a408d in _rl_read_mbstring (first=<optimised out>,
mb=mb@entry=0x3f967b0 "", mlen=mlen@entry=16) at
../.././gdb-10.2/readline/readline/input.c:686
#8  0x000000000099a259 in _rl_search_getchar (cxt=0x3f96740) at
../.././gdb-10.2/readline/readline/isearch.c:314
#9  0x000000000099b1d9 in _rl_isearch_callback (cxt=0x3f96740) at
../.././gdb-10.2/readline/readline/isearch.c:823
#10 0x00000000009a4546 in rl_callback_read_char () at
../.././gdb-10.2/readline/readline/callback.c:164
#11 0x00000000006b563e in gdb_rl_callback_read_char_wrapper_noexcept () at
.././gdb-10.2/gdb/event-top.c:177
#12 0x00000000006b660e in gdb_rl_callback_read_char_wrapper
(client_data=<optimised out>) at .././gdb-10.2/gdb/event-top.c:194
#13 0x00000000006b5580 in stdin_event_handler (error=<optimised out>,
client_data=0x3f27170) at .././gdb-10.2/gdb/event-top.c:516
#14 0x0000000000dc21d5 in gdb_wait_for_event (block=block@entry=1) at
.././gdb-10.2/gdbsupport/event-loop.cc:673
#15 0x0000000000dc22ad in gdb_wait_for_event (block=1) at
.././gdb-10.2/gdbsupport/event-loop.cc:569
#16 gdb_do_one_event () at .././gdb-10.2/gdbsupport/event-loop.cc:215
#17 0x00000000007709e5 in start_event_loop () at .././gdb-10.2/gdb/main.c:356

the second free happens in _rl_scxt_dispose() again but the call stack is
different:

#0  _rl_scxt_dispose (cxt=cxt@entry=0x3f96740, flags=flags@entry=0) at
../.././gdb-10.2/readline/readline/isearch.c:125
#1  0x000000000099aeb9 in _rl_isearch_cleanup (cxt=0x3f96740, r=-1) at
../.././gdb-10.2/readline/readline/isearch.c:768
#2  0x000000000099b1fb in _rl_isearch_callback (cxt=<optimised out>) at
../.././gdb-10.2/readline/readline/isearch.c:827
#3  0x00000000009a4546 in rl_callback_read_char () at
../.././gdb-10.2/readline/readline/callback.c:164
#4  0x00000000006b563e in gdb_rl_callback_read_char_wrapper_noexcept () at
.././gdb-10.2/gdb/event-top.c:177
#5  0x00000000006b660e in gdb_rl_callback_read_char_wrapper
(client_data=<optimised out>) at .././gdb-10.2/gdb/event-top.c:194
#6  0x00000000006b5580 in stdin_event_handler (error=<optimised out>,
client_data=0x3f27170) at .././gdb-10.2/gdb/event-top.c:516
#7  0x0000000000dc21d5 in gdb_wait_for_event (block=block@entry=1) at
.././gdb-10.2/gdbsupport/event-loop.cc:673
#8  0x0000000000dc22ad in gdb_wait_for_event (block=1) at
.././gdb-10.2/gdbsupport/event-loop.cc:569
#9  gdb_do_one_event () at .././gdb-10.2/gdbsupport/event-loop.cc:215
#10 0x00000000007709e5 in start_event_loop () at .././gdb-10.2/gdb/main.c:356


It looks like that as part of the signal handling the isearch context is being
clered up and once the signal handler finishes the search fails (correctly)
but it tries to free its own context again and thus it gets the double free.

I can provide more information if required. I have a GDB recording I can use to
analyze the bug further if needed.

Best Regards

Emiliano

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tui/30056] double free when using reverse-search for a previous command and Ctrl-C

[sam at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30056

Sam James <sam at gentoo dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sam at gentoo dot org

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tui/30056] double free when using reverse-search for a previous command and Ctrl-C

[ssbssa at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30056

Hannes Domani <ssbssa at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ssbssa at sourceware dot org

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tui/30056] double free when using reverse-search for a previous command and Ctrl-C

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30056

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
                 CC|                            |vries at gcc dot gnu.org
   Last reconfirmed|                            |2023-05-23
     Ever confirmed|0                           |1

--- Comment #1 from Tom de Vries <vries at gcc dot gnu.org> ---
This fixes it for me:
...
diff --git a/readline/readline/callback.c b/readline/readline/callback.c
index 93f23d97bc2..e2a97f7cee7 100644
--- a/readline/readline/callback.c
+++ b/readline/readline/callback.c
@@ -163,7 +163,7 @@ rl_callback_read_char (void)
       RL_CHECK_SIGNALS ();
       if  (RL_ISSTATE (RL_STATE_ISEARCH))
        {
-         eof = _rl_isearch_callback (_rl_iscxt);
+         eof = _rl_isearch_callback (&_rl_iscxt);
          if (eof == 0 && (RL_ISSTATE (RL_STATE_ISEARCH) == 0) && RL_ISSTATE
(RL_STATE_INPUTPENDING))
            rl_callback_read_char ();

diff --git a/readline/readline/isearch.c b/readline/readline/isearch.c
index 080ba3cbb9c..9f9b72d3b8e 100644
--- a/readline/readline/isearch.c
+++ b/readline/readline/isearch.c
@@ -877,14 +877,17 @@ rl_search_history (int direction, int invoking_key)
    If _rl_isearch_dispatch finishes searching, this function is responsible
    for turning off RL_STATE_ISEARCH, which it does using _rl_isearch_cleanup.
*/
 int
-_rl_isearch_callback (_rl_search_cxt *cxt)
+_rl_isearch_callback (_rl_search_cxt **cxt)
 {
   int c, r;

-  c = _rl_search_getchar (cxt);
+  c = _rl_search_getchar (*cxt);
+  if (*cxt == 0)
+    return 1;
   /* We might want to handle EOF here */
-  r = _rl_isearch_dispatch (cxt, cxt->lastc);
-
-  return (r <= 0) ? _rl_isearch_cleanup (cxt, r) : 0;
+  r = _rl_isearch_dispatch (*cxt, (*cxt)->lastc);
+  if (*cxt == 0)
+    return 1;
+  return (r <= 0) ? _rl_isearch_cleanup (*cxt, r) : 0;
 }
 #endif
diff --git a/readline/readline/rlprivate.h b/readline/readline/rlprivate.h
index 02838ae21ae..a549ec62607 100644
--- a/readline/readline/rlprivate.h
+++ b/readline/readline/rlprivate.h
@@ -306,7 +306,7 @@ extern _rl_search_cxt *_rl_scxt_alloc PARAMS((int, int));
 extern void _rl_scxt_dispose PARAMS((_rl_search_cxt *, int));

 extern int _rl_isearch_dispatch PARAMS((_rl_search_cxt *, int));
-extern int _rl_isearch_callback PARAMS((_rl_search_cxt *));
+extern int _rl_isearch_callback PARAMS((_rl_search_cxt **));
 extern int _rl_isearch_cleanup PARAMS((_rl_search_cxt *, int));

 extern int _rl_search_getchar PARAMS((_rl_search_cxt *));
...

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tui/30056] double free when using reverse-search for a previous command and Ctrl-C

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30056

--- Comment #2 from Tom de Vries <vries at gcc dot gnu.org> ---
The simplest way of fixing this is:
...
diff --git a/readline/readline/isearch.c b/readline/readline/isearch.c
index 080ba3cbb9c..407c8b61543 100644
--- a/readline/readline/isearch.c
+++ b/readline/readline/isearch.c
@@ -882,7 +882,8 @@ _rl_isearch_callback (_rl_search_cxt *cxt)
   int c, r;

   c = _rl_search_getchar (cxt);
-  /* We might want to handle EOF here */
+  if (c == EOF)
+    return 1;
   r = _rl_isearch_dispatch (cxt, cxt->lastc);

   return (r <= 0) ? _rl_isearch_cleanup (cxt, r) : 0;
...
but the question is if that sometimes skips _rl_isearch_cleanup while it
shouldn't.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tui/30056] double free when using reverse-search for a previous command and Ctrl-C

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30056

--- Comment #3 from Tom de Vries <vries at gcc dot gnu.org> ---
This looks ok:
...
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);

...

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tui/30056] double free when using reverse-search for a previous command and Ctrl-C

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30056

--- Comment #4 from Tom de Vries <vries at gcc dot gnu.org> ---
Alternatively, we can regard this as a multibyte problem:
...
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
     {
...

For "right arrow", tui_getc returns 0 and expects readline to treat it as a
nop.
However, readline treats it as the first part of a multi-byte character.  The
patch above fixes this, and allows it to be recognized as UTF8_SINGLEBYTE.

Consequently, we have a different call stack when ^C hit us, and the problem
doesn't trigger.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tui/30056] double free when using reverse-search for a previous command and Ctrl-C

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30056

--- Comment #5 from Tom de Vries <vries at gcc dot gnu.org> ---
Created attachment 14904
  --> https://sourceware.org/bugzilla/attachment.cgi?id=14904&action=edit
Tentative patch

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tui/30056] double free when using reverse-search for a previous command and Ctrl-C

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30056

--- Comment #6 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to Tom de Vries from comment #5)
> Created attachment 14904 [details]
> Tentative patch

Submitted: https://sourceware.org/pipermail/gdb-patches/2023-May/199785.html

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tui/30056] double free when using reverse-search for a previous command and Ctrl-C

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30056

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |raphael at pdev dot de

--- Comment #7 from Tom de Vries <vries at gcc dot gnu.org> ---
*** Bug 27900 has been marked as a duplicate of this bug. ***

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tui/30056] double free when using reverse-search for a previous command and Ctrl-C

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30056

--- Comment #8 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tom de Vries <vries@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=85f4cf41a852b5983ca436615a019315f6dc7301

commit 85f4cf41a852b5983ca436615a019315f6dc7301
Author: Tom de Vries <tdevries@suse.de>
Date:   Sun May 28 10:17:57 2023 +0200

    [readline] Fix double free in _rl_scxt_dispose

    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.

    Approved-By: Chet Ramey <chet.ramey@case.edu>

    PR tui/30056
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30056

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tui/30056] double free when using reverse-search for a previous command and Ctrl-C

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30056

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED
   Target Milestone|---                         |14.1

--- Comment #9 from Tom de Vries <vries at gcc dot gnu.org> ---
Fixed by commit in the in-repo copy of readline.

The problem is still there for system readlines that miss this patch, but
AFAICT there's nothing we can do about this in gdb.

That is, maybe we could fix half of the problem by not returning '\0' to
readline, but instead call tui_getc_1 in a loop until it returns something not
'\0'.  But the other half of the problem would still exist and cause the same
problem.

-- 
You are receiving this mail because:
You are on the CC list for the bug.