[Lldb-commits] [lldb] f9d8090 - Improve error handling for invalid breakpoint `-t` and `-x` options.

[Jordan Rupprecht via lldb-commits]

Author: Jordan Rupprecht
Date: 2022-12-08T17:53:54-08:00
New Revision: f9d8090a90a1f1f9ddf6aebb82e7fc4c1dbcf030

URL: https://github.com/llvm/llvm-project/commit/f9d8090a90a1f1f9ddf6aebb82e7fc4c1dbcf030
DIFF: https://github.com/llvm/llvm-project/commit/f9d8090a90a1f1f9ddf6aebb82e7fc4c1dbcf030.diff

LOG: Improve error handling for invalid breakpoint `-t` and `-x` options.

Breakpoint option `-t` checks that `option_arg` is empty by checking `option_arg[0] == '\0'`. This is unnecessary: the next two checks for comparing against "current" and calling `getAsInteger` already gracefully handle an empty StringRef. If the `option_arg` string is empty, this crashes (or triggers an assertion failure with asserts enabled). Also, this sets the thread id to `LLDB_INVALID_THREAD_ID` if the thread id is invalid -- it should just not set the thread id.

Likewise of `-x` which checks `option_arg[0] == '\n'` unnecessarily.

I believe both of these bugs are unreachable via normal LLDB usage, and are only accessible via the fuzzer -- most likely some other CLI parsing is trimming whitespace and rejecting empty inputs. Still, it doesn't hurt to simplify this bit.

Added: 
    

Modified: 
    lldb/source/Commands/CommandObjectBreakpoint.cpp

Removed: 
    


################################################################################
diff  --git a/lldb/source/Commands/CommandObjectBreakpoint.cpp b/lldb/source/Commands/CommandObjectBreakpoint.cpp
index 5d2141d767cb..62e46c59e169 100644
--- a/lldb/source/Commands/CommandObjectBreakpoint.cpp
+++ b/lldb/source/Commands/CommandObjectBreakpoint.cpp
@@ -110,24 +110,24 @@ class lldb_private::BreakpointOptionGroup : public OptionGroup {
     } break;
     case 't': {
       lldb::tid_t thread_id = LLDB_INVALID_THREAD_ID;
-      if (option_arg[0] != '\0') {
-        if (option_arg == "current") {
-          if (!execution_context) {
-            error.SetErrorStringWithFormat("No context to determine current "
-                                           "thread");
+      if (option_arg == "current") {
+        if (!execution_context) {
+          error.SetErrorStringWithFormat("No context to determine current "
+                                         "thread");
+        } else {
+          ThreadSP ctx_thread_sp = execution_context->GetThreadSP();
+          if (!ctx_thread_sp || !ctx_thread_sp->IsValid()) {
+            error.SetErrorStringWithFormat("No currently selected thread");
           } else {
-            ThreadSP ctx_thread_sp = execution_context->GetThreadSP();
-            if (!ctx_thread_sp || !ctx_thread_sp->IsValid()) {
-              error.SetErrorStringWithFormat("No currently selected thread");
-            } else {
-              thread_id = ctx_thread_sp->GetID();
-            }
+            thread_id = ctx_thread_sp->GetID();
           }
-        } else if (option_arg.getAsInteger(0, thread_id))
-          error.SetErrorStringWithFormat("invalid thread id string '%s'",
-                                         option_arg.str().c_str());
+        }
+      } else if (option_arg.getAsInteger(0, thread_id)) {
+        error.SetErrorStringWithFormat("invalid thread id string '%s'",
+                                       option_arg.str().c_str());
       }
-      m_bp_opts.SetThreadID(thread_id);
+      if (thread_id != LLDB_INVALID_THREAD_ID)
+        m_bp_opts.SetThreadID(thread_id);
     } break;
     case 'T':
       m_bp_opts.GetThreadSpec()->SetName(option_arg.str().c_str());
@@ -137,12 +137,12 @@ class lldb_private::BreakpointOptionGroup : public OptionGroup {
       break;
     case 'x': {
       uint32_t thread_index = UINT32_MAX;
-      if (option_arg[0] != '\n') {
-        if (option_arg.getAsInteger(0, thread_index))
-          error.SetErrorStringWithFormat("invalid thread index string '%s'",
-                                         option_arg.str().c_str());
+      if (option_arg.getAsInteger(0, thread_index)) {
+        error.SetErrorStringWithFormat("invalid thread index string '%s'",
+                                       option_arg.str().c_str());
+      } else {
+        m_bp_opts.GetThreadSpec()->SetIndex(thread_index);
       }
-      m_bp_opts.GetThreadSpec()->SetIndex(thread_index);
     } break;
     default:
       llvm_unreachable("Unimplemented option");