[PATCH] Fix new-ui tty stream leak

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix new-ui tty stream leak

Pedro Alves-7
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  <[hidden email]>

        * 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(-)

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.
--
2.14.5

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix new-ui tty stream leak

Simon Marchi-4
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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix new-ui tty stream leak

Pedro Alves-7
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