[1/4] tui-disasm: Fix window content buffer overrun

  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
  

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