[binutils-gdb] gdb: fix crash during command completion

[binutils-gdb] gdb: fix crash during command completion

[Andrew Burgess]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=454f8b67a85130b6d6f6a7c987d868183b411369

commit 454f8b67a85130b6d6f6a7c987d868183b411369
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Fri Jan 6 15:50:26 2023 +0000

    gdb: fix crash during command completion
    
    In some cases GDB will fail when attempting to complete a command that
    involves a rust symbol, the failure can manifest as a crash.
    
    The problem is caused by the completion_match_for_lcd object being
    left containing invalid data during calls to cp_symbol_name_matches_1.
    
    The first question to address is why we are calling a C++ support
    function when handling a rust symbol.  That's due to GDB's auto
    language detection for msymbols, in some cases GDB can't tell if a
    symbol is a rust symbol, or a C++ symbol.
    
    The test application contains symbols for functions which are
    statically linked in from various rust support libraries.  There's no
    DWARF for these symbols, so all GDB has is the msymbols built from the
    ELF symbol table.
    
    Here's the problematic symbol that leads to our crash:
    
        mangled: _ZN4core3str21_$LT$impl$u20$str$GT$5parse17h5111d2d6a50d22bdE
      demangled: core::str::<impl str>::parse
    
    As an msymbol this is initially created with language auto, then GDB
    eventually calls symbol_find_demangled_name, which loops over all
    languages calling language_defn::sniff_from_mangled_name, the first
    language that can demangle the symbol gets assigned as the language
    for that symbol.
    
    Unfortunately, there's overlap in the mangled symbol names,
    some (legacy) rust symbols can be demangled as both rust and C++, see
    cplus_demangle in libiberty/cplus-dem.c where this is mentioned.
    
    And so, because we check the C++ language before we check for rust,
    then the msymbol is (incorrectly) given the C++ language.
    
    Now it's true that is some cases we might be able to figure out that a
    demangled symbol is not actually a valid C++ symbol, for example, in
    our case, the construct '::<impl str>::' is not, I believe, valid in a
    C++ symbol, we could look for ':<' and '>:' and refuse to accept this
    as a C++ symbol.
    
    However, I'm not sure it is always possible to tell that a demangled
    symbol is rust or C++, so, I think, we have to accept that some times
    we will get this language detection wrong.
    
    If we accept that we can't fix the symbol language detection 100% of
    the time, then we should make sure that GDB doesn't crash when it gets
    the language wrong, that is what this commit addresses.
    
    In our test case the user tries to complete a symbol name like this:
    
      (gdb) complete break pars
    
    This results in GDB trying to find all symbols that match 'pars',
    eventually we consider our problematic symbol, and we end up with a
    call stack that looks like this:
    
      #0  0x0000000000f3c6bd in strncmp_iw_with_mode
      #1  0x0000000000706d8d in cp_symbol_name_matches_1
      #2  0x0000000000706fa4 in cp_symbol_name_matches
      #3  0x0000000000df3c45 in compare_symbol_name
      #4  0x0000000000df3c91 in completion_list_add_name
      #5  0x0000000000df3f1d in completion_list_add_msymbol
      #6  0x0000000000df4c94 in default_collect_symbol_completion_matches_break_on
      #7  0x0000000000658c08 in language_defn::collect_symbol_completion_matches
      #8  0x0000000000df54c9 in collect_symbol_completion_matches
      #9  0x00000000009d98fb in linespec_complete_function
      #10 0x00000000009d99f0 in complete_linespec_component
      #11 0x00000000009da200 in linespec_complete
      #12 0x00000000006e4132 in complete_address_and_linespec_locations
      #13 0x00000000006e4ac3 in location_completer
    
    In cp_symbol_name_matches_1 we enter a loop, this loop repeatedly
    tries to match the demangled problematic symbol name against the user
    supplied text ('pars').  Each time around the loop another component
    of the symbol name is stripped off, thus, we check 'pars' against
    these options:
    
      core::str::<impl str>::parse
      str::<impl str>::parse
      <impl str>::parse
      parse
    
    As soon as we get a match the cp_symbol_name_matches_1 exits its loop
    and returns.  In our case, when we're looking for 'pars', the match
    occurs on the last iteration of the loop, when we are comparing to
    'parse'.
    
    Now the problem here is that cp_symbol_name_matches_1 uses the
    strncmp_iw_with_mode, and inside strncmp_iw_with_mode we allow for
    skipping over template parameters.  This allows GDB to match the
    symbol name 'foo<int>(int,int)' if the user supplies 'foo(int,'.
    Inside strncmp_iw_with_mode GDB will record any template arguments
    that it has skipped over inside the completion_match_for_lcd object
    that is passed in as an argument.
    
    And so, when GDB tries to match against '<impl str>::parse', the first
    thing it sees is '<impl str>', GDB assumes this is a template argument
    and records this as a skipped region within the
    completion_match_for_lcd object.  After '<impl str>' GDB sees a ':'
    character, which doesn't match with the 'pars' the user supplied, so
    strncmp_iw_with_mode returns a value indicating a non-match.  GDB then
    removes the '<impl str>' component from the symbol name and tries
    again, this time comparing to 'parse', which does match.
    
    Having found a match, then in cp_symbol_name_matches_1 we record the
    match string, and the full symbol name within the
    completion_match_result object, and return.
    
    The problem here is that the skipped region, the '<impl str>' that we
    recorded in the penultimate loop iteration was never discarded, its
    still there in our returned result.
    
    If we look at what the pointers held in the completion_match_result
    that cp_symbol_name_matches_1 returns, this is what we see:
    
      core::str::<impl str>::parse
      |          \________/  |
      |               |      '--- completion match string
      |               '---skip range
      '--- full symbol name
    
    When GDB calls completion_match_for_lcd::finish, GDB tries to create a
    string using the completion match string (parse), but excluding the
    skip range, as the stored skip range is before the start of the
    completion match string, then GDB tries to do some weird string
    creation, which will cause GDB to crash.
    
    The reason we don't often see this problem in C++ is that for C++
    symbols there is always some non-template text before the template
    argument.  This non-template text means GDB is likely to either match
    the symbol, or reject the symbol without storing a skip range.
    
    However, notice, I did say, we don't often see this problem.  Once I
    understood the issue, I was able to reproduce the crash using a pure
    C++ example:
    
      template<typename S>
      struct foo
      {
        template<typename T>
        foo (int p1, T a)
        {
          s = 0;
        }
    
        S s;
      };
    
      int
      main ()
      {
        foo<int> obj (2.3, 0);
        return 0;
      }
    
    Then in GDB:
    
      (gdb) complete break foo(int
    
    The problem here is that the C++ symbol for the constructor looks like
    this:
    
      foo<int>::foo<double>(int, double)
    
    When GDB enters cp_symbol_name_matches_1 the symbols it examines are:
    
      foo<int>::foo<double>(int, double)
      foo<double>(int, double)
    
    The first iteration of the loop will match the 'foo', then add the
    '<int>' template argument will be added as a skip range.  When GDB
    find the ':' after the '<int>' the first iteration of the loop fails
    to match, GDB removes the 'foo<int>::' component, and starts the
    second iteration of the loop.
    
    Again, GDB matches the 'foo', and now adds '<double>' as a skip
    region.  After that the '(int' successfully matches, and so the second
    iteration of the loop succeeds, but, once again we left the '<int>' in
    place as a skip region, even though this occurs before the start of
    our match string, and this will cause GDB to crash.
    
    This problem was reported to the mailing list, and a solution
    discussed in this thread:
    
      https://sourceware.org/pipermail/gdb-patches/2023-January/195166.html
    
    The solution proposed here is similar to one proposed by the original
    bug reported, but implemented in a different location within GDB.
    Instead of placing the fix in strncmp_iw_with_mode, I place the fix in
    cp_symbol_name_matches_1.  I believe this is a better location as it
    is this function that implements the loop, and it is this loop, which
    repeatedly calls strncmp_iw_with_mode, that should be resetting the
    result object state (I believe).
    
    What I have done is add an assert to strncmp_iw_with_mode that the
    incoming result object is empty.
    
    I've also added some other asserts in related code, in
    completion_match_for_lcd::mark_ignored_range, I make some basic
    assertions about the incoming range pointers, and in
    completion_match_for_lcd::finish I also make some assertions about how
    the skip ranges relate to the match pointer.
    
    There's two new tests.  The original rust example that was used in the
    initial bug report, and a C++ test.  The rust example depends on which
    symbols are pulled in from the rust libraries, so it is possible that,
    at some future date, the problematic symbol will disappear from this
    test program.  The C++ test should be more reliable, as this only
    depends on symbols from within the C++ source code.
    
    Since I originally posted this patch to the mailing list, the
    following patch has been merged:
    
      commit 6e7eef72164c00d6a5a7b0bce9fa01f5481f33cb
      Date:   Sun Mar 19 09:13:10 2023 -0600
    
          Use rust_demangle to fix a crash
    
    This solves the problem of a rust symbol ending up in the C++ specific
    code by changing the order languages are sorted.  However, this new
    commit doesn't address the issue in the C++ code which was fixed with
    this commit.
    
    Given that the C++ issue is real, and has a reproducer, I'm still
    going to merge this fix.  I've left the discussion of rust in this
    commit message as I originally wrote it, but it should be read within
    the context of GDB prior to commit 6e7eef72164c00d6a5a7.
    
    Co-Authored-By:  Zheng Zhan <zzlossdev@163.com>

Diff:
---
 gdb/completer.h                       | 19 ++++++++++++++++++-
 gdb/cp-support.c                      |  7 +++++++
 gdb/testsuite/gdb.cp/cpcompletion.cc  | 23 +++++++++++++++++++++++
 gdb/testsuite/gdb.cp/cpcompletion.exp |  2 ++
 gdb/testsuite/gdb.rust/completion.exp | 33 +++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.rust/completion.rs  | 19 +++++++++++++++++++
 gdb/utils.c                           |  2 ++
 7 files changed, 104 insertions(+), 1 deletion(-)

diff --git a/gdb/completer.h b/gdb/completer.h
index 8b4ad8ec4d4..67d2fbf9d38 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -145,7 +145,12 @@ public:
 
   /* Mark the range between [BEGIN, END) as ignored.  */
   void mark_ignored_range (const char *begin, const char *end)
-  { m_ignored_ranges.emplace_back (begin, end); }
+  {
+    gdb_assert (begin < end);
+    gdb_assert (m_ignored_ranges.empty ()
+		|| m_ignored_ranges.back ().second < begin);
+    m_ignored_ranges.emplace_back (begin, end);
+  }
 
   /* Get the resulting LCD, after a successful match.  If there are
      ignored ranges, then this builds a new string with the ignored
@@ -160,9 +165,14 @@ public:
       {
 	m_finished_storage.clear ();
 
+	gdb_assert (m_ignored_ranges.back ().second
+		    <= (m_match + strlen (m_match)));
+
 	const char *prev = m_match;
 	for (const auto &range : m_ignored_ranges)
 	  {
+	    gdb_assert (prev < range.first);
+	    gdb_assert (range.second > range.first);
 	    m_finished_storage.append (prev, range.first);
 	    prev = range.second;
 	  }
@@ -179,6 +189,13 @@ public:
     m_ignored_ranges.clear ();
   }
 
+  /* Return true if this object has had no match data set since its
+     creation, or the last call to clear.  */
+  bool empty () const
+  {
+    return m_match == nullptr && m_ignored_ranges.empty ();
+  }
+
 private:
   /* The completion match result for LCD.  This is usually either a
      pointer into to a substring within a symbol's name, or to the
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 78eacd05e97..f39c5d051dd 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -1776,6 +1776,8 @@ cp_symbol_name_matches_1 (const char *symbol_search_name,
   completion_match_for_lcd *match_for_lcd
     = (comp_match_res != NULL ? &comp_match_res->match_for_lcd : NULL);
 
+  gdb_assert (match_for_lcd == nullptr || match_for_lcd->empty ());
+
   while (true)
     {
       if (strncmp_iw_with_mode (sname, lookup_name, lookup_name_len,
@@ -1809,6 +1811,11 @@ cp_symbol_name_matches_1 (const char *symbol_search_name,
 	  return true;
 	}
 
+      /* Clear match_for_lcd so the next strncmp_iw_with_mode call starts
+	 from scratch.  */
+      if (match_for_lcd != nullptr)
+	match_for_lcd->clear ();
+
       unsigned int len = cp_find_first_component (sname);
 
       if (sname[len] == '\0')
diff --git a/gdb/testsuite/gdb.cp/cpcompletion.cc b/gdb/testsuite/gdb.cp/cpcompletion.cc
index 54ddaafc0ca..b854d810b11 100644
--- a/gdb/testsuite/gdb.cp/cpcompletion.cc
+++ b/gdb/testsuite/gdb.cp/cpcompletion.cc
@@ -52,8 +52,31 @@ int qux;
 
 } /* namespace Test_NS */
 
