Fix error reporting for qLaunchSuccess, check for fix/enable it via qSupported

[jasonmolenda]

Summary

I've been experimenting with returning richer error messages in debugserver recently, and I found a bug the other day where qLaunchSuccess returns its error code with an "E" followed by the text of the error message. If the error message contains one of the magic gdb RSP characters (#, $, *, }), lldb will often crash. We've seen packets like vAttach adopt a new method of returning an error message as Exx;HEX-ENCODED instead of the usual Exx.

We've had instances where packets need to be fixed in the past, e.g. Spencer Michaels reports a bug in the A packet here https://bugs.llvm.org/show_bug.cgi?id=42471 and I need to fix the constant values we use in the vFile packets. Spender and I were discussing a "qProtocolFixes" packet, but Fred suggested we could piggyback on qSupported. I liked that idea, so that's what I have implemented here.

lldb will send qLaunchSuccessASCIIHexErrorText in its qSupported request, and if this is a debugserver that has the fix, it will enable the corrected error reporting mode and include qLaunchSuccessASCIIHexErrorText in the qSupported response. I think this is a pretty clean way of fixing things like this, Greg/Pavel what do you think?

I also found a bug in debugserver's cstring_to_asciihex_string() function with high-bit set characters in the error string (e.g. a utf-8 character) with signed chars where we'd end up with a bunch of 0xff's in the ascii hex string.

I'm trying to make an API test for this but haven't found a good way to fake up enough of the lldb side to add a gdb_remote_client test yet.

rdar://problem/62873581

[clayborg]

Why not just fix qLaunchSuccess by passing the string through a "escape_string(...)" function? Any packet that can return a string with unknown content should be escaped correctly. Adding a new packet doesn't really fix the problem in older debugserver/lldb-server binaries anyway.

lldb/tools/debugserver/source/RNBRemote.cpp

1651

Why now just fix this line? Something like:

ret_str << "E" << escape_string(m_ctx.LaunchStatusAsString(status_str));

[jasonmolenda]

In D79614#2027861, @clayborg wrote:

Why not just fix qLaunchSuccess by passing the string through a "escape_string(...)" function? Any packet that can return a string with unknown content should be escaped correctly. Adding a new packet doesn't really fix the problem in older debugserver/lldb-server binaries anyway.

lldb wouldn't know whether to decode the returned error string or not -- short of using a debugserver version #. We'll need to interoperate with debugservers that send the unescaped qLaunchSuccess error messages for years to come.

[jasonmolenda]

In D79614#2027861, @clayborg wrote:

Why not just fix qLaunchSuccess by passing the string through a "escape_string(...)" function? Any packet that can return a string with unknown content should be escaped correctly. Adding a new packet doesn't really fix the problem in older debugserver/lldb-server binaries anyway.

tbh I think the more interesting aspect of the patch - the part worth discussing - is using a keyword to qSupported to test for / enable fixes in debugserver/lldb-server implementation, so lldb can interoperate with both fixed & broken remote agents, until we can safely excise the support for the buggy behavior in the future.

[clayborg]

In D79614#2027869, @jasonmolenda wrote:

In D79614#2027861, @clayborg wrote:

Why not just fix qLaunchSuccess by passing the string through a "escape_string(...)" function? Any packet that can return a string with unknown content should be escaped correctly. Adding a new packet doesn't really fix the problem in older debugserver/lldb-server binaries anyway.

lldb wouldn't know whether to decode the returned error string or not -- short of using a debugserver version #. We'll need to interoperate with debugservers that send the unescaped qLaunchSuccess error messages for years to come.

It should always decode a raw string so I don't see what you mean here. Any packet that returns a normal string, not hex ascii encoded, must escape the string. The only bug here is that we are not escaping the string that debugserver sends back. No need for the hex ASCII stuff if we escape any special characters right?

[jasonmolenda]

In D79614#2028295, @clayborg wrote:

In D79614#2027869, @jasonmolenda wrote:

In D79614#2027861, @clayborg wrote:

Why not just fix qLaunchSuccess by passing the string through a "escape_string(...)" function? Any packet that can return a string with unknown content should be escaped correctly. Adding a new packet doesn't really fix the problem in older debugserver/lldb-server binaries anyway.

lldb wouldn't know whether to decode the returned error string or not -- short of using a debugserver version #. We'll need to interoperate with debugservers that send the unescaped qLaunchSuccess error messages for years to come.

It should always decode a raw string so I don't see what you mean here. Any packet that returns a normal string, not hex ascii encoded, must escape the string. The only bug here is that we are not escaping the string that debugserver sends back. No need for the hex ASCII stuff if we escape any special characters right?

Ah, I see what you're getting at --- yes I bet if we switch to using the binary escape protocol for the error message that would work just as well. It's a one-off way to report an error message which doesn't thrill me, but you're right that this may not be worth the addition to qSupported etc. I'll confirm that using the standard escaping is sufficient and decide whether to just do that, it's probably the right call. Thanks for the feedback.

[labath]

I think that "piggy-backing" on the qSupported packet for communicating protocol fixes is a good idea. However, I agree with Greg, that it does not seem like it's needed for this case. Fixing the problem purely on the debugserver side seems preferable, as it avoids adding compatibility code to lldb. It's true that it's a one-off error reporting mechanism, but it's already there, so it seems better to just support what's already implemented then try to support two mechanisms.

