From 454f8b67a85130b6d6f6a7c987d868183b411369 Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Fri, 6 Jan 2023 15:50:26 +0000 Subject: [PATCH] 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::::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 '::::' 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::::parse str::::parse ::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)' 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 '::parse', the first thing it sees is '', GDB assumes this is a template argument and records this as a skipped region within the completion_match_for_lcd object. After '' 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 '' 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 '' 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::::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 struct foo { template foo (int p1, T a) { s = 0; } S s; }; int main () { foo 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::foo(int, double) When GDB enters cp_symbol_name_matches_1 the symbols it examines are: foo::foo(int, double) foo(int, double) The first iteration of the loop will match the 'foo', then add the '' template argument will be added as a skip range. When GDB find the ':' after the '' the first iteration of the loop fails to match, GDB removes the 'foo::' component, and starts the second iteration of the loop. Again, GDB matches the 'foo', and now adds '' 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 '' 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 --- 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(-) create mode 100644 gdb/testsuite/gdb.rust/completion.exp create mode 100644 gdb/testsuite/gdb.rust/completion.rs 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::baz(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 +struct baz +{ + template + baz (int p1, T a) + { + s = 0; + } + + S s; +}; + int main () { + baz 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 . + +# 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 . + +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 -- 2.30.2