[lldb] Don't crash when attempting to parse breakpoint id N. as N.*

[rupprecht]

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.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jordan Rupprecht (rupprecht)

Changes

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/87263.diff

2 Files Affected:

• (modified) lldb/source/Breakpoint/BreakpointIDList.cpp (+22-26)
• (modified) lldb/test/API/functionalities/breakpoint/breakpoint_locations/TestBreakpointLocations.py (+6)

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",

[jimingham]

LGMT

[bulbazord]

Rewriting this has been on my backlog for a few months now. This looks better than what I had in mind. Thank you! 😄