This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/4] tui-disasm: Fix window content buffer overrun
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