Patchwork Fix double free in tui_source_element

login
register
mail settings
Submitter Bogdan Harjoc
Date Aug. 4, 2019, 5:28 p.m.
Message ID <CAF4+tmqQWAudA-og1vi0cVps+XK0FTWBWpwTuKVyz3u0sip=FQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/33957/
State New
Headers show

Comments

Bogdan Harjoc - Aug. 4, 2019, 5:28 p.m.
To reproduce, cycle a few times between these layouts: no tui, tui
one-window, tui two-windows (including some layout that shows
disassembly).

tui_set_source_content() expands win_info->content, and has to move
tui_source_element items to the new vector storage, destroying the
items in the old storage, and ~tui_source_element() calls xfree on
'line'. Due to a missing copy ctor, items in the new storage have the
old 'line' pointer which eventually gets freed again. Patch is
attached, I added DISABLE_COPY_AND_ASSIGN() in a few more tui classes
to check for more similar issues.

Valgrind output:

==27971== Invalid free() / delete / delete[] / realloc()
void xfree<char>(char*) (common-utils.h:60)
tui_set_source_content(...) (tui-source.c:198)
tui_update_source_window_as_is(...) (tui-winsource.c:95)
tui_show_symtab_source(...) (tui-source.c:221)
tui_update_source_windows_with_addr(...) (tui-winsource.c:152)
tui_set_layout(tui_layout_type) (tui-layout.c:198)

==27971==  Address 0x... is 0 bytes inside a block of size 63 free'd
void xfree<char> (common-utils.h:60)
tui_source_element::~tui_source_element() (tui-data.h:184)
void std::_Destroy<tui_source_element,...) (stl_construct.h:206)
std::vector<tui_source_element,...>::_M_default_append(...)
std::vector<tui_source_element, ...>::resize(...) (stl_vector.h:692)
tui_alloc_source_buffer(...) (tui-winsource.c:693)
tui_set_source_content(...) (tui-source.c:138)
Tom Tromey - Aug. 4, 2019, 9:19 p.m.
>>>>> "Bogdan" == Bogdan Harjoc <harjoc@gmail.com> writes:

Bogdan> To reproduce, cycle a few times between these layouts: no tui, tui
Bogdan> one-window, tui two-windows (including some layout that shows
Bogdan> disassembly).

Bogdan> tui_set_source_content() expands win_info->content, and has to move
Bogdan> tui_source_element items to the new vector storage, destroying the
Bogdan> items in the old storage, and ~tui_source_element() calls xfree on
Bogdan> 'line'. Due to a missing copy ctor, items in the new storage have the
Bogdan> old 'line' pointer which eventually gets freed again. Patch is
Bogdan> attached, I added DISABLE_COPY_AND_ASSIGN() in a few more tui classes
Bogdan> to check for more similar issues.

Thanks.  I already have something like this on my big TUI refactoring
branch, but your patch is better.

It needs a ChangeLog entry.
Also, do you have a copyright assignment in place?

Tom

Patch

diff --git a/gdb/tui/tui-data.h b/gdb/tui/tui-data.h
index 214f728bef..71c1854a8f 100644
--- a/gdb/tui/tui-data.h
+++ b/gdb/tui/tui-data.h
@@ -39,6 +39,8 @@  struct tui_point
 /* Generic window information.  */
 struct tui_gen_win_info
 {
+  DISABLE_COPY_AND_ASSIGN(tui_gen_win_info);
+
 protected:
 
   explicit tui_gen_win_info (enum tui_win_type t)
@@ -183,6 +185,19 @@  struct tui_source_element
     xfree (line);
   }
 
+  tui_source_element (tui_source_element && o):
+    line(o.line),
+    line_or_addr(o.line_or_addr),
+    is_exec_point(o.is_exec_point),
+    break_mode(o.break_mode)
+  {
+    o.line = nullptr;
+  }
+
+  tui_source_element (const tui_source_element & o) = delete;
+  tui_source_element & operator=(tui_source_element && o) = delete;
+  tui_source_element & operator=(const tui_source_element & o) = delete;
+
   char *line = nullptr;
   struct tui_line_or_address line_or_addr;
   bool is_exec_point = false;
@@ -208,6 +223,8 @@  typedef char tui_exec_info_content[TUI_EXECINFO_SIZE];
 
 struct tui_locator_window : public tui_gen_win_info
 {
+  DISABLE_COPY_AND_ASSIGN(tui_locator_window);
+
   tui_locator_window ()
     : tui_gen_win_info (LOCATOR_WIN)
   {
diff --git a/gdb/tui/tui-file.h b/gdb/tui/tui-file.h
index 44d66b56d8..2d86a8454c 100644
--- a/gdb/tui/tui-file.h
+++ b/gdb/tui/tui-file.h
@@ -25,6 +25,8 @@ 
 
 class tui_file : public stdio_file
 {
+  DISABLE_COPY_AND_ASSIGN(tui_file);
+
 public:
   explicit tui_file (FILE *stream);
 
diff --git a/gdb/tui/tui-out.h b/gdb/tui/tui-out.h
index 083094ba75..7f368c21d3 100644
--- a/gdb/tui/tui-out.h
+++ b/gdb/tui/tui-out.h
@@ -26,6 +26,8 @@ 
    window instead of printing the line in the console window.  */
 class tui_ui_out : public cli_ui_out
 {
+  DISABLE_COPY_AND_ASSIGN(tui_ui_out);
+
 public:
 
   explicit tui_ui_out (ui_file *stream);
diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
index 0646729917..89467affa2 100644
--- a/gdb/tui/tui-regs.h
+++ b/gdb/tui/tui-regs.h
@@ -28,6 +28,8 @@ 
 
 struct tui_data_item_window : public tui_gen_win_info
 {
+  DISABLE_COPY_AND_ASSIGN(tui_data_item_window);
+
   tui_data_item_window ()
     : tui_gen_win_info (DATA_ITEM_WIN)
   {
diff --git a/gdb/tui/tui-winsource.h b/gdb/tui/tui-winsource.h
index ec44d1d2c0..b3a282a021 100644
--- a/gdb/tui/tui-winsource.h
+++ b/gdb/tui/tui-winsource.h
@@ -28,6 +28,8 @@ 
 
 struct tui_exec_info_window : public tui_gen_win_info
 {
+  DISABLE_COPY_AND_ASSIGN(tui_exec_info_window);
+
   tui_exec_info_window ()
     : tui_gen_win_info (EXEC_INFO_WIN)
   {