Page MenuHomePhabricator

[lldb] Handle malformed qfThreadInfo reply
ClosedPublic

Authored by ted on Sep 16 2021, 4:41 PM.

Details

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.

Diff Detail

Event Timeline

ted requested review of this revision.Sep 16 2021, 4:41 PM
ted created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2021, 4:41 PM

We should make a test for this.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
2916–2918

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

+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
2916–2918

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.

2918–2919

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

ted added a comment.Sep 17 2021, 8:01 AM

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

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
2918–2919

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 added a comment.Sep 17 2021, 12:35 PM

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.

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 updated this revision to Diff 374049.Sep 21 2021, 3:20 PM

[lldb] Handle malformed qfThreadInfo reply

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

labath accepted this revision.Sep 22 2021, 1:37 AM
labath added inline comments.
lldb/test/API/functionalities/gdb_remote_client/TestThreadInfoTrailingComma.py
25–26

s/print/self.trace/

This revision is now accepted and ready to land.Sep 22 2021, 1:37 AM
ted updated this revision to Diff 374233.Sep 22 2021, 7:43 AM

Remove unneeded prints from test

ted marked an inline comment as done.Sep 22 2021, 7:45 AM

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

clayborg accepted this revision.Sep 22 2021, 8:32 AM
This revision was automatically updated to reflect the committed changes.