Fix new-ui tty stream leak

Message ID 20190718184124.21907-1-palves@redhat.com
State New
Headers show
Series
  • Fix new-ui tty stream leak
Related show

Commit Message

Pedro Alves July 18, 2019, 6:41 p.m.
I noticed that we never close the stream opened by new-ui.

This fixes it, by adding a new struct ui ctor that takes ownership of
the passed-in stream.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* top.c (ui::ui (FILE*, FILE*, FILE*)): Use ui::init.
	(ui::ui(gdb_file_up&&)): New.
	(ui::init): New, factored out from 'ui::ui(FILE*, FILE*, FILE*)'.
	(ui::~ui): Close streams, if they're owned.
	(new_ui_command): Use ui::ui(gdb_file_up&&)) ctor.
	* top.h: Include "gdbsupport/filestuff.h".
	(struct ui): In-class-initialize some fields.
	(ui::ui(gdb_file_up&&)): Declare.
	(ui::init): Declare.
---
 gdb/top.c | 59 ++++++++++++++++++++++++++++++++++++-----------------------
 gdb/top.h | 36 ++++++++++++++++++++++++++----------
 2 files changed, 62 insertions(+), 33 deletions(-)

-- 
2.14.5

Comments

Simon Marchi July 18, 2019, 8:09 p.m. | #1
On 2019-07-18 2:41 p.m., Pedro Alves wrote:
> I noticed that we never close the stream opened by new-ui.

> 

> This fixes it, by adding a new struct ui ctor that takes ownership of

> the passed-in stream.


Hi Pedro,

If I am not mistaken, we never delete the UIs themselves either, so it won't
make a difference today, is that right?  They just live until the end of the
GDB process.

Still, I think this patch is fine, in that it makes things right in case we
decide to make it possible to close an existing UI some day.

Simon
Pedro Alves July 18, 2019, 8:53 p.m. | #2
On 7/18/19 9:09 PM, Simon Marchi wrote:
> On 2019-07-18 2:41 p.m., Pedro Alves wrote:

>> I noticed that we never close the stream opened by new-ui.

>>

>> This fixes it, by adding a new struct ui ctor that takes ownership of

>> the passed-in stream.

> 

> Hi Pedro,

> 

> If I am not mistaken, we never delete the UIs themselves either, so it won't

> make a difference today, is that right?  They just live until the end of the

> GDB process.


We actually delete them if the tty closes.  See stdin_event_handler.

E.g., spawn an instance of KDE's console that just runs "tty" to print
the terminal device:

 $ konsole --hold -e tty&

Run a new-ui on that terminal:

 $ gdb -ex "new-ui console /dev/pts/36"
 ...
 New UI allocated

Now close the konsole window, and back in the main console, GDB prints:

 Error detected on fd 13

> Still, I think this patch is fine, in that it makes things right in case we

> decide to make it possible to close an existing UI some day.

Some day is here.  :-)

Thanks,
Pedro Alves

Patch

diff --git a/gdb/top.c b/gdb/top.c
index 60f81b3bf85..4e102777409 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -260,27 +260,38 @@  static int highest_ui_num;
 /* See top.h.  */
 
 ui::ui (FILE *instream_, FILE *outstream_, FILE *errstream_)
-  : next (nullptr),
-    num (++highest_ui_num),
-    call_readline (nullptr),
-    input_handler (nullptr),
-    command_editing (0),
-    interp_info (nullptr),
-    async (0),
-    secondary_prompt_depth (0),
-    stdin_stream (instream_),
-    instream (instream_),
+  : instream (instream_),
     outstream (outstream_),
     errstream (errstream_),
-    input_fd (fileno (instream)),
-    input_interactive_p (ISATTY (instream)),
-    prompt_state (PROMPT_NEEDED),
-    m_gdb_stdout (new stdio_file (outstream)),
-    m_gdb_stdin (new stdio_file (instream)),
-    m_gdb_stderr (new stderr_file (errstream)),
-    m_gdb_stdlog (m_gdb_stderr),
-    m_current_uiout (nullptr)
+    m_owns_streams (false)
 {
+  init ();
+}
+
+ui::ui (gdb_file_up &&tty)
+  : instream (tty.get ()),
+    outstream (tty.get ()),
+    errstream (tty.get ()),
+    m_owns_streams (true)
+{
+  tty.release ();
+
+  init ();
+}
+
+void
+ui::init ()
+{
+  num = ++highest_ui_num;
+  stdin_stream = instream;
+  input_fd = fileno (instream);
+  input_interactive_p = ISATTY (instream);
+
+  m_gdb_stdout = new stdio_file (outstream);
+  m_gdb_stdin = new stdio_file (instream);
+  m_gdb_stderr = new stderr_file (errstream);
+  m_gdb_stdlog = m_gdb_stderr;
+
   buffer_init (&line_buffer);
 
   if (ui_list == NULL)
@@ -315,6 +326,12 @@  ui::~ui ()
   delete m_gdb_stdin;
   delete m_gdb_stdout;
   delete m_gdb_stderr;
+
+  if (m_owns_streams)
+    {
+      gdb_assert (instream == outstream && outstream == errstream);
+      fclose (instream);
+    }
 }
 
 /* Open file named NAME for read/write, making sure not to make it the
@@ -360,8 +377,7 @@  new_ui_command (const char *args, int from_tty)
        with Windows named pipes.  */
     gdb_file_up stream = open_terminal_stream (tty_name);
 
