[Lldb-commits] [lldb] r328693 - gdb-remote: Fix checksum verification for messages with escape chars
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Wed Mar 28 03:19:10 PDT 2018
Author: labath
Date: Wed Mar 28 03:19:10 2018
New Revision: 328693
URL: http://llvm.org/viewvc/llvm-project?rev=328693&view=rev
Log:
gdb-remote: Fix checksum verification for messages with escape chars
Summary:
We've had a mismatch in the checksum computation between the sender and
receiver. The sender computed the payload checksum using the wire
encoding of the packet, while the receiver did this after expanding
un-escaping and expanding run-length-encoded sequences. This resulted in
communication breakdown if packets using these feature were sent in the
ack mode.
Normally, this did not cause any issues since the only packet we send in
the ack-mode is the QStartNoAckMode packet, but I ran into this when
debugging the lldb-server tests which (for better or worse) don't use
this mode.
According to the gdb-remote documentation "The two-digit checksum is computed as
the modulo 256 sum of all characters between the leading ‘$’ and the
trailing ‘#’", it seems that our sender is doing the right thing here.
Therefore, I fix the receiver the match the sender behavior and add a
test.
With this bug fixed, we can see that lldb-server is sending a stop-reply
after receiving the "k" in the same way as debugserver does (but we
weren't detecting this because at that point the connection was dead
already). I fix that expectation as well.
Reviewers: clayborg, jasonmolenda
Subscribers: mgorny, lldb-commits
Differential Revision: https://reviews.llvm.org/D44922
Added:
lldb/trunk/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
Modified:
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
lldb/trunk/unittests/Process/gdb-remote/CMakeLists.txt
lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp
Modified: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp?rev=328693&r1=328692&r2=328693&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp (original)
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp Wed Mar 28 03:19:10 2018
@@ -914,7 +914,8 @@ GDBRemoteCommunication::CheckForPacket(c
if (GetSendAcks()) {
const char *packet_checksum_cstr = &m_bytes[checksum_idx];
char packet_checksum = strtol(packet_checksum_cstr, NULL, 16);
- char actual_checksum = CalculcateChecksum(packet_str);
+ char actual_checksum = CalculcateChecksum(
+ llvm::StringRef(m_bytes).slice(content_start, content_end));
success = packet_checksum == actual_checksum;
if (!success) {
if (log)
Modified: lldb/trunk/unittests/Process/gdb-remote/CMakeLists.txt
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Process/gdb-remote/CMakeLists.txt?rev=328693&r1=328692&r2=328693&view=diff
==============================================================================
--- lldb/trunk/unittests/Process/gdb-remote/CMakeLists.txt (original)
+++ lldb/trunk/unittests/Process/gdb-remote/CMakeLists.txt Wed Mar 28 03:19:10 2018
@@ -1,6 +1,7 @@
add_lldb_unittest(ProcessGdbRemoteTests
GDBRemoteClientBaseTest.cpp
GDBRemoteCommunicationClientTest.cpp
+ GDBRemoteCommunicationTest.cpp
GDBRemoteTestUtils.cpp
LINK_LIBS
Added: lldb/trunk/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp?rev=328693&view=auto
==============================================================================
--- lldb/trunk/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp (added)
+++ lldb/trunk/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp Wed Mar 28 03:19:10 2018
@@ -0,0 +1,67 @@
+//===-- GDBRemoteCommunicationTest.cpp --------------------------*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+#include "GDBRemoteTestUtils.h"
+#include "llvm/Testing/Support/Error.h"
+
+using namespace lldb_private::process_gdb_remote;
+using namespace lldb_private;
+using namespace lldb;
+typedef GDBRemoteCommunication::PacketResult PacketResult;
+
+namespace {
+
+class TestClient : public GDBRemoteCommunication {
+public:
+ TestClient()
+ : GDBRemoteCommunication("test.client", "test.client.listener") {}
+
+ PacketResult ReadPacket(StringExtractorGDBRemote &response) {
+ return GDBRemoteCommunication::ReadPacket(response, std::chrono::seconds(1),
+ /*sync_on_timeout*/ false);
+ }
+};
+
+class GDBRemoteCommunicationTest : public GDBRemoteTest {
+public:
+ void SetUp() override {
+ ASSERT_THAT_ERROR(Connect(client, server), llvm::Succeeded());
+ }
+
+protected:
+ TestClient client;
+ MockServer server;
+
+ bool Write(llvm::StringRef packet) {
+ ConnectionStatus status;
+ return server.Write(packet.data(), packet.size(), status, nullptr) ==
+ packet.size();
+ }
+};
+} // end anonymous namespace
+
+TEST_F(GDBRemoteCommunicationTest, ReadPacket_checksum) {
+ struct TestCase {
+ llvm::StringLiteral Packet;
+ llvm::StringLiteral Payload;
+ };
+ static constexpr TestCase Tests[] = {
+ {{"$#00"}, {""}},
+ {{"$foobar#79"}, {"foobar"}},
+ {{"$}}#fa"}, {"]"}},
+ {{"$x*%#c7"}, {"xxxxxxxxx"}},
+ };
+ for (const auto &Test : Tests) {
+ SCOPED_TRACE(Test.Packet + " -> " + Test.Payload);
+ StringExtractorGDBRemote response;
+ ASSERT_TRUE(Write(Test.Packet));
+ ASSERT_EQ(PacketResult::Success, client.ReadPacket(response));
+ ASSERT_EQ(Test.Payload, response.GetStringRef());
+ ASSERT_EQ(PacketResult::Success, server.GetAck());
+ }
+}
Modified: lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp?rev=328693&r1=328692&r2=328693&view=diff
==============================================================================
--- lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp (original)
+++ lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp Wed Mar 28 03:19:10 2018
@@ -37,12 +37,7 @@ TestClient::~TestClient() {
if (!IsConnected())
return;
- std::string response;
- // Debugserver (non-conformingly?) sends a reply to the k packet instead of
- // simply closing the connection.
- PacketResult result =
- IsDebugServer() ? PacketResult::Success : PacketResult::ErrorDisconnected;
- EXPECT_THAT_ERROR(SendMessage("k", response, result), Succeeded());
+ EXPECT_THAT_ERROR(SendMessage("k"), Succeeded());
}
Expected<std::unique_ptr<TestClient>> TestClient::launch(StringRef Log) {
More information about the lldb-commits
mailing list