For a long-term solution, I am wondering whether we need qLaunchSuccess at all? It seems like the packet is completely redundant in a world where we can return textual error messages to _any_ packet. What was the reason for introducing it in the first place? Could we just switch to using error replies to the A packet for communicating the launch errors (with some transition plan for supporting qLaunchSuccess for a while)?

[jasonmolenda]

Thanks for the feedback.

In D79614#2029157, @labath wrote:

I think that "piggy-backing" on the qSupported packet for communicating protocol fixes is a good idea. However, I agree with Greg, that it does not seem like it's needed for this case. Fixing the problem purely on the debugserver side seems preferable, as it avoids adding compatibility code to lldb. It's true that it's a one-off error reporting mechanism, but it's already there, so it seems better to just support what's already implemented then try to support two mechanisms.

For a long-term solution, I am wondering whether we need qLaunchSuccess at all? It seems like the packet is completely redundant in a world where we can return textual error messages to _any_ packet. What was the reason for introducing it in the first place? Could we just switch to using error replies to the A packet for communicating the launch errors (with some transition plan for supporting qLaunchSuccess for a while)?

It's hard to tell what we were thinking (GREG :) back when we used the A packet to set the binary name / arguments /etc, and then the qLaunchSuccess packet to start the process. Reading over the current era gdb Remote Serial Protocol docs, I think the vRun packet does the combination of these two things in one packet, which makes sense.

I tested a patch using Greg's suggestion of using the binary escape protocol for the error string and that does work, as expected, I'll land the patch like that.

[clayborg]

In D79614#2030692, @jasonmolenda wrote:

Thanks for the feedback.

In D79614#2029157, @labath wrote:

I think that "piggy-backing" on the qSupported packet for communicating protocol fixes is a good idea. However, I agree with Greg, that it does not seem like it's needed for this case. Fixing the problem purely on the debugserver side seems preferable, as it avoids adding compatibility code to lldb. It's true that it's a one-off error reporting mechanism, but it's already there, so it seems better to just support what's already implemented then try to support two mechanisms.

For a long-term solution, I am wondering whether we need qLaunchSuccess at all? It seems like the packet is completely redundant in a world where we can return textual error messages to _any_ packet. What was the reason for introducing it in the first place? Could we just switch to using error replies to the A packet for communicating the launch errors (with some transition plan for supporting qLaunchSuccess for a while)?

It's hard to tell what we were thinking (GREG :) back when we used the A packet to set the binary name / arguments /etc, and then the qLaunchSuccess packet to start the process. Reading over the current era gdb Remote Serial Protocol docs, I think the vRun packet does the combination of these two things in one packet, which makes sense.

There was no "just return error strings" packet that I was aware of. The problem is identifying a correct error packet. Typically you can get some canned responses to packets:

• "" (empty) response means not supported
• "EXX" where XX is a hex code only
• "OK" if the packet was successful
• open ended output

How do error strings get returned when not using the EXX format? Are they safely prefixed with something? Do they just start with "E" and then a string? If so, how would you tell the difference between the response "Everyone has a string" and "Einvalid path error"? Or does this only apply to a packet that returns an error if some setting was enabled with a previous "Q" packet?

I tested a patch using Greg's suggestion of using the binary escape protocol for the error string and that does work, as expected, I'll land the patch like that.

Much easier, thanks!

lldb/tools/debugserver/source/RNBRemote.cpp

rnb_err_t RNBRemote::HandlePacket_H(const char *p) {
   return SendPacket("OK");
 }
 
 rnb_err_t RNBRemote::HandlePacket_qLaunchSuccess(const char *p) {
   if (m_ctx.HasValidProcessID() || m_ctx.LaunchStatus().Status() == 0)
     return SendPacket("OK");
   std::ostringstream ret_str;
   std::string status_str;
-  ret_str << "E" << m_ctx.LaunchStatusAsString(status_str);
+  std::string error_quoted = binary_encode_string
+               (m_ctx.LaunchStatusAsString(status_str));
+  ret_str << "E" << error_quoted;
 
   return SendPacket(ret_str.str());
 }
 
 rnb_err_t RNBRemote::HandlePacket_qShlibInfoAddr(const char *p) {
   if (m_ctx.HasValidProcessID()) {
     nub_addr_t shlib_info_addr =
         DNBProcessGetSharedLibraryInfoAddress(m_ctx.ProcessID());
     if (shlib_info_addr != INVALID_NUB_ADDRESS) {
       std::ostringstream ostrm;
       ostrm << RAW_HEXBASE << shlib_info_addr;
for (size_t i = 0; i < buf_size; i++)
       ostrm << RAWHEX8(p[i]);
   }
 }
 
 std::string cstring_to_asciihex_string(const char *str) {
   std::string hex_str;
   hex_str.reserve (strlen (str) * 2);
   while (str && *str) {
+    uint8_t c = *str++;
     char hexbuf[5];
-    snprintf (hexbuf, sizeof(hexbuf), "%02x", *str++);
+    snprintf (hexbuf, sizeof(hexbuf), "%02x", c);
     hex_str += hexbuf;
   }
   return hex_str;
 }
 
 void append_hexified_string(std::ostream &ostrm, const std::string &string) {
   size_t string_size = string.size();
   const char *string_buf = string.c_str();