[lldb] Handle malformed qfThreadInfo reply

[ted]

Summary

If the remote gdbserver's qfThreadInfo reply has a trailing comma,
GDBRemoteCommunicationClient::GetCurrentProcessAndThreadIDs will return
an empty vector of thread ids. This will cause lldb to recurse through
three functions trying to get the list of threads, until it blows its
stack and crashes.

A trailing comma is a malformed response, but it shouldn't cause lldb to
crash. This patch will return the tids received before the malformed
response.

[clayborg]

We should make a test for this.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

2914–2916

Might be nice to make sure we got LLDB_INVALID_PROCESS_ID back, as it might not always be zero?

[labath]

+1 for the test. It should be fairly easy to rig up a "gdb-remote client" test that sends such a packet.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

2914–2916

pid == LLDB_INVALID_PROCESS_ID is a valid response if the stub does not support the multiprocess syntax. pid == GetPID() is a valid response if it supports it. IIRC, further validation/interpretation of these values is done in the caller.

2916–2917

Isn't it still possible to get the infinite recursion you mentioned, if the response consists only of a single comma ?

[ted]

I agree about the test. I'll work on one.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

2916–2917

Yes. I was wondering if the correct response to this case is to drop down and go through the "pid=tid=1" case from line 2931. What do you think?

[ted]

I've got a test written. It doesn't crash like the debugger in the wild does, but it does give a tid of 0 for each thread I ask about. So I can assert if the threads don't have the correct tid.

With the patch, the test passes (gets the tids correctly)
Without the patch and asserts enabled, the test causes an assert in llvm::Optional
WIthout the patch and asserts disabled, the test fails when it checks the tid.

Which leads me to 1 more question - what should I do when we have a malformed response, and there are no threads listed? I'm leaning towards just falling through to the if at line 2931, and returning a pid and tid of 1. That's what we do if there's and empty response.

[labath]

In D109937#3007071, @ted wrote:

I've got a test written. It doesn't crash like the debugger in the wild does, but it does give a tid of 0 for each thread I ask about. So I can assert if the threads don't have the correct tid.

I guess you forgot to upload it (?)

Which leads me to 1 more question - what should I do when we have a malformed response, and there are no threads listed? I'm leaning towards just falling through to the if at line 2931, and returning a pid and tid of 1. That's what we do if there's and empty response.

Sounds reasonable to me. We'd be basically treating it as if the stub did not support the packet at all.

[ted]

[lldb] Handle malformed qfThreadInfo reply

Add requested test.
Refine handling of malformed reply with no valid threads.

[labath]

lldb/test/API/functionalities/gdb_remote_client/TestThreadInfoTrailingComma.py

24–25

s/print/self.trace/

[ted]

Remove unneeded prints from test

[ted]

Prints were there for my debugging. They don't serve a purpose, so I've removed them.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

 //===-- GDBRemoteCommunicationClient.cpp ----------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
for (packet_result =
          packet_result =
              SendPacketAndWaitForResponseNoLock("qsThreadInfo", response)) {
       char ch = response.GetChar();
       if (ch == 'l')
         break;
       if (ch == 'm') {
         do {
           auto pid_tid = response.GetPidTid(LLDB_INVALID_PROCESS_ID);
+          // If we get an invalid response, break out of the loop.
+          // If there are valid tids, they have been added to ids.
+          // If there are no valid tids, we'll fall through to the
+          // bare-iron target handling below.
           if (!pid_tid)
-            return {};
+            break;
 
           ids.push_back(pid_tid.getValue());
           ch = response.GetChar(); // Skip the command separator
         } while (ch == ',');       // Make sure we got a comma separator
       }
     }
 
     /*
      * Connected bare-iron target (like YAMON gdb-stub) may not have support for
      * qProcessInfo, qC and qfThreadInfo packets. The reply from '?' packet
      * could

lldb/test/API/functionalities/gdb_remote_client/TestThreadInfoTrailingComma.py

+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestThreadInfoTrailingComma(GDBRemoteTestBase):
+
+    def test(self):
+        class MyResponder(MockGDBServerResponder):
+            def haltReason(self):
+                return "T02thread:1"
+
+            def qfThreadInfo(self):
+                return "m1,2,3,4,"
+
+        self.server.responder = MyResponder()
+        target = self.dbg.CreateTarget('')
+        if self.TraceOn():
+          self.runCmd("log enable gdb-remote packets")
+          self.addTearDownHook(
+                lambda: self.runCmd("log disable gdb-remote packets"))
+        process = self.connect(target)
+        print(process.GetThreadAtIndex(0).GetFrameAtIndex(0))
+        print(process.GetThreadAtIndex(0).GetFrameAtIndex(0).FindRegister("pc"))
+        self.assertEqual(process.GetThreadAtIndex(0).GetThreadID(), 1)
+        self.assertEqual(process.GetThreadAtIndex(1).GetThreadID(), 2)
+        self.assertEqual(process.GetThreadAtIndex(2).GetThreadID(), 3)
+        self.assertEqual(process.GetThreadAtIndex(3).GetThreadID(), 4)