[PATCH RESEND] gdb: remove static buffer in command_line_input

[PATCH RESEND] gdb: remove static buffer in command_line_input

[Simon Marchi]

[I sent this earlier today, but I don't see it in the archives.
Resending it through a different computer / SMTP.]

The use of the static buffer in command_line_input is becoming
problematic, as explained here [1].  In short, with this patch [2] that
attempt to fix a post-hook bug, when running gdb.base/commands.exp, we
hit a case where we read a "define" command line from a script file
using command_command_line_input.  The command line is stored in
command_line_input's static buffer.  Inside the define command's
execution, we read the lines inside the define using command_line_input,
which overwrites the define command, in command_line_input's static
buffer.  After the execution of the define command, execute_command does
a command look up to see if a post-hook is registered.  For that, it
uses a now stale pointer that used to point to the define command, in
the static buffer, causing a use-after-free.  Note that the pointer in
execute_command points to the dynamically-allocated buffer help by the
static buffer in command_line_input, not to the static object itself,
hence why we see a use-after-free.

Fix that by removing the static buffer.  I initially changed
command_line_input and other related functions to return an std::string,
which is the obvious but naive solution.  The thing is that some callees
don't need to return an allocated string, so this this an unnecessary
pessimization.  I changed it to passing in a reference to an std::string
buffer, which the callee can use if it needs to return
dynamically-allocated content.  It fills the buffer and returns a
pointers to the C string inside.  The callees that don't need to return
dynamically-allocated content simply don't use it.

So, it started with modifying command_line_input as described above, all
the other changes derive directly from that.

One slightly shady thing is in handle_line_of_input, where we now pass a
pointer to an std::string's internal buffer to readline's history_value
function, which takes a `char *`.  I'm pretty sure that this function
does not modify the input string, because I was able to change it (with
enough massaging) to take a `const char *`.

A subtle change is that we now clear a UI's line buffer using a
SCOPE_EXIT in command_line_handler, after executing the command.
This was previously done by this line in handle_line_of_input:

  /* We have a complete command line now.  Prepare for the next
     command, but leave ownership of memory to the buffer .  */
  cmd_line_buffer->used_size = 0;

I think the new way is clearer.

[1] https://inbox.sourceware.org/gdb-patches/becb8438-81ef-8ad8-cc42-fcbfaea8cddd@simark.ca/
[2] https://inbox.sourceware.org/gdb-patches/20221213112241.621889-1-jan.vrany@labware.com/

