[Lldb-commits] [lldb] [lldb/gdb-remote] Do not crash on an invalid server response (PR #131979)

[Igor Kudrin via lldb-commits]

https://github.com/igorkudrin created https://github.com/llvm/llvm-project/pull/131979

An invalid RLE sequence in the received packet could result in an out-of-bounds reading that could cause a crash.

>From 239aeaa5686350681dc79cad63d5aae5a5f2310a Mon Sep 17 00:00:00 2001
From: Igor Kudrin <ikudrin at accesssoftek.com>
Date: Tue, 18 Mar 2025 23:24:55 -0700
Subject: [PATCH] [lldb/gdb-remote] Do not crash on an invalid server response

An invalid RLE sequence in the received packet could result in an
out-of-bounds reading that could cause a crash.
---
 .../gdb-remote/GDBRemoteCommunication.cpp     | 22 ++++++++++++++----
 .../gdb-remote/GDBRemoteCommunication.h       |  2 +-
 .../gdb-remote/GDBRemoteCommunicationTest.cpp | 23 +++++++++++++++++++
 3 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
index dad72a176b5fa..77eadfc8c9f6c 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -788,9 +788,14 @@ GDBRemoteCommunication::CheckForPacket(const uint8_t *src, size_t src_len,
 
       // Copy the packet from m_bytes to packet_str expanding the run-length
       // encoding in the process.
-      std ::string packet_str =
+      auto maybe_packet_str =
           ExpandRLE(m_bytes.substr(content_start, content_end - content_start));
-      packet = StringExtractorGDBRemote(packet_str);
+      if (!maybe_packet_str) {
+        m_bytes.erase(0, total_length);
+        packet.Clear();
+        return GDBRemoteCommunication::PacketType::Invalid;
+      }
+      packet = StringExtractorGDBRemote(*maybe_packet_str);
 
       if (m_bytes[0] == '$' || m_bytes[0] == '%') {
         assert(checksum_idx < m_bytes.size());
@@ -1311,17 +1316,22 @@ void llvm::format_provider<GDBRemoteCommunication::PacketResult>::format(
   }
 }
 
-std::string GDBRemoteCommunication::ExpandRLE(std::string packet) {
+std::optional<std::string>
+GDBRemoteCommunication::ExpandRLE(std::string packet) {
   // Reserve enough byte for the most common case (no RLE used).
   std::string decoded;
   decoded.reserve(packet.size());
   for (std::string::const_iterator c = packet.begin(); c != packet.end(); ++c) {
     if (*c == '*') {
+      if (decoded.empty())
+        return std::nullopt;
       // '*' indicates RLE. Next character will give us the repeat count and
       // previous character is what is to be repeated.
       char char_to_repeat = decoded.back();
       // Number of time the previous character is repeated.
-      int repeat_count = *++c + 3 - ' ';
+      if (++c == packet.end())
+        return std::nullopt;
+      int repeat_count = *c + 3 - ' ';
       // We have the char_to_repeat and repeat_count. Now push it in the
       // packet.
       for (int i = 0; i < repeat_count; ++i)
@@ -1329,7 +1339,9 @@ std::string GDBRemoteCommunication::ExpandRLE(std::string packet) {
     } else if (*c == 0x7d) {
       // 0x7d is the escape character.  The next character is to be XOR'd with
       // 0x20.
-      char escapee = *++c ^ 0x20;
+      if (++c == packet.end())
+        return std::nullopt;
+      char escapee = *c ^ 0x20;
       decoded.push_back(escapee);
     } else {
       decoded.push_back(*c);
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
index 754797ba7f504..107c0896c4e61 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
@@ -168,7 +168,7 @@ class GDBRemoteCommunication : public Communication {
                                     GDBRemoteCommunication &server);
 
   /// Expand GDB run-length encoding.
-  static std::string ExpandRLE(std::string);
+  static std::optional<std::string> ExpandRLE(std::string);
 
 protected:
   std::chrono::seconds m_packet_timeout;
diff --git a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
index 425f6cfcd5bc9..3da1f0a718fc5 100644
--- a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
+++ b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
@@ -68,3 +68,26 @@ TEST_F(GDBRemoteCommunicationTest, ReadPacket) {
     ASSERT_EQ(PacketResult::Success, server.GetAck());
   }
 }
+
+// Test that packets with incorrect RLE sequences do not cause a crash and
+// reported as invalid.
+TEST_F(GDBRemoteCommunicationTest, CheckForPacket) {
+  using PacketType = GDBRemoteCommunication::PacketType;
+  struct TestCase {
+    llvm::StringLiteral Packet;
+    PacketType Result;
+  };
+  static constexpr TestCase Tests[] = {
+      {{"$#00"}, PacketType::Standard},
+      {{"$xx*#00"}, PacketType::Invalid}, // '*' without a count
+      {{"$*#00"}, PacketType::Invalid},   // '*' without a preceding character
+      {{"$xx}#00"}, PacketType::Invalid}, // bare escape character '}'
+      {{"%#00"}, PacketType::Notify},     // a correct packet after an invalid
+  };
+  for (const auto &Test : Tests) {
+    SCOPED_TRACE(Test.Packet);
+    StringExtractorGDBRemote response;
+    EXPECT_EQ(Test.Result, client.CheckForPacket(Test.Packet.bytes_begin(),
+                                                 Test.Packet.size(), response));
+  }
+}