[PATCH] gdb/cp-namespace.c: Fix assert failure caused by malformed user input

[PATCH] gdb/cp-namespace.c: Fix assert failure caused by malformed user input

[Aaron Merey]

When debugging C++ programs, it is possible to trigger a spurious assert
failure when attempting to set a breakpoint on a malformed symbol name.
Names of the form 'A>::B' trigger this assert failure in
cp_lookup_bare_symbol:

    $ gdb gdb
    [...]
    (gdb) br test>::assert
    Function "test>::assert" not defined.
    Make breakpoint pending on future shared library load? (y or [n]) y
    Breakpoint 1 (test>::assert) pending.
    (gdb) start
    [...]
    cp-namespace.c:181: internal-error: cp_lookup_bare_symbol: Assertion `strstr (name, "::") == NULL' failed.
    A problem internal to GDB has been detected,
    further debugging may prove unreliable.
    ----- Backtrace -----
    0x5217e2 gdb_internal_backtrace_1
            /home/amerey/binutils-gdb/gdb/bt-utils.c:122
    0x521885 _Z22gdb_internal_backtracev
            /home/amerey/binutils-gdb/gdb/bt-utils.c:168
    0xaf8303 internal_vproblem
            /home/amerey/binutils-gdb/gdb/utils.c:396
    0xaf86be _Z15internal_verrorPKciS0_P13__va_list_tag
            /home/amerey/binutils-gdb/gdb/utils.c:476
    0xccdb3f _Z18internal_error_locPKciS0_z
            /home/amerey/binutils-gdb/gdbsupport/errors.cc:58
    0x5dded9 cp_lookup_bare_symbol
            /home/amerey/binutils-gdb/gdb/cp-namespace.c:181
    0x5de39d cp_lookup_symbol_in_namespace
            /home/amerey/binutils-gdb/gdb/cp-namespace.c:328
    [...]

Currently this assert is skipped if the symbol name contains '<' or '('.
Fix this spurious failure by also skipping the assert when the symbol
name contains '>'.

Regression tested on F38 x86_64.
---
 gdb/cp-namespace.c                 | 5 +++--
 gdb/testsuite/gdb.cp/namespace.exp | 4 ++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
index 14d807694b7..f36bec5b1f8 100644
--- a/gdb/cp-namespace.c
+++ b/gdb/cp-namespace.c
@@ -177,8 +177,9 @@ cp_lookup_bare_symbol (const struct language_defn *langdef,
   /* Note: We can't do a simple assert for ':' not being in NAME because
      ':' may be in the args of a template spec.  This isn't intended to be
      a complete test, just cheap and documentary.  */
-  if (strchr (name, '<') == NULL && strchr (name, '(') == NULL)
-    gdb_assert (strstr (name, "::") == NULL);
+  if (strchr (name, '<') == nullptr && strchr (name, '>') == nullptr
+      && strchr (name, '(') == nullptr)
+    gdb_assert (strstr (name, "::") == nullptr);
 
   sym = lookup_symbol_in_static_block (name, block, domain);
   if (sym.symbol != NULL)
diff --git a/gdb/testsuite/gdb.cp/namespace.exp b/gdb/testsuite/gdb.cp/namespace.exp
index e364816fcb7..359b85ac393 100644
--- a/gdb/testsuite/gdb.cp/namespace.exp
+++ b/gdb/testsuite/gdb.cp/namespace.exp
@@ -250,3 +250,7 @@ gdb_test "print AAA::ALPHA" "\\$\[0-9\].* = AAA::ALPHA"
 # Regression tests for PR 9496.
 gdb_test "whatis ::C::CClass::NestedClass" "type = C::CClass::NestedClass"
 gdb_test "whatis ::C::CClass::NestedClass *" "type = C::CClass::NestedClass \\*"
+
+# Break on a function with a malformed name.
+gdb_test "break DNE>::DNE" "" "br malformed" \
+    "Make breakpoint pending on future shared library load?.*" "y"
-- 
2.41.0

Re: [PATCH] gdb/cp-namespace.c: Fix assert failure caused by malformed user input

[Tom Tromey]

>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> When debugging C++ programs, it is possible to trigger a spurious assert
Aaron> failure when attempting to set a breakpoint on a malformed symbol name.
Aaron> Names of the form 'A>::B' trigger this assert failure in
Aaron> cp_lookup_bare_symbol:

Thanks for the patch.

Aaron> -  if (strchr (name, '<') == NULL && strchr (name, '(') == NULL)
Aaron> -    gdb_assert (strstr (name, "::") == NULL);
Aaron> +  if (strchr (name, '<') == nullptr && strchr (name, '>') == nullptr
Aaron> +      && strchr (name, '(') == nullptr)
Aaron> +    gdb_assert (strstr (name, "::") == nullptr);

I think it would be better to use strpbrk here, like:

gdb_assert (strpbrk (name, "<(>") != nullptr || strstr (name, "::") == nullptr);

Writing it like this, though, I wonder if a stray ")" could also make it
to this assert, the way that ">" did.

Tom

[PATCH] gdb/cp-namespace.c: Fix assert failure caused by malformed user input

[Aaron Merey]

Hi Tom,

On Thu, Jul 6, 2023 at 11:40 AM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
> Aaron> -  if (strchr (name, '<') == NULL && strchr (name, '(') == NULL)
> Aaron> -    gdb_assert (strstr (name, "::") == NULL);
> Aaron> +  if (strchr (name, '<') == nullptr && strchr (name, '>') == nullptr
> Aaron> +      && strchr (name, '(') == nullptr)
> Aaron> +    gdb_assert (strstr (name, "::") == nullptr);
>
> I think it would be better to use strpbrk here, like:
>
> gdb_assert (strpbrk (name, "<(>") != nullptr || strstr (name, "::") == nullptr);
>
> Writing it like this, though, I wonder if a stray ")" could also make it
> to this assert, the way that ">" did.

The stray ')' does trigger the same assert.  Below I've tweaked the patch to
prevent the assert with stray ')'.

Aaron

Commit message:

When debugging C++ programs, it is possible to trigger a spurious assert
failure when attempting to set a breakpoint on a malformed symbol name.
Names of the form 'A>::B' and 'A)::B' trigger this assert failure in
cp_lookup_bare_symbol:

    $ gdb gdb
    [...]
    (gdb) br test>::assert
    Function "test>::assert" not defined.
    Make breakpoint pending on future shared library load? (y or [n]) y
    Breakpoint 1 (test>::assert) pending.
    (gdb) start
    [...]
    cp-namespace.c:181: internal-error: cp_lookup_bare_symbol: Assertion `strstr (name, "::") == NULL' failed.
    A problem internal to GDB has been detected,
    further debugging may prove unreliable.
    ----- Backtrace -----
    0x5217e2 gdb_internal_backtrace_1
            /home/amerey/binutils-gdb/gdb/bt-utils.c:122
    0x521885 _Z22gdb_internal_backtracev
            /home/amerey/binutils-gdb/gdb/bt-utils.c:168
    0xaf8303 internal_vproblem
            /home/amerey/binutils-gdb/gdb/utils.c:396
    0xaf86be _Z15internal_verrorPKciS0_P13__va_list_tag
            /home/amerey/binutils-gdb/gdb/utils.c:476
    0xccdb3f _Z18internal_error_locPKciS0_z
            /home/amerey/binutils-gdb/gdbsupport/errors.cc:58
    0x5dded9 cp_lookup_bare_symbol
            /home/amerey/binutils-gdb/gdb/cp-namespace.c:181
    0x5de39d cp_lookup_symbol_in_namespace
            /home/amerey/binutils-gdb/gdb/cp-namespace.c:328
    [...]