Change-Id: I8fc89b1c69870c7fc7ad9c1705724bd493596300
---
 gdb/ada-lang.c               |   3 +-
 gdb/breakpoint.c             |   4 +-
 gdb/cli/cli-script.c         |  19 +++---
 gdb/cli/cli-script.h         |  11 +++-
 gdb/defs.h                   |   3 +-
 gdb/event-top.c              | 110 ++++++++++++++++-------------------
 gdb/linespec.c               |   4 +-
 gdb/mi/mi-cmd-break.c        |   2 +-
 gdb/python/py-breakpoint.c   |   2 +-
 gdb/python/py-gdb-readline.c |   3 +-
 gdb/python/python.c          |   2 +-
 gdb/top.c                    |  39 +++++--------
 gdb/top.h                    |   8 +--
 13 files changed, 102 insertions(+), 108 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 5054bdb29572..d59a5ae576e9 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -3561,7 +3561,8 @@ get_selections (int *choices, int n_choices, int max_results,
   if (prompt == NULL)
     prompt = "> ";
 
-  args = command_line_input (prompt, annotation_suffix);
+  std::string buffer;
+  args = command_line_input (buffer, prompt, annotation_suffix);
 
   if (args == NULL)
     error_no_arg (_("one or more choice numbers"));
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f0276a963c00..7a245b6fb5f9 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -13782,8 +13782,8 @@ strace_command (const char *arg, int from_tty)
 static struct uploaded_tp *this_utp;
 static int next_cmd;
 
-static char *
-read_uploaded_action (void)
+static const char *
+read_uploaded_action (std::string &buffer)
 {
   char *rslt = nullptr;
 
diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index 2101d6fface9..88afd1a82aad 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -44,7 +44,7 @@
 
 static enum command_control_type
 recurse_read_control_structure
-    (gdb::function_view<const char * ()> read_next_line_func,
+    (read_next_line_ftype read_next_line_func,
      struct command_line *current_cmd,
      gdb::function_view<void (const char *)> validator);
 
@@ -54,7 +54,7 @@ static void do_define_command (const char *comname, int from_tty,
 static void do_document_command (const char *comname, int from_tty,
                                  const counted_command_line *commands);
 
-static const char *read_next_line ();
+static const char *read_next_line (std::string &buffer);
 
 /* Level of control structure when reading.  */
 static int control_level;
@@ -894,7 +894,7 @@ user_args::insert_args (const char *line) const
    from stdin.  */
 
 static const char *
-read_next_line ()
+read_next_line (std::string &buffer)
 {
   struct ui *ui = current_ui;
   char *prompt_ptr, control_prompt[256];
@@ -917,7 +917,7 @@ read_next_line ()
   else
     prompt_ptr = NULL;
 
-  return command_line_input (prompt_ptr, "commands");
+  return command_line_input (buffer, prompt_ptr, "commands");
 }
 
 /* Given an input line P, skip the command and return a pointer to the
@@ -1064,7 +1064,7 @@ process_next_line (const char *p, command_line_up *command,
    obtain lines of the command.  */
 
 static enum command_control_type
-recurse_read_control_structure (gdb::function_view<const char * ()> read_next_line_func,
+recurse_read_control_structure (read_next_line_ftype read_next_line_func,
 				struct command_line *current_cmd,
 				gdb::function_view<void (const char *)> validator)
 {
@@ -1085,8 +1085,9 @@ recurse_read_control_structure (gdb::function_view<const char * ()> read_next_li
     {
       dont_repeat ();
 
+      std::string buffer;
       next = nullptr;
-      val = process_next_line (read_next_line_func (), &next,
+      val = process_next_line (read_next_line_func (buffer), &next,
 			       current_cmd->control_type != python_control
 			       && current_cmd->control_type != guile_control
 			       && current_cmd->control_type != compile_control,
@@ -1215,7 +1216,7 @@ read_command_lines (const char *prompt_arg, int from_tty, int parse_commands,
    obtained using READ_NEXT_LINE_FUNC.  */
 
 counted_command_line
-read_command_lines_1 (gdb::function_view<const char * ()> read_next_line_func,
+read_command_lines_1 (read_next_line_ftype read_next_line_func,
 		      int parse_commands,
 		      gdb::function_view<void (const char *)> validator)
 {
@@ -1231,7 +1232,9 @@ read_command_lines_1 (gdb::function_view<const char * ()> read_next_line_func,
   while (1)
     {
       dont_repeat ();
-      val = process_next_line (read_next_line_func (), &next, parse_commands,
+
+      std::string buffer;
+      val = process_next_line (read_next_line_func (buffer), &next, parse_commands,
 			       validator);
 
       /* Ignore blank lines or comments.  */
diff --git a/gdb/cli/cli-script.h b/gdb/cli/cli-script.h
index 2b9483f115c0..d9ca7de5128f 100644
--- a/gdb/cli/cli-script.h
+++ b/gdb/cli/cli-script.h
@@ -112,11 +112,18 @@ struct command_line
   }
 };
 
+/* Prototype for a function to call to get one more input line.
+
+   If the function needs to return a dynamically allocated string, it can place
+   in the passed-in buffer, and return a pointer to it.  Otherwise, it can
+   simply ignore it.  */
+
+using read_next_line_ftype = gdb::function_view<const char * (std::string &)>;
+
 extern counted_command_line read_command_lines
     (const char *, int, int, gdb::function_view<void (const char *)>);
 extern counted_command_line read_command_lines_1
-    (gdb::function_view<const char * ()>, int,
-     gdb::function_view<void (const char *)>);
+    (read_next_line_ftype, int, gdb::function_view<void (const char *)>);
 
 
 /* Exported to cli/cli-cmds.c */
diff --git a/gdb/defs.h b/gdb/defs.h
index f51ab9e5c0c3..4771d02a92a4 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -316,7 +316,8 @@ typedef void initialize_file_ftype (void);
 
 extern char *gdb_readline_wrapper (const char *);
 
-extern const char *command_line_input (const char *, const char *);
+extern const char *command_line_input (std::string &cmd_line_buffer,
+				       const char *, const char *);
 
 extern void print_prompt (void);
 
diff --git a/gdb/event-top.c b/gdb/event-top.c
index bcf80bbd7d0b..fa86a89e4a19 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -483,13 +483,13 @@ struct ui *main_ui;
 struct ui *current_ui;
 struct ui *ui_list;
 
-/* Get a pointer to the current UI's line buffer.  This is used to
+/* Get a reference to the current UI's line buffer.  This is used to
    construct a whole line of input from partial input.  */
 
-static struct buffer *
+static std::string &
 get_command_line_buffer (void)
 {
-  return &current_ui->line_buffer;
+  return current_ui->line_buffer;
 }
 
 /* When there is an event ready on the stdin file descriptor, instead
@@ -620,46 +620,41 @@ command_handler (const char *command)
     }
 }
 
-/* Append RL, an input line returned by readline or one of its
-   emulations, to CMD_LINE_BUFFER.  Returns the command line if we
-   have a whole command line ready to be processed by the command
-   interpreter or NULL if the command line isn't complete yet (input
-   line ends in a backslash).  */
+/* Append RL, an input line returned by readline or one of its emulations, to
+   CMD_LINE_BUFFER.  Return true if we have a whole command line ready to be
+   processed by the command interpreter or false if the command line isn't
+   complete yet (input line ends in a backslash).  */
 
-static char *
-command_line_append_input_line (struct buffer *cmd_line_buffer, const char *rl)
+static bool
+command_line_append_input_line (std::string &cmd_line_buffer, const char *rl)
 {
-  char *cmd;
-  size_t len;
-
-  len = strlen (rl);
+  size_t len = strlen (rl);
 
   if (len > 0 && rl[len - 1] == '\\')
     {
       /* Don't copy the backslash and wait for more.  */
-      buffer_grow (cmd_line_buffer, rl, len - 1);
-      cmd = NULL;
+      cmd_line_buffer.append (rl, len - 1);
+      return false;
     }
   else
     {
       /* Copy whole line including terminating null, and we're
 	 done.  */
-      buffer_grow (cmd_line_buffer, rl, len + 1);
-      cmd = cmd_line_buffer->buffer;
+      cmd_line_buffer.append (rl, len + 1);
+      return true;
     }
-
-  return cmd;
 }
 
 /* Handle a line of input coming from readline.
 
-   If the read line ends with a continuation character (backslash),
-   save the partial input in CMD_LINE_BUFFER (except the backslash),
-   and return NULL.  Otherwise, save the partial input and return a
-   pointer to CMD_LINE_BUFFER's buffer (null terminated), indicating a
-   whole command line is ready to be executed.
+   If the read line ends with a continuation character (backslash), return
+   nullptr.  Otherwise, return a pointer to the command line, indicating a whole
+   command line is ready to be executed.
 
-   Returns EOF on end of file.
+   The returned pointer may or may not point to CMD_LINE_BUFFER's internal
+   buffer.
+
+   Return EOF on end of file.
 
    If REPEAT, handle command repetitions:
 
@@ -670,38 +665,32 @@ command_line_append_input_line (struct buffer *cmd_line_buffer, const char *rl)
        command instead of the empty input line.
 */
 
-char *
-handle_line_of_input (struct buffer *cmd_line_buffer,
+const char *
+handle_line_of_input (std::string &cmd_line_buffer,
 		      const char *rl, int repeat,
 		      const char *annotation_suffix)
 {
   struct ui *ui = current_ui;
   int from_tty = ui->instream == ui->stdin_stream;
-  char *p1;
-  char *cmd;
 
   if (rl == NULL)
     return (char *) EOF;
 
-  cmd = command_line_append_input_line (cmd_line_buffer, rl);
-  if (cmd == NULL)
+  bool complete = command_line_append_input_line (cmd_line_buffer, rl);
+  if (!complete)
     return NULL;
 
-  /* We have a complete command line now.  Prepare for the next
-     command, but leave ownership of memory to the buffer .  */
-  cmd_line_buffer->used_size = 0;
-
   if (from_tty && annotation_level > 1)
     printf_unfiltered (("\n\032\032post-%s\n"), annotation_suffix);
 
 #define SERVER_COMMAND_PREFIX "server "
-  server_command = startswith (cmd, SERVER_COMMAND_PREFIX);
+  server_command = startswith (cmd_line_buffer.c_str (), SERVER_COMMAND_PREFIX);
   if (server_command)
     {
       /* Note that we don't call `save_command_line'.  Between this
 	 and the check in dont_repeat, this insures that repeating
 	 will still do the right thing.  */
-      return cmd + strlen (SERVER_COMMAND_PREFIX);
+      return cmd_line_buffer.c_str () + strlen (SERVER_COMMAND_PREFIX);
     }
 
   /* Do history expansion if that is wished.  */
@@ -710,32 +699,29 @@ handle_line_of_input (struct buffer *cmd_line_buffer,
       char *cmd_expansion;
       int expanded;
 
-      expanded = history_expand (cmd, &cmd_expansion);
+      /* Note: here, we pass a pointer to the std::string's internal buffer as
+	 a `char *`.  At the time of writing, readline's history_expand does
+	 not modify the passed-in string.  Ideally, readline should be modified
+	 to make that parameter `const char *`.  */
+      expanded = history_expand (&cmd_line_buffer[0], &cmd_expansion);
       gdb::unique_xmalloc_ptr<char> history_value (cmd_expansion);
       if (expanded)
 	{
-	  size_t len;
-
 	  /* Print the changes.  */
 	  printf_unfiltered ("%s\n", history_value.get ());
 
 	  /* If there was an error, call this function again.  */
 	  if (expanded < 0)
-	    return cmd;
-
-	  /* history_expand returns an allocated string.  Just replace
-	     our buffer with it.  */
-	  len = strlen (history_value.get ());
-	  xfree (buffer_finish (cmd_line_buffer));
-	  cmd_line_buffer->buffer = history_value.get ();
-	  cmd_line_buffer->buffer_size = len + 1;
-	  cmd = history_value.release ();
+	    return cmd_line_buffer.c_str ();
+
+	  cmd_line_buffer = history_value.get ();
 	}
     }
 
   /* If we just got an empty line, and that is supposed to repeat the
      previous command, return the previously saved command.  */
-  for (p1 = cmd; *p1 == ' ' || *p1 == '\t'; p1++)
+  const char *p1;
+  for (p1 = cmd_line_buffer.c_str (); *p1 == ' ' || *p1 == '\t'; p1++)
     ;
   if (repeat && *p1 == '\0')
     return get_saved_command_line ();
@@ -747,17 +733,21 @@ handle_line_of_input (struct buffer *cmd_line_buffer,
      and then later fetch it from the value history and remove the
      '#'.  The kill ring is probably better, but some people are in
      the habit of commenting things out.  */
-  if (*cmd != '\0' && from_tty && current_ui->input_interactive_p ())
-    gdb_add_history (cmd);
+  if (cmd_line_buffer[0] != '\0' && from_tty && current_ui->input_interactive_p ())
+    gdb_add_history (cmd_line_buffer.c_str ());
 
   /* Save into global buffer if appropriate.  */
   if (repeat)
     {
-      save_command_line (cmd);
+      save_command_line (cmd_line_buffer.c_str ());
+
+      /* It is important that we return a pointer to the saved command line
+	 here, for the `cmd_start == saved_command_line` check in
+	 execute_command to work.  */
       return get_saved_command_line ();
     }
-  else
-    return cmd;
+
+  return cmd_line_buffer.c_str ();
 }
 
 /* See event-top.h.  */
@@ -790,11 +780,10 @@ gdb_rl_deprep_term_function (void)
 void
 command_line_handler (gdb::unique_xmalloc_ptr<char> &&rl)
 {
-  struct buffer *line_buffer = get_command_line_buffer ();
+  std::string &line_buffer = get_command_line_buffer ();
   struct ui *ui = current_ui;
-  char *cmd;
 
-  cmd = handle_line_of_input (line_buffer, rl.get (), 1, "prompt");
+  const char *cmd = handle_line_of_input (line_buffer, rl.get (), 1, "prompt");
   if (cmd == (char *) EOF)
     {
       /* stdin closed.  The connection with the terminal is gone.
@@ -857,6 +846,9 @@ command_line_handler (gdb::unique_xmalloc_ptr<char> &&rl)
     {
       ui->prompt_state = PROMPT_NEEDED;
 
+      /* Ensure the UI's line buffer is empty for the next command.  */
+      SCOPE_EXIT { line_buffer.clear (); };
+
       command_handler (cmd);
 
       if (ui->prompt_state != PROMPTED)
diff --git a/gdb/linespec.c b/gdb/linespec.c
index 3db35998f7e1..5963823603fd 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -1501,7 +1501,9 @@ decode_line_2 (struct linespec_state *self,
     {
       prompt = "> ";
     }
-  args = command_line_input (prompt, "overload-choice");
+
+  std::string buffer;
+  args = command_line_input (buffer, prompt, "overload-choice");
 
   if (args == 0 || *args == 0)
     error_no_arg (_("one or more choice numbers"));
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 408f582ebc5e..777ca19af4db 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -564,7 +564,7 @@ mi_cmd_break_commands (const char *command, char **argv, int argc)
 
   int count = 1;
   auto reader
-    = [&] ()
+    = [&] (std::string &buffer)
       {
 	const char *result = nullptr;
 	if (count < argc)
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 63b18bd0f92a..de7b9f4266bf 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -582,7 +582,7 @@ bppy_set_commands (PyObject *self, PyObject *newvalue, void *closure)
       bool first = true;
       char *save_ptr = nullptr;
       auto reader
-	= [&] ()
+	= [&] (std::string &buffer)
 	  {
 	    const char *result = strtok_r (first ? commands.get () : nullptr,
 					   "\n", &save_ptr);
diff --git a/gdb/python/py-gdb-readline.c b/gdb/python/py-gdb-readline.c
index af388d5ed72e..c82f1c81ea70 100644
--- a/gdb/python/py-gdb-readline.c
+++ b/gdb/python/py-gdb-readline.c
@@ -38,11 +38,12 @@ gdbpy_readline_wrapper (FILE *sys_stdin, FILE *sys_stdout,
 {
   int n;
   const char *p = NULL;
+  std::string buffer;
   char *q;
 
   try
     {
-      p = command_line_input (prompt, "python");
+      p = command_line_input (buffer, prompt, "python");
     }
   /* Handle errors by raising Python exceptions.  */
   catch (const gdb_exception &except)
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 29f2010ee8e7..4aa24421dece 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -644,7 +644,7 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
       bool first = true;
       char *save_ptr = nullptr;
       auto reader
-	= [&] ()
+	= [&] (std::string &buffer)
 	  {
 	    const char *result = strtok_r (first ? &arg_copy[0] : nullptr,
 					   "\n", &save_ptr);
diff --git a/gdb/top.c b/gdb/top.c
index e0e7e48cf7ca..45e65beb3b39 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -307,8 +307,6 @@ ui::ui (FILE *instream_, FILE *outstream_, FILE *errstream_)
     m_gdb_stderr (new stderr_file (errstream)),
     m_gdb_stdlog (m_gdb_stderr)
 {
-  buffer_init (&line_buffer);
-
   unbuffer_stream (instream_);
 
   if (ui_list == NULL)
@@ -343,8 +341,6 @@ ui::~ui ()
   delete m_gdb_stdin;
   delete m_gdb_stdout;
   delete m_gdb_stderr;
-
-  buffer_free (&line_buffer);
 }
 
 /* Open file named NAME for read/write, making sure not to make it the
@@ -452,11 +448,11 @@ read_command_file (FILE *stream)
 
   while (ui->instream != NULL && !feof (ui->instream))
     {
-      const char *command;
-
       /* Get a command-line.  This calls the readline package.  */
-      command = command_line_input (NULL, NULL);
-      if (command == NULL)
+      std::string command_buffer;
+      const char *command
+	= command_line_input (command_buffer, nullptr, nullptr);
+      if (command == nullptr)
 	break;
       command_handler (command);
     }
@@ -1333,23 +1329,23 @@ gdb_safe_append_history (void)
     }
 }
 
-/* Read one line from the command input stream `instream' into a local
-   static buffer.  The buffer is made bigger as necessary.  Returns
-   the address of the start of the line.
+/* Read one line from the command input stream `instream'.
+
+   CMD_LINE_BUFFER is a buffer that the function may use to store the result, if
+   it needs to be dynamically-allocated.  Otherwise, it is unused.string
 
-   NULL is returned for end of file.
+   Return nullptr for end of file.
 
    This routine either uses fancy command line editing or simple input
    as the user has requested.  */
 
 const char *
-command_line_input (const char *prompt_arg, const char *annotation_suffix)
+command_line_input (std::string &cmd_line_buffer, const char *prompt_arg,
+		    const char *annotation_suffix)
 {
-  static struct buffer cmd_line_buffer;
-  static int cmd_line_buffer_initialized;
   struct ui *ui = current_ui;
   const char *prompt = prompt_arg;
-  char *cmd;
+  const char *cmd;
   int from_tty = ui->instream == ui->stdin_stream;
 
   /* The annotation suffix must be non-NULL.  */
@@ -1374,15 +1370,6 @@ command_line_input (const char *prompt_arg, const char *annotation_suffix)
       prompt = local_prompt;
     }
 
-  if (!cmd_line_buffer_initialized)
-    {
-      buffer_init (&cmd_line_buffer);
-      cmd_line_buffer_initialized = 1;
-    }
-
-  /* Starting a new command line.  */
-  cmd_line_buffer.used_size = 0;
-
 #ifdef SIGTSTP
   if (job_control)
     signal (SIGTSTP, handle_sigtstp);
@@ -1422,7 +1409,7 @@ command_line_input (const char *prompt_arg, const char *annotation_suffix)
 	  rl.reset (gdb_readline_no_editing (prompt));
 	}
 
-      cmd = handle_line_of_input (&cmd_line_buffer, rl.get (),
+      cmd = handle_line_of_input (cmd_line_buffer, rl.get (),
 				  0, annotation_suffix);
       if (cmd == (char *) EOF)
 	{
diff --git a/gdb/top.h b/gdb/top.h
index 29778f69262c..cbe50432c600 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -68,7 +68,7 @@ struct ui
 
   /* The UI's command line buffer.  This is to used to accumulate
      input until we have a whole command line.  */
-  struct buffer line_buffer;
+  std::string line_buffer;
 
   /* The callback used by the event loop whenever an event is detected
      on the UI's input file descriptor.  This function incrementally
@@ -289,9 +289,9 @@ extern void show_commands (const char *args, int from_tty);
 
 extern void set_verbose (const char *, int, struct cmd_list_element *);
 
-extern char *handle_line_of_input (struct buffer *cmd_line_buffer,
-				   const char *rl, int repeat,
-				   const char *annotation_suffix);
+extern const char *handle_line_of_input (std::string &cmd_line_buffer,
+					 const char *rl, int repeat,
+					 const char *annotation_suffix);
 
 /* Call at startup to see if the user has requested that gdb start up
    quietly.  */

base-commit: 38665d717a3e65c70e6432243d5eed9728a4888a
-- 
2.38.1

Re: [PATCH RESEND] gdb: remove static buffer in command_line_input

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> [I sent this earlier today, but I don't see it in the archives.
Simon> Resending it through a different computer / SMTP.]

I didn't see it either.

Simon> Fix that by removing the static buffer.

Wow, I've wanted to remove 'struct buffer' for a while now, this is a
big step toward that.

Simon> So, it started with modifying command_line_input as described above, all
Simon> the other changes derive directly from that.

It's hard to review / understand a change like this but it seems ok to
me, as far as I can tell.

Simon> One slightly shady thing is in handle_line_of_input, where we now pass a
Simon> pointer to an std::string's internal buffer to readline's history_value
Simon> function, which takes a `char *`.  I'm pretty sure that this function
Simon> does not modify the input string, because I was able to change it (with
Simon> enough massaging) to take a `const char *`.

I think sending a note to upstream readline would be good.

thanks,
Tom

Re: [PATCH RESEND] gdb: remove static buffer in command_line_input

[Simon Marchi]

On 12/15/22 16:44, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> [I sent this earlier today, but I don't see it in the archives.
> Simon> Resending it through a different computer / SMTP.]
> 
> I didn't see it either.
> 
> Simon> Fix that by removing the static buffer.
> 
> Wow, I've wanted to remove 'struct buffer' for a while now, this is a
> big step toward that.

Ah yeah, that's a nice side-effect indeed.

> 
> Simon> So, it started with modifying command_line_input as described above, all
> Simon> the other changes derive directly from that.
> 
> It's hard to review / understand a change like this but it seems ok to
> me, as far as I can tell.

I was eager to make more simplifications / C++ifications, but some of
them subtly broke things.  So I opted to keep things the same as much as
possible, really just change the type used to the buffer (use
std::string) and change where it was stored.

After spending a few hours debugging regressions, I am fairly confident
that I understand the code flow and that the change is correct.  I'll go
ahead and push it, to unblock Jan's patch.

> 
> Simon> One slightly shady thing is in handle_line_of_input, where we now pass a
> Simon> pointer to an std::string's internal buffer to readline's history_value
> Simon> function, which takes a `char *`.  I'm pretty sure that this function
> Simon> does not modify the input string, because I was able to change it (with
> Simon> enough massaging) to take a `const char *`.
> 
> I think sending a note to upstream readline would be good.

I forgot to mention, but I did send this message:

  https://lists.gnu.org/archive/html/bug-readline/2022-12/msg00002.html

Simon

Re: [PATCH RESEND] gdb: remove static buffer in command_line_input

[Pedro Alves]

On 2022-12-15 7:06 p.m., Simon Marchi via Gdb-patches wrote:
>    else
>      {
>        /* Copy whole line including terminating null, and we're
>  	 done.  */
> -      buffer_grow (cmd_line_buffer, rl, len + 1);
> -      cmd = cmd_line_buffer->buffer;
> +      cmd_line_buffer.append (rl, len + 1);
> +      return true;

std::string is guaranteed to be null terminated, but, it can also hold
\0 characters in the middle of the string.  Isn't that

  cmd_line_buffer.append (rl, len + 1);

ending up with an embedded \0 at the end of the string, with cmd_line_buffer.size() accounting for it?



>      }

Re: [PATCH RESEND] gdb: remove static buffer in command_line_input

[Simon Marchi]

On 12/16/22 08:21, Pedro Alves wrote:
> On 2022-12-15 7:06 p.m., Simon Marchi via Gdb-patches wrote:
>>    else
>>      {
>>        /* Copy whole line including terminating null, and we're
>>  	 done.  */
>> -      buffer_grow (cmd_line_buffer, rl, len + 1);
>> -      cmd = cmd_line_buffer->buffer;
>> +      cmd_line_buffer.append (rl, len + 1);
>> +      return true;
> 
> std::string is guaranteed to be null terminated, but, it can also hold
> \0 characters in the middle of the string.  Isn't that
> 
>   cmd_line_buffer.append (rl, len + 1);
> 
> ending up with an embedded \0 at the end of the string, with cmd_line_buffer.size() accounting for it?

Argh, yes!  I rewrote this patch from scratch multiple times until I got
something satisfactory, and I know I changed that in a previous
iteration, because I remember having that thought.

Here's a patch to fix it:


From 8757d2ec9edf2ae0b77039ca0ecc9ca32fd87256 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Fri, 16 Dec 2022 08:34:56 -0500
Subject: [PATCH] gdb: don't copy final null character in
 command_line_append_input_line

The modified std::string::append call currently copies the null
character from the source string, leaving the std::string with a null
character in its content, part of its size, in addition to the null
character managed by the std::string itself  We don't want to copy that
null character, so remove that "+ 1".

Change-Id: I412fe9bd6b08e7ce7604ef5d8038b62f1a784c9b
---
 gdb/event-top.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index fa86a89e4a19..fa9e1f3f6a73 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -640,7 +640,7 @@ command_line_append_input_line (std::string &cmd_line_buffer, const char *rl)
     {
       /* Copy whole line including terminating null, and we're
 	 done.  */
-      cmd_line_buffer.append (rl, len + 1);
+      cmd_line_buffer.append (rl, len);
       return true;
     }
 }

base-commit: e60a615dde5d6674a6488b74afe807a775551407
-- 
2.38.1

Re: [PATCH RESEND] gdb: remove static buffer in command_line_input

[Pedro Alves]

On 2022-12-16 1:38 p.m., Simon Marchi wrote:
> 
> 
> On 12/16/22 08:21, Pedro Alves wrote:
>> On 2022-12-15 7:06 p.m., Simon Marchi via Gdb-patches wrote:
>>>    else
>>>      {
>>>        /* Copy whole line including terminating null, and we're
>>>  	 done.  */
>>> -      buffer_grow (cmd_line_buffer, rl, len + 1);
>>> -      cmd = cmd_line_buffer->buffer;
>>> +      cmd_line_buffer.append (rl, len + 1);
>>> +      return true;
>>
>> std::string is guaranteed to be null terminated, but, it can also hold
>> \0 characters in the middle of the string.  Isn't that
>>
>>   cmd_line_buffer.append (rl, len + 1);
>>
>> ending up with an embedded \0 at the end of the string, with cmd_line_buffer.size() accounting for it?
> 
> Argh, yes!  I rewrote this patch from scratch multiple times until I got
> something satisfactory, and I know I changed that in a previous
> iteration, because I remember having that thought.
> 
> Here's a patch to fix it:
> 
> 
> From 8757d2ec9edf2ae0b77039ca0ecc9ca32fd87256 Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Fri, 16 Dec 2022 08:34:56 -0500
> Subject: [PATCH] gdb: don't copy final null character in
>  command_line_append_input_line
> 
> The modified std::string::append call currently copies the null
> character from the source string, leaving the std::string with a null
> character in its content, part of its size, in addition to the null
> character managed by the std::string itself  We don't want to copy that

Missing period after "itself".

> null character, so remove that "+ 1".
> 
> Change-Id: I412fe9bd6b08e7ce7604ef5d8038b62f1a784c9b
> ---
>  gdb/event-top.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/event-top.c b/gdb/event-top.c
> index fa86a89e4a19..fa9e1f3f6a73 100644
> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -640,7 +640,7 @@ command_line_append_input_line (std::string &cmd_line_buffer, const char *rl)
>      {
>        /* Copy whole line including terminating null, and we're
>  	 done.  */
> -      cmd_line_buffer.append (rl, len + 1);
> +      cmd_line_buffer.append (rl, len);

I'd update the comment -- the "including terminating null" part seems
misleading now, just drop that part?

Otherwise LTGM.

>        return true;
>      }
>  }
> 
> base-commit: e60a615dde5d6674a6488b74afe807a775551407
>