From: Tom de Vries <tdevries@suse.de>
To: chet.ramey@case.edu, gdb-patches@sourceware.org, bug-readline@gnu.org
Subject: Re: [PATCH] [readline] Fix double free in _rl_scxt_dispose
Date: Sun, 28 May 2023 10:20:30 +0200 [thread overview]
Message-ID: <6f27566e-7339-16a1-54d2-3e8317588e9b@suse.de> (raw)
In-Reply-To: <d3e1cef2-d8d6-2ec9-7369-1176f8f3b7c3@case.edu>
[-- Attachment #1: Type: text/plain, Size: 326 bytes --]
On 5/27/23 21:10, Chet Ramey wrote:
> On 5/23/23 12:04 PM, Tom de Vries wrote:
>
>> Both these issues need fixing independently, though after fixing the
>> first we
>> no longer trigger the second.
>
> Thanks for the report. These are both good fixes.
Thanks for the review.
Added test-case and committed.
Thanks,
- Tom
[-- Attachment #2: 0001-readline-Fix-double-free-in-_rl_scxt_dispose.patch --]
[-- Type: text/x-patch, Size: 5716 bytes --]
From a06bc90120aff3c65fe0f276e64d430358928ef9 Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Tue, 23 May 2023 14:34:19 +0200
Subject: [PATCH] [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
---
gdb/testsuite/gdb.tui/pr30056.exp | 52 +++++++++++++++++++++++++++++++
readline/readline/isearch.c | 3 ++
readline/readline/mbutil.c | 2 +-
3 files changed, 56 insertions(+), 1 deletion(-)
create mode 100644 gdb/testsuite/gdb.tui/pr30056.exp
diff --git a/gdb/testsuite/gdb.tui/pr30056.exp b/gdb/testsuite/gdb.tui/pr30056.exp
new file mode 100644
index 00000000000..7a57a5627a8
--- /dev/null
+++ b/gdb/testsuite/gdb.tui/pr30056.exp
@@ -0,0 +1,52 @@
+# Copyright 2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+# Regression test for PR30056.
+
+tuiterm_env
+
+save_vars { env(LC_ALL) } {
+ setenv LC_ALL C.UTF-8
+
+ # Start terminal.
+ Term::clean_restart 24 80
+
+ # Start TUI.
+ if {![Term::enter_tui]} {
+ unsupported "TUI not supported"
+ return
+ }
+
+ # Send "^R", starting reverse-i-search.
+ send_gdb "\022"
+ Term::wait_for_region_contents 0 $Term::_cur_row $Term::_cols 1 \
+ [string_to_regexp "(reverse-i-search)`': "]
+
+ # Send "xyz".
+ send_gdb "xyz"
+ Term::wait_for_region_contents 0 $Term::_cur_row $Term::_cols 1 \
+ [string_to_regexp "(failed reverse-i-search)`xyz': "]
+
+ # Send arrow-right.
+ send_gdb "\033\[C"
+
+ # We need to the arrow-right to be processed by readline, before we
+ # send the following ^C. Waiting 1 ms seems to do that.
+ after 1
+
+ # Send ^C.
+ send_gdb "\003"
+ gdb_assert { [Term::wait_for "Quit"] } "Control-C"
+}
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: 040f24e2eb2b9b46e7337e588244e9eb9a585550
--
2.35.3
next prev parent reply other threads:[~2023-05-28 8:20 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-23 16:04 Tom de Vries
2023-05-24 18:13 ` Andrew Burgess
2023-05-24 18:31 ` Tom de Vries
2023-05-24 18:39 ` Chet Ramey
2023-05-27 19:10 ` Chet Ramey
2023-05-28 8:20 ` Tom de Vries [this message]
2023-05-29 16:43 ` Simon Marchi
2023-05-29 16:50 ` Tom de Vries
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6f27566e-7339-16a1-54d2-3e8317588e9b@suse.de \
--to=tdevries@suse.de \
--cc=bug-readline@gnu.org \
--cc=chet.ramey@case.edu \
--cc=gdb-patches@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).