From 4c19cbce5742eeb122f55065eecfed93d3bc8016 Mon Sep 17 00:00:00 2001
From: Boris Zbarsky <bzbarsky@apple.com>
Date: Wed, 17 Jun 2020 13:52:47 -0400
Subject: [PATCH] Fix discrimination between data model messages and echo
 messages. (#1151)

We don't want to take the data model message codepath if we know it's
not a valid data model message based on the first byte of the message.

Also, echo messages do not generally need to be null-terminated, so we
also fix the chip-tool command-line app to not send a null terminator
for those.
---
 examples/chip-tool/main.cpp                   |  4 +--
 .../server/esp32/main/EchoServer.cpp          | 36 ++++++++++++-------
 2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/examples/chip-tool/main.cpp b/examples/chip-tool/main.cpp
index e9609536f684d0..1019d13de54d73 100644
--- a/examples/chip-tool/main.cpp
+++ b/examples/chip-tool/main.cpp
@@ -208,7 +208,7 @@ bool DetermineCommandArgs(int argc, char * argv[], Command command, CommandArgs
 // Handle the echo case, where we just send a string and expect to get it back.
 void DoEcho(DeviceController::ChipDeviceController * controller, const IPAddress & host_addr, uint16_t port)
 {
-    size_t payload_len = strlen(PAYLOAD) + 1;
+    size_t payload_len = strlen(PAYLOAD);
 
     // Run the client
     char host_ip_str[40];
@@ -218,7 +218,7 @@ void DoEcho(DeviceController::ChipDeviceController * controller, const IPAddress
         // Reallocate buffer on each run, as the secure transport encrypts and
         // overwrites the buffer from previous iteration.
         auto * buffer = System::PacketBuffer::NewWithAvailableSize(payload_len);
-        snprintf((char *) buffer->Start(), payload_len, "%s", PAYLOAD);
+        memcpy(buffer->Start(), PAYLOAD, payload_len);
         buffer->SetDataLength(payload_len);
 
         controller->SendMessage(NULL, buffer);
diff --git a/examples/wifi-echo/server/esp32/main/EchoServer.cpp b/examples/wifi-echo/server/esp32/main/EchoServer.cpp
index e77e92bb84daf6..889bca85f81299 100644
--- a/examples/wifi-echo/server/esp32/main/EchoServer.cpp
+++ b/examples/wifi-echo/server/esp32/main/EchoServer.cpp
@@ -68,22 +68,28 @@ const unsigned char remote_public_key[] = { 0x04, 0x30, 0x77, 0x2c, 0xe7, 0xd4,
                                             0x44, 0x72, 0x1b, 0xcd, 0xb9, 0x02, 0x53, 0xd9, 0xaf, 0xcc, 0x1a, 0xcd, 0xae,
                                             0xe8, 0x87, 0x2e, 0x52, 0x3b, 0x98, 0xf0, 0xa1, 0x88, 0x4a, 0xe3, 0x03, 0x75 };
 
-/// A printable string has all characters printable and ends with a '\0'
-bool ContentIsAPrintableString(System::PacketBuffer * buffer)
+/**
+ * A data model message has nonzero length and always has a first byte whose
+ * value is one of: 0x00, 0x01, 0x02, 0x03.  See chipZclEncodeZclHeader for the
+ * construction of the message and in particular the first byte.
+ *
+ * Echo messages should generally not have a first byte with those values, so we
+ * can use that to try to distinguish between the two.
+ */
+static bool ContentMayBeADataModelMessage(System::PacketBuffer * buffer)
 {
-    const size_t data_len = buffer->DataLength();
-    const uint8_t * data  = buffer->Start();
-    bool isPrintable      = true;
+    const size_t data_len      = buffer->DataLength();
+    const uint8_t * data       = buffer->Start();
+    bool maybeDataModelMessage = true;
 
-    // Has to end with a 0 terminator
-    VerifyOrExit(data_len > 0, isPrintable = false);
-    VerifyOrExit(data[data_len - 1] == 0, isPrintable = false);
+    // Has to have nonzero length.
+    VerifyOrExit(data_len > 0, maybeDataModelMessage = false);
 
-    // all other characters are printable
-    isPrintable = std::all_of(data, data + data_len - 1, isprint);
+    // Has to have a valid first byte value.
+    VerifyOrExit(data[0] < 0x04, maybeDataModelMessage = false);
 
 exit:
-    return isPrintable;
+    return maybeDataModelMessage;
 }
 
 void newConnectionHandler(const MessageHeader & header, const IPPacketInfo & packet_info, SecureSessionMgr * transport)
@@ -127,9 +133,13 @@ void echo(const MessageHeader & header, const IPPacketInfo & packet_info, System
                  packet_info.DestPort, static_cast<size_t>(data_len));
     }
 
-    if (!ContentIsAPrintableString(buffer))
+    // FIXME: Long-term we shouldn't be guessing what sort of message this is
+    // based on the message bytes.  We're doing this for now to support both
+    // data model messages and text echo messages, but in the long term we
+    // should either do echo via a data model command or do echo on a separate
+    // port from data model processing.
+    if (ContentMayBeADataModelMessage(buffer))
     {
-        // Non-ACII; assume it's a data model message.
         HandleDataModelMessage(buffer);
         buffer = NULL;
     }