From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 42891 invoked by alias); 8 Nov 2016 19:29:04 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 42873 invoked by uid 89); 8 Nov 2016 19:29:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.8 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=displaying, playing, 1727, 1323 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 08 Nov 2016 19:28:53 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C6BA57F3E1; Tue, 8 Nov 2016 19:28:51 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uA8JSoUB001211; Tue, 8 Nov 2016 14:28:50 -0500 Subject: Re: [PATCH 1/4] tui-disasm: Fix window content buffer overrun To: Andreas Arnez , gdb-patches@sourceware.org References: <1478631454-9447-1-git-send-email-arnez@linux.vnet.ibm.com> <1478631454-9447-2-git-send-email-arnez@linux.vnet.ibm.com> From: Pedro Alves Message-ID: <763f0466-3261-faca-fcc1-da6772c952c8@redhat.com> Date: Tue, 08 Nov 2016 19:29:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1478631454-9447-2-git-send-email-arnez@linux.vnet.ibm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-11/txt/msg00183.txt.bz2 On 11/08/2016 06:56 PM, Andreas Arnez wrote: > A user reported a GDB crash with TUI when trying to debug a function > with a long demangled C++ method name. It turned out that the logic for > displaying the TUI disassembly window has a bug that can cause a buffer > overrun, possibly overwriting GDB-internal data structures. In > particular, the logic performs an unguarded strcpy. > > Another (harmless) bug in tui_alloc_source_buffer causes the buffer to > be two lines longer than needed. This may have made the crash appear > less frequently. > > gdb/ChangeLog: > > * tui/tui-disasm.c (tui_set_disassem_content): Fix line buffer > overrun due to unchecked strcpy. > > gdb/testsuite/ChangeLog: > > * gdb.base/tui-layout.c: New file. > * gdb.base/tui-layout.exp: Use tui-layout.c, to ensure that the > disassembly window contains very long lines. > --- > gdb/testsuite/gdb.base/tui-layout.c | 30 ++++++++++++++++++++++++++++++ > gdb/testsuite/gdb.base/tui-layout.exp | 17 ++++++++++++++--- > gdb/tui/tui-disasm.c | 24 ++++++++++-------------- > 3 files changed, 54 insertions(+), 17 deletions(-) > create mode 100644 gdb/testsuite/gdb.base/tui-layout.c > > diff --git a/gdb/testsuite/gdb.base/tui-layout.c b/gdb/testsuite/gdb.base/tui-layout.c > new file mode 100644 > index 0000000..951bea7 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/tui-layout.c > @@ -0,0 +1,30 @@ Missing copyright header. > +#define LONGER_NAME(x) x ## x > +#define LONGER(x) LONGER_NAME(x) > +#define LONGNAME1 d_this_identifier_of_32_chars_an > +#define LONGNAME2 LONGER (LONGER (LONGER (LONGER (LONGER (LONGNAME1))))) Funny, I was just playing with throwing a large function name at the compiler last week, after seeing: https://sourceware.org/ml/binutils/2016-11/msg00027.html #define STRCAT_1(a) a ## a #define STRCAT(a) STRCAT_1 (a) #define F1 x #define F2 STRCAT (F1) #define F4 STRCAT (F2) #define F8 STRCAT (F4) #define F10 STRCAT (F8) #define F20 STRCAT (F10) #define F40 STRCAT (F20) #define F80 STRCAT (F40) #define F100 STRCAT (F80) #define F200 STRCAT (F100) #define F400 STRCAT (F200) #define F800 STRCAT (F400) #define F1000 STRCAT (F800) #define F2000 STRCAT (F1000) #define F4000 STRCAT (F2000) #define F8000 STRCAT (F4000) #define F10000 STRCAT (F8000) #define F20000 STRCAT (F10000) #define F40000 STRCAT (F20000) #define F80000 STRCAT (F40000) #define F100000 STRCAT (F80000) #define F200000 STRCAT (F100000) #define F400000 STRCAT (F200000) #define F800000 STRCAT (F400000) #define F1000000 STRCAT (F800000) #define F2000000 STRCAT (F1000000) #define F4000000 STRCAT (F2000000) #define F8000000 STRCAT (F4000000) #define F10000000 STRCAT (F8000000) #define F20000000 STRCAT (F10000000) #define F40000000 STRCAT (F20000000) #define F80000000 STRCAT (F40000000) #define bigfunctionname F80000000 void bigfunctionname () { } int main () { bigfunctionname (); bigfunctionname (); bigfunctionname (); bigfunctionname (); bigfunctionname (); bigfunctionname (); return 0; } $ /opt/gcc/bin/gcc -v [...] gcc version 7.0.0 20161024 (experimental) (GCC) $ /opt/gcc/bin/gcc big.c -o big gcc: internal compiler error: Segmentation fault (program cc1) Please submit a full bug report, with preprocessed source if appropriate. See for instructions. Yay! :-) > + > +/* Construct a long identifier name. If SHORT_IDENTIFIERS is set, limit > + it to 1024 chars. */ > + > +#ifdef SHORT_IDENTIFIERS > +#define LONGNAME3 LONGNAME2 > +#else > +#define LONGNAME3 LONGER (LONGER (LONGER (LONGER (LONGER (LONGNAME2))))) > +#endif > + > +void LONGNAME3 (void); > + > +int > +main () > +{ > + LONGNAME3 (); > + return 0; > +} > + > +/* Function with a long name. Placing it after main makes it more likely > + to be shown in the disassembly window on startup. */ > + > +void > +LONGNAME3 (void) > +{ > +} > diff --git a/gdb/testsuite/gdb.base/tui-layout.exp b/gdb/testsuite/gdb.base/tui-layout.exp > index 43b3a4f..0c7169e 100644 > --- a/gdb/testsuite/gdb.base/tui-layout.exp > +++ b/gdb/testsuite/gdb.base/tui-layout.exp > @@ -13,12 +13,23 @@ > # You should have received a copy of the GNU General Public License > # along with this program. If not, see . > > -standard_testfile start.c > +standard_testfile > > -if { [prepare_for_testing ${testfile}.exp ${testfile} $srcfile] } { > - return -1 > +set ccopts {debug quiet} > +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "$binfile" \ > + executable $ccopts] != "" } { > + # Maybe the compiler can't handle arbitrarily long identfier names. > + # Try with a shorter version. > + lappend ccopts "additional_flags=-DSHORT_IDENTIFIERS" > + if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "$binfile" \ > + executable $ccopts] != "" } { > + fail "compile" Shouldn't this be "untested" ? > + return -1 > + } > } > > +clean_restart "$binfile" > + > if {[skip_tui_tests]} { > # TUI support is disabled. Check for error message. > gdb_test "layout asm" "Undefined command: \"layout\". Try \"help\"." > diff --git a/gdb/tui/tui-disasm.c b/gdb/tui/tui-disasm.c > index 29c1443..5368aa4 100644 > --- a/gdb/tui/tui-disasm.c > +++ b/gdb/tui/tui-disasm.c > @@ -172,7 +172,7 @@ tui_set_disassem_content (struct gdbarch *gdbarch, CORE_ADDR pc) > enum tui_status ret = TUI_FAILURE; > int i; > int offset = TUI_DISASM_WIN->detail.source_info.horizontal_offset; > - int max_lines; > + int max_lines, line_width; > CORE_ADDR cur_pc; > struct tui_gen_win_info *locator = tui_locator_win_info_ptr (); > int tab_len = tui_default_tab_len (); > @@ -193,8 +193,9 @@ tui_set_disassem_content (struct gdbarch *gdbarch, CORE_ADDR pc) > TUI_DISASM_WIN->detail.source_info.start_line_or_addr.u.addr = pc; > cur_pc = locator->content[0]->which_element.locator.addr; > > - max_lines = TUI_DISASM_WIN->generic.height - 2; /* Account for > - hilite. */ > + /* Window size, excluding highlight box. */ > + max_lines = TUI_DISASM_WIN->generic.height - 2; > + line_width = TUI_DISASM_WIN->generic.width - 2; > > /* Get temporary table that will hold all strings (addr & insn). */ > asm_lines = XALLOCAVEC (struct tui_asm_line, max_lines); > @@ -233,20 +234,15 @@ tui_set_disassem_content (struct gdbarch *gdbarch, CORE_ADDR pc) > src = &element->which_element.source; > strcpy (line, asm_lines[i].addr_string); > cur_len = strlen (line); > - > - /* Add spaces to make the instructions start on the same > - column. */ > - while (cur_len < insn_pos) > - { > - strcat (line, " "); > - cur_len++; > - } > - > - strcat (line, asm_lines[i].insn); > + memset (line + cur_len, ' ', insn_pos - cur_len); > + strcpy (line + insn_pos, asm_lines[i].insn); > > /* Now copy the line taking the offset into account. */ > if (strlen (line) > offset) > - strcpy (src->line, &line[offset]); > + { > + strncpy (src->line, &line[offset], line_width); > + src->line[line_width] = '\0'; > + } > else > src->line[0] = '\0'; > > This is OK. This could probably all be greatly simplified with std::string and getting rid of the alloca... Thanks, Pedro Alves