[Lldb-commits] [lldb] [lldb] Don't crash when attempting to parse breakpoint id `N.` as `N.*` (PR #87263)

[Jordan Rupprecht via lldb-commits]

https://github.com/rupprecht created https://github.com/llvm/llvm-project/pull/87263

We check if the next character after `N.` is `*` before we check its length. Using `split` on the string is cleaner and less error prone than using indices with `find` and `substr`.

Note: this does not make `N.` mean anything, it just prevents assertion failures. `N.` is treated the same as an unrecognized breakpoint name:

```
(lldb) breakpoint enable 1
1 breakpoints enabled.
(lldb) breakpoint enable 1.*
1 breakpoints enabled.
(lldb) breakpoint enable 1.
0 breakpoints enabled.
(lldb) breakpoint enable xyz
0 breakpoints enabled.
```

Found via LLDB fuzzers.

>From dc0aac066ec882d564dfc09acfae4b3dca5c8091 Mon Sep 17 00:00:00 2001
From: Jordan Rupprecht <rupprecht at google.com>
Date: Mon, 1 Apr 2024 16:44:09 +0000
Subject: [PATCH] [lldb] Don't crash when attempting to parse breakpoint id
 `N.` as `N.*`

We check if the next character after `N.` is `*` before we check its length. Using `split` on the string is cleaner and less error prone than using indices with `find` and `substr`.

Note: this does not make `N.` mean anything, it just prevents assertion failures. `N.` is treated the same as an unrecognized breakpoint name:

```
(lldb) breakpoint enable 1
1 breakpoints enabled.
(lldb) breakpoint enable 1.*
1 breakpoints enabled.
(lldb) breakpoint enable 1.
0 breakpoints enabled.
(lldb) breakpoint enable xyz
0 breakpoints enabled.
```

Found via LLDB fuzzers.
---
 lldb/source/Breakpoint/BreakpointIDList.cpp   | 48 +++++++++----------
 .../TestBreakpointLocations.py                |  6 +++
 2 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/lldb/source/Breakpoint/BreakpointIDList.cpp b/lldb/source/Breakpoint/BreakpointIDList.cpp
index 851d074e753588..97af1d40eb7a58 100644
--- a/lldb/source/Breakpoint/BreakpointIDList.cpp
+++ b/lldb/source/Breakpoint/BreakpointIDList.cpp
@@ -16,6 +16,7 @@
 #include "lldb/Utility/StreamString.h"
 
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -111,32 +112,27 @@ llvm::Error BreakpointIDList::FindAndReplaceIDRanges(
     } else {
       // See if user has specified id.*
       llvm::StringRef tmp_str = old_args[i].ref();
-      size_t pos = tmp_str.find('.');
-      if (pos != llvm::StringRef::npos) {
-        llvm::StringRef bp_id_str = tmp_str.substr(0, pos);
-        if (BreakpointID::IsValidIDExpression(bp_id_str) &&
-            tmp_str[pos + 1] == '*' && tmp_str.size() == (pos + 2)) {
-
-          BreakpointSP breakpoint_sp;
-          auto bp_id = BreakpointID::ParseCanonicalReference(bp_id_str);
-          if (bp_id)
-            breakpoint_sp = target->GetBreakpointByID(bp_id->GetBreakpointID());
-          if (!breakpoint_sp) {
-            new_args.Clear();
-            return llvm::createStringError(
-                llvm::inconvertibleErrorCode(),
-                "'%d' is not a valid breakpoint ID.\n",
-                bp_id->GetBreakpointID());
-          }
-          const size_t num_locations = breakpoint_sp->GetNumLocations();
-          for (size_t j = 0; j < num_locations; ++j) {
-            BreakpointLocation *bp_loc =
-                breakpoint_sp->GetLocationAtIndex(j).get();
-            StreamString canonical_id_str;
-            BreakpointID::GetCanonicalReference(
-                &canonical_id_str, bp_id->GetBreakpointID(), bp_loc->GetID());
-            new_args.AppendArgument(canonical_id_str.GetString());
-          }
+      auto [prefix, suffix] = tmp_str.split('.');
+      if (suffix == "*" && BreakpointID::IsValidIDExpression(prefix)) {
+
+        BreakpointSP breakpoint_sp;
+        auto bp_id = BreakpointID::ParseCanonicalReference(prefix);
+        if (bp_id)
+          breakpoint_sp = target->GetBreakpointByID(bp_id->GetBreakpointID());
+        if (!breakpoint_sp) {
+          new_args.Clear();
+          return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                         "'%d' is not a valid breakpoint ID.\n",
+                                         bp_id->GetBreakpointID());
+        }
+        const size_t num_locations = breakpoint_sp->GetNumLocations();
+        for (size_t j = 0; j < num_locations; ++j) {
+          BreakpointLocation *bp_loc =
+              breakpoint_sp->GetLocationAtIndex(j).get();
+          StreamString canonical_id_str;
+          BreakpointID::GetCanonicalReference(
+              &canonical_id_str, bp_id->GetBreakpointID(), bp_loc->GetID());
+          new_args.AppendArgument(canonical_id_str.GetString());
         }
       }
     }
diff --git a/lldb/test/API/functionalities/breakpoint/breakpoint_locations/TestBreakpointLocations.py b/lldb/test/API/functionalities/breakpoint/breakpoint_locations/TestBreakpointLocations.py
index 8930bea619bb6e..d87e6275f7b51e 100644
--- a/lldb/test/API/functionalities/breakpoint/breakpoint_locations/TestBreakpointLocations.py
+++ b/lldb/test/API/functionalities/breakpoint/breakpoint_locations/TestBreakpointLocations.py
@@ -293,6 +293,12 @@ def breakpoint_locations_test(self):
             startstr="3 breakpoints enabled.",
         )
 
+        # The 'breakpoint enable 1.' command should not crash.
+        self.expect(
+            "breakpoint enable 1.",
+            startstr="0 breakpoints enabled.",
+        )
+
         # The 'breakpoint disable 1.1' command should disable 1 location.
         self.expect(
             "breakpoint disable 1.1",