+/* The important thing with class baz is that both the class and the
+   constructor must have a template argument, we need the symbol to look
+   like:
+
+   baz<TYPE_1>::baz<TYPE_2>(int,....whatever...)
+
+   It doesn't really matter if TYPE_1 and TYPE_2 are the same or different,
+   but we create them differently in this test as it makes debugging GDB
+   slightly easier.  */
+
+template<typename S>
+struct baz
+{
+  template<typename T>
+  baz (int p1, T a)
+  {
+    s = 0;
+  }
+
+  S s;
+};
+
 int main ()
 {
+  baz<int> obj (2.3, 0.1);
   // Anonymous struct with method.
   struct {
     int get() { return 5; }
diff --git a/gdb/testsuite/gdb.cp/cpcompletion.exp b/gdb/testsuite/gdb.cp/cpcompletion.exp
index 8c5c90b9357..5838a54976f 100644
--- a/gdb/testsuite/gdb.cp/cpcompletion.exp
+++ b/gdb/testsuite/gdb.cp/cpcompletion.exp
@@ -135,3 +135,5 @@ with_test_prefix "expression with namespace" {
     # Add a disambiguating character and we get a unique completion.
     test_gdb_complete_unique "p Test_NS::f" "p Test_NS::foo"
 }
+
+test_gdb_complete_unique "break baz(int" "break baz(int, double)"
diff --git a/gdb/testsuite/gdb.rust/completion.exp b/gdb/testsuite/gdb.rust/completion.exp
new file mode 100644
index 00000000000..88763952dfa
--- /dev/null
+++ b/gdb/testsuite/gdb.rust/completion.exp
@@ -0,0 +1,33 @@
+# Copyright (C) 2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test a completion case that was causing GDB to crash.
+
+load_lib rust-support.exp
+
+require allow_rust_tests
+
+standard_testfile .rs
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug rust}]} {
+    return -1
+}
+
+set line [gdb_get_line_number "set breakpoint here"]
+if {![runto ${srcfile}:$line]} {
+    untested "could not run to breakpoint"
+    return -1
+}
+
+gdb_test "complete break pars" ".*"
diff --git a/gdb/testsuite/gdb.rust/completion.rs b/gdb/testsuite/gdb.rust/completion.rs
new file mode 100644
index 00000000000..8396e3f4086
--- /dev/null
+++ b/gdb/testsuite/gdb.rust/completion.rs
@@ -0,0 +1,19 @@
+// Copyright (C) 2023 Free Software Foundation, Inc.
+
+// This program is free software; you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation; either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+fn main() {
+    let four: u8 = "4".parse().unwrap();
+    println!("{}", four);	// set breakpoint here
+}
diff --git a/gdb/utils.c b/gdb/utils.c
index 0138c8e9fb6..8928295efdd 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -2108,6 +2108,8 @@ strncmp_iw_with_mode (const char *string1, const char *string2,
 			|| language == language_rust
 			|| language == language_fortran);
 
+  gdb_assert (match_for_lcd == nullptr || match_for_lcd->empty ());
+
   while (1)
     {
       if (skip_spaces