-    std::unique_ptr<ui> ui
-      (new struct ui (stream.get (), stream.get (), stream.get ()));
+    std::unique_ptr<ui> ui (new struct ui (std::move (stream)));
 
     ui->async = 1;
 
@@ -371,9 +387,6 @@  new_ui_command (const char *args, int from_tty)
 
     interp_pre_command_loop (top_level_interpreter ());
 
-    /* Make sure the file is not closed.  */
-    stream.release ();
-
     ui.release ();
   }
 
diff --git a/gdb/top.h b/gdb/top.h
index 32a898b82f5..cef8f851eda 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -21,6 +21,7 @@ 
 #define TOP_H
 
 #include "gdbsupport/buffer.h"
+#include "gdbsupport/filestuff.h"
 #include "event-loop.h"
 #include "value.h"
 
@@ -55,14 +56,20 @@  enum prompt_state
 
 struct ui
 {
-  /* Create a new UI.  */
+  /* Create a new UI.  Does not take ownership of the
+     files/streams.  */
   ui (FILE *instream, FILE *outstream, FILE *errstream);
+
+  /* Create a new UI, taking ownership of TTY and using it for all of
+     stdin/stdout/stderr.  */
+  explicit ui (gdb_file_up &&tty);
+
   ~ui ();
 
   DISABLE_COPY_AND_ASSIGN (ui);
 
   /* Pointer to next in singly-linked list.  */
-  struct ui *next;
+  struct ui *next = nullptr;
 
   /* Convenient handle (UI number).  Unique across all UIs.  */
   int num;
@@ -77,19 +84,19 @@  struct ui
      point of invocation.  In the special case in which the character
      read is newline, the function invokes the INPUT_HANDLER callback
      (see below).  */
-  void (*call_readline) (gdb_client_data);
+  void (*call_readline) (gdb_client_data) = nullptr;
 
   /* The function to invoke when a complete line of input is ready for
      processing.  */
-  void (*input_handler) (gdb::unique_xmalloc_ptr<char> &&);
+  void (*input_handler) (gdb::unique_xmalloc_ptr<char> &&) = nullptr;
 
   /* True if this UI is using the readline library for command
      editing; false if using GDB's own simple readline emulation, with
      no editing support.  */
-  int command_editing;
+  int command_editing = 0;
 
   /* Each UI has its own independent set of interpreters.  */
-  struct ui_interp_info *interp_info;
+  struct ui_interp_info *interp_info = nullptr;
 
   /* True if the UI is in async mode, false if in sync mode.  If in
      sync mode, a synchronous execution command (e.g, "next") does not
@@ -99,11 +106,11 @@  struct ui
      the top event loop.  For the main UI, this starts out disabled,
      until all the explicit command line arguments (e.g., `gdb -ex
      "start" -ex "next"') are processed.  */
-  int async;
+  int async = 0;
 
   /* The number of nested readline secondary prompts that are
      currently active.  */
-  int secondary_prompt_depth;
+  int secondary_prompt_depth = 0;
 
   /* The UI's stdin.  Set to stdin for the main UI.  */
   FILE *stdin_stream;
@@ -128,7 +135,7 @@  struct ui
   int input_interactive_p;
 
   /* See enum prompt_state's description.  */
-  enum prompt_state prompt_state;
+  enum prompt_state prompt_state = PROMPT_NEEDED;
 
   /* The fields below that start with "m_" are "private".  They're
      meant to be accessed through wrapper macros that make them look
@@ -148,7 +155,16 @@  struct ui
   struct ui_file *m_gdb_stdlog;
 
   /* The current ui_out.  */
-  struct ui_out *m_current_uiout;
+  struct ui_out *m_current_uiout = nullptr;
+
+private:
+  /* Shared initialization, called by all constructors.  */
+  void init ();
+
+  /* True if INSTREAM/OUTSTREAM/ERRSTREAM must be closed on
+     destruction.  If true, assumes INSTREAM/OUTSTREAM/ERRSTREAM all
+     point to the same file.  */
+  bool m_owns_streams;
 };
 
 /* The main UI.  This is the UI that is bound to stdin/stdout/stderr.