[lldb] Correct format of qMemTags type field

[DavidSpickett]

Summary

The type field is a signed integer.
(https://sourceware.org/gdb/current/onlinedocs/gdb/General-Query-Packets.html)

However it's not packed in the packet in the way
you might think. For example the type -1 should be:
qMemTags:<addr>,<len>:ffffffff
Instead of:
qMemTags:<addr>,<len>:-1

This change makes lldb-server's parsing more strict
and adds more tests to check that we handle negative types
correctly in lldb and lldb-server.

We only support one tag type value at this point,
for AArch64 MTE, which is positive. So this doesn't change
any of those interactions. It just brings us in line with GDB.

Also check that the test target has MTE. Previously
we just checked that we were AArch64 with a toolchain
that supports MTE.

Finally, update the tag type check for QMemTags to use
the same conversion steps that qMemTags now does.
Using static_cast can invoke UB and though we do do a limit
check to avoid this, I think it's clearer with the new method.

[omjavaid]

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

3477

First condition looks redundant given that anything above 32 bit range is invalid and being tested in next condition?

[omjavaid]

lldb/test/API/tools/lldb-server/memory-tagging/TestGdbRemoteMemoryTagging.py

22

Missing isAArch64MTE check here?

[DavidSpickett]

• Remove redundant check on type value
• Explain how we check the target has MTE

[DavidSpickett]

Any other comments?

lldb/test/API/tools/lldb-server/memory-tagging/TestGdbRemoteMemoryTagging.py

22

Added a comment to explain this.

[omjavaid]

I am trying to run test_qmemtags_packets but the test hangs while waiting for lldb-server to terminate.

My test compiler is gcc-linaro-11.0.0-2021.02-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-gcc (Monthly Pre built binaries uploaded by linaro)

using commandline lldb-dotest -A aarch64 -C compiler --platform-name=remote-linux --platform-url=<QEMU VM URL>

[DavidSpickett]

I am trying to run test_qmemtags_packets but the test hangs while waiting for lldb-server to terminate.

Do you have any more detail than that? I presume it runs the test then hangs during the cleanup. Without this patch do you see the same thing?

I'm using Kernel 5.14-rc1, Qemu 6.0.0 and a gcc 10.2 I built with crosstool NG (because of some glibc version issues). The compiler could be the difference.

[DavidSpickett]

• Rebase onto main
• Update the way we cast down to int32_t to avoid use of static_cast.

We weren't invoking the undefined behaviour of unsigned to signed
due to the limit check. However I think this shows our intent more clearly.

It's essentially C++20's std::bitcast.

Also I was unable to reproduce the test failures,
ok to land this as is given that QMemTags has similair tests
and is already on main?

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

 //===-- GDBRemoteCommunicationServerLLGS.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
 //
 //===----------------------------------------------------------------------===//
 
 #include <cerrno>
 
 #include "lldb/Host/Config.h"
 
 
 #include <chrono>
 #include <cstring>
 #include <limits>
 #include <thread>
 
 #include "GDBRemoteCommunicationServerLLGS.h"
return SendIllFormedResponse(
         packet, "Too short qMemtags: packet (looking for length)");
   const size_t length = packet.GetHexMaxU64(/*little_endian=*/false, 0);
 
   // Type
   const char *invalid_type_err = "Invalid type field in qMemTags: packet";
   if (packet.GetBytesLeft() < 1 || packet.GetChar() != ':')
     return SendIllFormedResponse(packet, invalid_type_err);
 
-  int32_t type =
-      packet.GetS32(std::numeric_limits<int32_t>::max(), /*base=*/16);
-  if (type == std::numeric_limits<int32_t>::max() ||
+  // Type is a signed integer but packed into the packet as its raw bytes.
+  // However, our GetU64 uses strtoull which allows +/-. We do not want this.
+  const char *first_type_char = packet.Peek();
+  if (first_type_char && (*first_type_char == '+' || *first_type_char == '-'))
+    return SendIllFormedResponse(packet, invalid_type_err);
+
+  // Extract type as unsigned then cast to signed.
+  // Using a uint64_t here so that we have some value outside of the 32 bit
+  // range to use as the invalid return value.
+  uint64_t raw_type =
+      packet.GetU64(std::numeric_limits<uint64_t>::max(), /*base=*/16);
+
+  if (raw_type == std::numeric_limits<uint64_t>::max() ||
+      // Make sure the cast below would be valid
+      raw_type > std::numeric_limits<uint32_t>::max() ||
       // To catch inputs like "123aardvark" that will parse but clearly aren't
       // valid in this case.
       packet.GetBytesLeft()) {
     return SendIllFormedResponse(packet, invalid_type_err);
   }
+  int32_t type = static_cast<int32_t>(raw_type);
 
   StreamGDBRemote response;
   std::vector<uint8_t> tags;
   Status error = m_current_process->ReadMemoryTags(type, addr, length, tags);
   if (error.Fail())
     return SendErrorResponse(1);
 
   // This m is here in case we want to support multi part replies in the future.

lldb/test/API/tools/lldb-server/memory-tagging/TestGdbRemoteMemoryTagging.py

def check_qmemtags_response(self, body, expected):
                                          True)
         self.expect_gdbremote_sequence()
 
     @skipUnlessArch("aarch64")
     @skipUnlessPlatform(["linux"])
     @skipUnlessAArch64MTELinuxCompiler
     def test_qmemtags_packets(self):
         """ Test that qMemTags packets are parsed correctly and/or rejected. """
 
         self.build()
         self.set_inferior_startup_launch()
         procs = self.prep_debug_monitor_and_inferior()
 
         # Run the process
         self.test_sequence.add_log_lines(
             [
                 # Start running after initial stop
# Match the address of the MTE page
         # Invalid lengths
         self.check_qmemtags_response("{:x},food:1".format(buf_address), "E03")
         self.check_qmemtags_response("{:x},-1:1".format(buf_address), "E03")
         self.check_qmemtags_response("{:x},12??:1".format(buf_address), "E03")
         # Empty type
         self.check_qmemtags_response("{:x},10:".format(buf_address), "E03")
         # Types we don't support
         self.check_qmemtags_response("{:x},10:FF".format(buf_address), "E01")
+        # Types can also be negative, so this is E01 for not supported,
+        # instead of E03 for invalid formatting.
+        self.check_qmemtags_response("{:x},10:FFFFFFFF".format(buf_address), "E01")
         # (even if the length of the read is zero)
         self.check_qmemtags_response("{:x},0:FF".format(buf_address), "E01")
-        self.check_qmemtags_response("{:x},10:-1".format(buf_address), "E01")
-        self.check_qmemtags_response("{:x},10:+20".format(buf_address), "E01")
         # Invalid type format
         self.check_qmemtags_response("{:x},10:cat".format(buf_address), "E03")
         self.check_qmemtags_response("{:x},10:?11".format(buf_address), "E03")
+        # Type is signed but in packet as raw bytes, no +/-.
+        self.check_qmemtags_response("{:x},10:-1".format(buf_address), "E03")
+        self.check_qmemtags_response("{:x},10:+20".format(buf_address), "E03")
+        # We do use a uint64_t for unpacking but that's just an implementation
+        # detail. Any value > 32 bit is invalid.
+        self.check_qmemtags_response("{:x},10:123412341".format(buf_address), "E03")
 
         # Valid packets
 
         # Reading nothing is allowed
         self.check_qmemtags_response("{:x},0:1".format(buf_address), "m")
         # A range that's already aligned
         self.check_qmemtags_response("{:x},20:1".format(buf_address), "m0001")
         # lldb-server should re-align the range

lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp

 //===-- GDBRemoteCommunicationClientTest.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
 //
 //===----------------------------------------------------------------------===//
 #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h"
 #include "GDBRemoteTestUtils.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/XML.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/StructuredData.h"
 #include "lldb/lldb-enumerations.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include <future>
+#include <limits>
 
 using namespace lldb_private::process_gdb_remote;
 using namespace lldb_private;
 using namespace lldb;
 using namespace llvm;
 
 namespace {
 
TEST_F(GDBRemoteCommunicationClientTest, GetQOffsets) {
   EXPECT_EQ(llvm::None, GetQOffsets("Text=1234;Data=1234;Bss=1234;"));
   EXPECT_EQ(llvm::None, GetQOffsets("TEXTSEG=1234"));
   EXPECT_EQ(llvm::None, GetQOffsets("TextSeg=0x1234"));
   EXPECT_EQ(llvm::None, GetQOffsets("TextSeg=12345678123456789"));
 }
 
 static void
 check_qmemtags(TestClient &client, MockServer &server, size_t read_len,
-               const char *packet, llvm::StringRef response,
+               int32_t type, const char *packet, llvm::StringRef response,
                llvm::Optional<std::vector<uint8_t>> expected_tag_data) {
-  const auto &ReadMemoryTags = [&](size_t len, const char *packet,
-                                   llvm::StringRef response) {
+  const auto &ReadMemoryTags = [&]() {
     std::future<DataBufferSP> result = std::async(std::launch::async, [&] {
-      return client.ReadMemoryTags(0xDEF0, read_len, 1);
+      return client.ReadMemoryTags(0xDEF0, read_len, type);
     });
 
     HandlePacket(server, packet, response);
     return result.get();
   };
 
-  auto result = ReadMemoryTags(0, packet, response);
+  auto result = ReadMemoryTags();
   if (expected_tag_data) {
     ASSERT_TRUE(result);
     llvm::ArrayRef<uint8_t> expected(*expected_tag_data);
     llvm::ArrayRef<uint8_t> got = result->GetData();
     ASSERT_THAT(expected, testing::ContainerEq(got));
   } else {
     ASSERT_FALSE(result);
   }
 }
 
 TEST_F(GDBRemoteCommunicationClientTest, ReadMemoryTags) {
   // Zero length reads are valid
-  check_qmemtags(client, server, 0, "qMemTags:def0,0:1", "m",
+  check_qmemtags(client, server, 0, 1, "qMemTags:def0,0:1", "m",
                  std::vector<uint8_t>{});
 
+  // Type can be negative. Put into the packet as the raw bytes
+  // (as opposed to a literal -1)
+  check_qmemtags(client, server, 0, -1, "qMemTags:def0,0:ffffffff", "m",
+                 std::vector<uint8_t>{});
+  check_qmemtags(client, server, 0, std::numeric_limits<int32_t>::min(),
+                 "qMemTags:def0,0:80000000", "m", std::vector<uint8_t>{});
+  check_qmemtags(client, server, 0, std::numeric_limits<int32_t>::max(),
+                 "qMemTags:def0,0:7fffffff", "m", std::vector<uint8_t>{});
+
   // The client layer does not check the length of the received data.
   // All we need is the "m" and for the decode to use all of the chars
-  check_qmemtags(client, server, 32, "qMemTags:def0,20:1", "m09",
+  check_qmemtags(client, server, 32, 2, "qMemTags:def0,20:2", "m09",
                  std::vector<uint8_t>{0x9});
 
   // Zero length response is fine as long as the "m" is present
-  check_qmemtags(client, server, 0, "qMemTags:def0,0:1", "m",
+  check_qmemtags(client, server, 0, 0x34, "qMemTags:def0,0:34", "m",
                  std::vector<uint8_t>{});
 
   // Normal responses
-  check_qmemtags(client, server, 16, "qMemTags:def0,10:1", "m66",
+  check_qmemtags(client, server, 16, 1, "qMemTags:def0,10:1", "m66",
                  std::vector<uint8_t>{0x66});
-  check_qmemtags(client, server, 32, "qMemTags:def0,20:1", "m0102",
+  check_qmemtags(client, server, 32, 1, "qMemTags:def0,20:1", "m0102",
                  std::vector<uint8_t>{0x1, 0x2});
 
   // Empty response is an error
-  check_qmemtags(client, server, 17, "qMemTags:def0,11:1", "", llvm::None);
+  check_qmemtags(client, server, 17, 1, "qMemTags:def0,11:1", "", llvm::None);
   // Usual error response
-  check_qmemtags(client, server, 17, "qMemTags:def0,11:1", "E01", llvm::None);
+  check_qmemtags(client, server, 17, 1, "qMemTags:def0,11:1", "E01",
+                 llvm::None);
   // Leading m missing
-  check_qmemtags(client, server, 17, "qMemTags:def0,11:1", "01", llvm::None);
+  check_qmemtags(client, server, 17, 1, "qMemTags:def0,11:1", "01", llvm::None);
   // Anything other than m is an error
-  check_qmemtags(client, server, 17, "qMemTags:def0,11:1", "z01", llvm::None);
+  check_qmemtags(client, server, 17, 1, "qMemTags:def0,11:1", "z01",
+                 llvm::None);
   // Decoding tag data doesn't use all the chars in the packet
-  check_qmemtags(client, server, 32, "qMemTags:def0,20:1", "m09zz", llvm::None);
+  check_qmemtags(client, server, 32, 1, "qMemTags:def0,20:1", "m09zz",
+                 llvm::None);
   // Data that is not hex bytes
-  check_qmemtags(client, server, 32, "qMemTags:def0,20:1", "mhello",
+  check_qmemtags(client, server, 32, 1, "qMemTags:def0,20:1", "mhello",
                  llvm::None);
   // Data is not a complete hex char
-  check_qmemtags(client, server, 32, "qMemTags:def0,20:1", "m9", llvm::None);
+  check_qmemtags(client, server, 32, 1, "qMemTags:def0,20:1", "m9", llvm::None);
   // Data has a trailing hex char
-  check_qmemtags(client, server, 32, "qMemTags:def0,20:1", "m01020",
+  check_qmemtags(client, server, 32, 1, "qMemTags:def0,20:1", "m01020",
                  llvm::None);
 }