Skip to content
Snippets Groups Projects
  • Simon Marchi's avatar
    f8631e5e
    gdb: remove static buffer in command_line_input · f8631e5e
    Simon Marchi authored
    [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
    Reviewed-By: default avatarTom Tromey <tom@tromey.com>
    f8631e5e
    History
    gdb: remove static buffer in command_line_input
    Simon Marchi authored
    [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
    Reviewed-By: default avatarTom Tromey <tom@tromey.com>