Message ID | 20230523160431.28769-1-tdevries@suse.de |
---|---|
State | Committed |
Headers |
Return-Path: <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5EFBF3858413 for <patchwork@sourceware.org>; Tue, 23 May 2023 16:05:00 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 5EFBF3858413 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1684857900; bh=8vg38y5+slpHxKHjqaQWKHXalyPqFSdW1u/kzQPfx74=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=amjTZEtOhunsbc9pktJYwtIMdICCD70Trn7IkPZZoLoXQzOINUoyh42v9ILDcBL7B O9+uQHmRERuxzJm0Z/xZZQ/q2OZ/ReD4n+5jEeTIlD2OVDULojAa6PQdNCYOmFB2Ow O5TICyTwab3HGbxxwA3+tpPPfgJF2x2IMVq0lSsk= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 0BF1D3858D35 for <gdb-patches@sourceware.org>; Tue, 23 May 2023 16:04:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0BF1D3858D35 Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 3F5BD21A39; Tue, 23 May 2023 16:04:27 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 27A0613A1A; Tue, 23 May 2023 16:04:27 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id ACwDCAvkbGRdFAAAMHmgww (envelope-from <tdevries@suse.de>); Tue, 23 May 2023 16:04:27 +0000 To: gdb-patches@sourceware.org, bug-readline@gnu.org Subject: [PATCH] [readline] Fix double free in _rl_scxt_dispose Date: Tue, 23 May 2023 18:04:31 +0200 Message-Id: <20230523160431.28769-1-tdevries@suse.de> X-Mailer: git-send-email 2.35.3 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list <gdb-patches.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=subscribe> From: Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> Reply-To: Tom de Vries <tdevries@suse.de> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> |
Series |
[readline] Fix double free in _rl_scxt_dispose
|
|
Commit Message
Tom de Vries
May 23, 2023, 4:04 p.m. UTC
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(-) base-commit: 9196be90bd9572bd09fad63d7e0b2fa199738b90
Comments
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
On 5/24/23 20:13, Andrew Burgess wrote: > Tom de Vries <tdevries@suse.de> writes: > 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. Yes, I send this to both gdb-patches and bug-readline, as you can see in the CC of your reply ;) Thanks, - Tom
On 5/24/23 2:31 PM, Tom de Vries wrote: > On 5/24/23 20:13, Andrew Burgess wrote: >> Tom de Vries <tdevries@suse.de> writes: >> 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. > > Yes, I send this to both gdb-patches and bug-readline, as you can see in > the CC of your reply ;) I got it; I just haven't looked at it yet.
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. Chet
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
On 5/28/23 04:20, Tom de Vries via Gdb-patches wrote: > 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 Hi Tom, ASan sees a double-free in the test: ==144635==ERROR: AddressSanitizer: attempting double-free on 0x60200001ae90 in thread T0: #0 0x7f39ef4dfdc2 in __interceptor_free /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:52 #1 0x7f39ef3cef65 in _rl_scxt_dispose (/usr/lib/libreadline.so.8+0x25f65) (BuildId: 092e91fc4361b0ef94561e3ae03a75f69398acbb) #2 0x7f39ef3d0f5d in _rl_isearch_cleanup (/usr/lib/libreadline.so.8+0x27f5d) (BuildId: 092e91fc4361b0ef94561e3ae03a75f69398acbb) #3 0x7f39ef3e42ec in rl_callback_read_char (/usr/lib/libreadline.so.8+0x3b2ec) (BuildId: 092e91fc4361b0ef94561e3ae03a75f69398acbb) #4 0x5649f96ec632 in gdb_rl_callback_read_char_wrapper_noexcept /home/smarchi/src/binutils-gdb/gdb/event-top.c:192 #5 0x5649f96ec88a in gdb_rl_callback_read_char_wrapper /home/smarchi/src/binutils-gdb/gdb/event-top.c:225 #6 0x5649fafd3641 in stdin_event_handler /home/smarchi/src/binutils-gdb/gdb/ui.c:155 #7 0x5649fb6dbe79 in handle_file_event /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:573 #8 0x5649fb6dc80f in gdb_wait_for_event /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:694 #9 0x5649fb6da468 in gdb_do_one_event(int) /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:264 #10 0x5649f9e61094 in start_event_loop /home/smarchi/src/binutils-gdb/gdb/main.c:412 #11 0x5649f9e615a6 in captured_command_loop /home/smarchi/src/binutils-gdb/gdb/main.c:476 #12 0x5649f9e66b5c in captured_main /home/smarchi/src/binutils-gdb/gdb/main.c:1320 #13 0x5649f9e66c99 in gdb_main(captured_main_args*) /home/smarchi/src/binutils-gdb/gdb/main.c:1339 #14 0x5649f83b758d in main /home/smarchi/src/binutils-gdb/gdb/gdb.c:32 #15 0x7f39eda3984f (/usr/lib/libc.so.6+0x2384f) (BuildId: 2f005a79cd1a8e385972f5a102f16adba414d75e) #16 0x7f39eda39909 in __libc_start_main (/usr/lib/libc.so.6+0x23909) (BuildId: 2f005a79cd1a8e385972f5a102f16adba414d75e) #17 0x5649f83b7354 in _start (/home/smarchi/build/binutils-gdb/gdb/gdb+0xb0f0354) (BuildId: 2bb3933a88a2426705e531a680e7075402ea19f8) 0x60200001ae90 is located 0 bytes inside of 1-byte region [0x60200001ae90,0x60200001ae91) freed by thread T0 here: #0 0x7f39ef4dfdc2 in __interceptor_free /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:52 #1 0x7f39ef3cef65 in _rl_scxt_dispose (/usr/lib/libreadline.so.8+0x25f65) (BuildId: 092e91fc4361b0ef94561e3ae03a75f69398acbb) previously allocated by thread T0 here: #0 0x7f39ef4e1369 in __interceptor_malloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:69 #1 0x5649f865bca2 in xmalloc /home/smarchi/src/binutils-gdb/gdb/alloc.c:57 #2 0x7f39ef3eb6da (/usr/lib/libreadline.so.8+0x426da) (BuildId: 092e91fc4361b0ef94561e3ae03a75f69398acbb) SUMMARY: AddressSanitizer: double-free /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:52 in __interceptor_free Simon
On 5/29/23 18:43, Simon Marchi wrote: > On 5/28/23 04:20, Tom de Vries via Gdb-patches wrote: >> 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 > > Hi Tom, > > ASan sees a double-free in the test: > > ==144635==ERROR: AddressSanitizer: attempting double-free on 0x60200001ae90 in thread T0: > #0 0x7f39ef4dfdc2 in __interceptor_free /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:52 > #1 0x7f39ef3cef65 in _rl_scxt_dispose (/usr/lib/libreadline.so.8+0x25f65) (BuildId: 092e91fc4361b0ef94561e3ae03a75f69398acbb) > #2 0x7f39ef3d0f5d in _rl_isearch_cleanup (/usr/lib/libreadline.so.8+0x27f5d) (BuildId: 092e91fc4361b0ef94561e3ae03a75f69398acbb) > #3 0x7f39ef3e42ec in rl_callback_read_char (/usr/lib/libreadline.so.8+0x3b2ec) (BuildId: 092e91fc4361b0ef94561e3ae03a75f69398acbb) > #4 0x5649f96ec632 in gdb_rl_callback_read_char_wrapper_noexcept /home/smarchi/src/binutils-gdb/gdb/event-top.c:192 > #5 0x5649f96ec88a in gdb_rl_callback_read_char_wrapper /home/smarchi/src/binutils-gdb/gdb/event-top.c:225 > #6 0x5649fafd3641 in stdin_event_handler /home/smarchi/src/binutils-gdb/gdb/ui.c:155 > #7 0x5649fb6dbe79 in handle_file_event /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:573 > #8 0x5649fb6dc80f in gdb_wait_for_event /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:694 > #9 0x5649fb6da468 in gdb_do_one_event(int) /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:264 > #10 0x5649f9e61094 in start_event_loop /home/smarchi/src/binutils-gdb/gdb/main.c:412 > #11 0x5649f9e615a6 in captured_command_loop /home/smarchi/src/binutils-gdb/gdb/main.c:476 > #12 0x5649f9e66b5c in captured_main /home/smarchi/src/binutils-gdb/gdb/main.c:1320 > #13 0x5649f9e66c99 in gdb_main(captured_main_args*) /home/smarchi/src/binutils-gdb/gdb/main.c:1339 > #14 0x5649f83b758d in main /home/smarchi/src/binutils-gdb/gdb/gdb.c:32 > #15 0x7f39eda3984f (/usr/lib/libc.so.6+0x2384f) (BuildId: 2f005a79cd1a8e385972f5a102f16adba414d75e) > #16 0x7f39eda39909 in __libc_start_main (/usr/lib/libc.so.6+0x23909) (BuildId: 2f005a79cd1a8e385972f5a102f16adba414d75e) > #17 0x5649f83b7354 in _start (/home/smarchi/build/binutils-gdb/gdb/gdb+0xb0f0354) (BuildId: 2bb3933a88a2426705e531a680e7075402ea19f8) > > 0x60200001ae90 is located 0 bytes inside of 1-byte region [0x60200001ae90,0x60200001ae91) > freed by thread T0 here: > #0 0x7f39ef4dfdc2 in __interceptor_free /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:52 > #1 0x7f39ef3cef65 in _rl_scxt_dispose (/usr/lib/libreadline.so.8+0x25f65) (BuildId: 092e91fc4361b0ef94561e3ae03a75f69398acbb) > > previously allocated by thread T0 here: > #0 0x7f39ef4e1369 in __interceptor_malloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:69 > #1 0x5649f865bca2 in xmalloc /home/smarchi/src/binutils-gdb/gdb/alloc.c:57 > #2 0x7f39ef3eb6da (/usr/lib/libreadline.so.8+0x426da) (BuildId: 092e91fc4361b0ef94561e3ae03a75f69398acbb) > > SUMMARY: AddressSanitizer: double-free /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:52 in __interceptor_free > Hm, I guess the test-case detects the bug in system readline. I suppose we can add a KFAIL or skip the test-case entirely when using system readline. Thanks, - Tom
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 {