| Message ID | 1478631454-9447-2-git-send-email-arnez@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 <http://gcc.gnu.org/bugs.html> 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 <http://www.gnu.org/licenses/>. > > -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
On Tue, Nov 08 2016, Pedro Alves wrote: [...] > Missing copyright header. Added. > 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 Wow, "mangled name of some 590K characters"... that's impressive. The TUI crash already occurs with much shorter names. On the other hand, IIRC, I had a version of my test with more than that (I think it was over 1MB), and it worked. So I'm not sure why I didn't hit the problem above. [...] >> + fail "compile" > > Shouldn't this be "untested" ? Yeah, probably more appropriate. Changed. [...] > This is OK. Thanks. I pushed the whole patch series with the changes above. > This could probably all be greatly simplified with std::string > and getting rid of the alloca... Also note that the current implementation has a usability issue. With long mangled names, the instructions are no longer visible on the screen, and a lot of scrolling may be required. See also https://sourceware.org/bugzilla/show_bug.cgi?id=15580 Of course, fixing that would be a different topic. -- Andreas
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 @@ +#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))))) + +/* 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 <http://www.gnu.org/licenses/>. -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" + 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';