Fix new-ui tty stream leak
Commit Message
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(-)
Comments
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
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
@@ -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 ();
}
@@ -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.