[1/4] tui-disasm: Fix window content buffer overrun
Commit Message
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
Comments
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
new file mode 100644
@@ -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)
+{
+}
@@ -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\"."
@@ -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';