Currently this assert is skipped if the symbol name contains '<' or '('.
Fix this spurious failure by also skipping the assert when the symbol
name contains '>' or ')'.

Regression tested on F38 x86_64.
---
 gdb/cp-namespace.c                 | 5 +++--
 gdb/testsuite/gdb.cp/namespace.exp | 6 ++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
index 14d807694b7..d4a47eaac1f 100644
--- a/gdb/cp-namespace.c
+++ b/gdb/cp-namespace.c
@@ -34,6 +34,7 @@
 #include "namespace.h"
 #include <map>
 #include <string>
+#include <string.h>
 
 static struct block_symbol
   cp_lookup_nested_symbol_1 (struct type *container_type,
@@ -177,8 +178,8 @@ cp_lookup_bare_symbol (const struct language_defn *langdef,
   /* Note: We can't do a simple assert for ':' not being in NAME because
      ':' may be in the args of a template spec.  This isn't intended to be
      a complete test, just cheap and documentary.  */
-  if (strchr (name, '<') == NULL && strchr (name, '(') == NULL)
-    gdb_assert (strstr (name, "::") == NULL);
+  gdb_assert (strpbrk ("<>()", name) != nullptr
+	      || strstr (name, "::") == nullptr);
 
   sym = lookup_symbol_in_static_block (name, block, domain);
   if (sym.symbol != NULL)
diff --git a/gdb/testsuite/gdb.cp/namespace.exp b/gdb/testsuite/gdb.cp/namespace.exp
index e364816fcb7..a9de09f087d 100644
--- a/gdb/testsuite/gdb.cp/namespace.exp
+++ b/gdb/testsuite/gdb.cp/namespace.exp
@@ -250,3 +250,9 @@ gdb_test "print AAA::ALPHA" "\\$\[0-9\].* = AAA::ALPHA"
 # Regression tests for PR 9496.
 gdb_test "whatis ::C::CClass::NestedClass" "type = C::CClass::NestedClass"
 gdb_test "whatis ::C::CClass::NestedClass *" "type = C::CClass::NestedClass \\*"
+
+# Break on functions with a malformed name.
+gdb_test "break DNE>::DNE" "" "br malformed \'>\'" \
+    "Make breakpoint pending on future shared library load?.*" "y"
+gdb_test "break DNE)::DNE" "" "br malformed \')\'" \
+    "Make breakpoint pending on future shared library load?.*" "y"
-- 
2.41.0

Re: [PATCH] gdb/cp-namespace.c: Fix assert failure caused by malformed user input

[Tom Tromey]

>>>>> "Aaron" == Aaron Merey <amerey@redhat.com> writes:

Aaron> The stray ')' does trigger the same assert.  Below I've tweaked the patch to
Aaron> prevent the assert with stray ')'.

Thank you.  This is ok.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH] gdb/cp-namespace.c: Fix assert failure caused by malformed user input

[Aaron Merey]

On Fri, Jul 7, 2023 at 5:27 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Aaron" == Aaron Merey <amerey@redhat.com> writes:
>
> Aaron> The stray ')' does trigger the same assert.  Below I've tweaked the patch to
> Aaron> prevent the assert with stray ')'.
>
> Thank you.  This is ok.
>
> Approved-By: Tom Tromey <tom@tromey.com>

Thanks, pushed as commit 4d792107c916.

Aaron