Implement QName compression in minmdns (#12445)

* Revamp constants: qname length constant is NOT used

* First pass on an indirection  to use a dedicated writer. No test updates yet nor functionality

* Updates to use the record writer, more tests pass now

* Compressed writer with unit tests

* Fix memory leak in UDP responders when a multicast join fails

* Add a harder unit test

* Make the complex reuse also validate that we can write things other than qnames

* Compression applied on all answers (no queries yet)

* Prepare to make query writing do compression as well, add more unit tests to qname writing

* Record compression for writing serialized qnames as well. Tested that query compression on replies is done

* Fix unit tests for resource records for writing QName compression

* Code review comments, fix android compile

* Fix test advertiser to consider the entire valid packet range when validating (for qname compression support)

* Only remember non-fully compressed  qnames

* Address some  code review comments

* Add a fit validation for response building

* update the code to have RecordWriter be more transparent/easier to use in put statements

.vscode/settings.json

@@ -156,5 +156,6 @@
     "clang-format.fallbackStyle": "WebKit",
     "files.trimFinalNewlines": true,
     "C_Cpp.default.cppStandard": "gnu++14",
-    "C_Cpp.default.cStandard": "gnu11"
+    "C_Cpp.default.cStandard": "gnu11",
+    "cmake.configureOnOpen": false
 }

src/lib/dnssd/minimal_mdns/Parser.cpp

@@ -66,23 +66,24 @@ bool QueryData::Parse(const BytesRange & validData, const uint8_t ** start)
     return true;
 }
 
-bool QueryData::Append(HeaderRef & hdr, chip::Encoding::BigEndian::BufferWriter & out) const
+bool QueryData::Append(HeaderRef & hdr, RecordWriter & out) const
 {
     if ((hdr.GetAdditionalCount() != 0) || (hdr.GetAnswerCount() != 0) || (hdr.GetAuthorityCount() != 0))
     {
         return false;
     }
 
-    GetName().Put(out);
-    out.Put16(static_cast<uint16_t>(mType));
-    out.Put16(static_cast<uint16_t>(mClass) | (mAnswerViaUnicast ? kQClassUnicastAnswerFlag : 0));
+    out.WriteQName(GetName())
+        .Put16(static_cast<uint16_t>(mType))
+        .Put16(static_cast<uint16_t>(mClass) | (mAnswerViaUnicast ? kQClassUnicastAnswerFlag : 0));
 
-    if (out.Fit())
+    if (!out.Fit())
     {
-        hdr.SetQueryCount(static_cast<uint16_t>(hdr.GetQueryCount() + 1));
+        return false;
     }
 
-    return out.Fit();
+    hdr.SetQueryCount(static_cast<uint16_t>(hdr.GetQueryCount() + 1));
+    return true;
 }
 
 bool ResourceData::Parse(const BytesRange & validData, const uint8_t ** start)

src/lib/dnssd/minimal_mdns/Parser.h

@@ -20,6 +20,7 @@
 #include <lib/dnssd/minimal_mdns/core/Constants.h>
 #include <lib/dnssd/minimal_mdns/core/DnsHeader.h>
 #include <lib/dnssd/minimal_mdns/core/QName.h>
+#include <lib/dnssd/minimal_mdns/core/RecordWriter.h>
 
 namespace mdns {
 namespace Minimal {
@@ -56,7 +57,7 @@ class QueryData
     bool Parse(const BytesRange & validData, const uint8_t ** start);
 
     /// Write out this query data back into an output buffer.
-    bool Append(HeaderRef & hdr, chip::Encoding::BigEndian::BufferWriter & out) const;
+    bool Append(HeaderRef & hdr, RecordWriter & out) const;
 
 private:
     QType mType            = QType::ANY;

src/lib/dnssd/minimal_mdns/Query.h

@@ -21,6 +21,7 @@
 
 #include <lib/dnssd/minimal_mdns/core/Constants.h>
 #include <lib/dnssd/minimal_mdns/core/QName.h>
+#include <lib/dnssd/minimal_mdns/core/RecordWriter.h>
 
 namespace mdns {
 namespace Minimal {
@@ -56,25 +57,25 @@ class Query
     ///
     /// @param hdr will be updated with a query count
     /// @param out where to write the query data
-    bool Append(HeaderRef & hdr, chip::Encoding::BigEndian::BufferWriter & out) const
+    bool Append(HeaderRef & hdr, RecordWriter & out) const
     {
         // Questions can only be appended before any other data is added
         if ((hdr.GetAdditionalCount() != 0) || (hdr.GetAnswerCount() != 0) || (hdr.GetAuthorityCount() != 0))
         {
             return false;
         }
 
-        mQName.Output(out);
+        out.WriteQName(mQName)
+            .Put16(static_cast<uint16_t>(mType))
+            .Put16(static_cast<uint16_t>(static_cast<uint16_t>(mClass) | (mAnswerViaUnicast ? kQClassUnicastAnswerFlag : 0)));
 
-        out.Put16(static_cast<uint16_t>(mType));
-        out.Put16(static_cast<uint16_t>(static_cast<uint16_t>(mClass) | (mAnswerViaUnicast ? kQClassUnicastAnswerFlag : 0)));
-
-        if (out.Fit())
+        if (!out.Fit())
         {
-            hdr.SetQueryCount(static_cast<uint16_t>(hdr.GetQueryCount() + 1));
+            return false;
         }
 
-        return out.Fit();
+        hdr.SetQueryCount(static_cast<uint16_t>(hdr.GetQueryCount() + 1));
+        return true;
     }
 
 private:

src/lib/dnssd/minimal_mdns/QueryBuilder.h

@@ -68,8 +68,9 @@ class QueryBuilder
         }
 
         chip::Encoding::BigEndian::BufferWriter out(mPacket->Start() + mPacket->DataLength(), mPacket->AvailableDataLength());
+        RecordWriter writer(&out);
 
-        if (!query.Append(mHeader, out))
+        if (!query.Append(mHeader, writer))
         {
             mQueryBuildOk = false;
         }

src/lib/dnssd/minimal_mdns/ResponseBuilder.h

@@ -29,8 +29,12 @@ namespace Minimal {
 class ResponseBuilder
 {
 public:
-    ResponseBuilder() : mHeader(nullptr) {}
-    ResponseBuilder(chip::System::PacketBufferHandle && packet) : mHeader(nullptr) { Reset(std::move(packet)); }
+    ResponseBuilder() : mHeader(nullptr), mEndianOutput(nullptr, 0), mWriter(&mEndianOutput) {}
+    ResponseBuilder(chip::System::PacketBufferHandle && packet) :
+        mHeader(nullptr), mEndianOutput(nullptr, 0), mWriter(&mEndianOutput)
+    {
+        Reset(std::move(packet));
+    }
 
     ResponseBuilder & Reset(chip::System::PacketBufferHandle && packet)
     {
@@ -49,6 +53,13 @@ class ResponseBuilder
         }
 
         mHeader.SetFlags(mHeader.GetFlags().SetResponse());
+
+        mEndianOutput =
+            chip::Encoding::BigEndian::BufferWriter(mPacket->Start(), mPacket->DataLength() + mPacket->AvailableDataLength());
+        mEndianOutput.Skip(mPacket->DataLength());
+
+        mWriter.Reset();
+
         return *this;
     }
 
@@ -77,16 +88,16 @@ class ResponseBuilder
             return *this;
         }
 
-        chip::Encoding::BigEndian::BufferWriter out(mPacket->Start() + mPacket->DataLength(), mPacket->AvailableDataLength());
-
-        if (!record.Append(mHeader, type, out))
+        if (!record.Append(mHeader, type, mWriter))
         {
             mBuildOk = false;
         }
         else
         {
-            mPacket->SetDataLength(static_cast<uint16_t>(mPacket->DataLength() + out.Needed()));
+            VerifyOrDie(mEndianOutput.Fit()); // should be guaranteed because record Append succeeded
+            mPacket->SetDataLength(static_cast<uint16_t>(mEndianOutput.Needed()));
         }
+
         return *this;
     }
 
@@ -97,15 +108,13 @@ class ResponseBuilder
             return *this;
         }
 
-        chip::Encoding::BigEndian::BufferWriter out(mPacket->Start() + mPacket->DataLength(), mPacket->AvailableDataLength());
-
-        if (!query.Append(mHeader, out))
+        if (!query.Append(mHeader, mWriter))
         {
             mBuildOk = false;
         }
         else
         {
-            mPacket->SetDataLength(static_cast<uint16_t>(mPacket->DataLength() + out.Needed()));
+            mPacket->SetDataLength(static_cast<uint16_t>(mEndianOutput.Needed()));
         }
         return *this;
     }
@@ -116,6 +125,8 @@ class ResponseBuilder
 private:
     chip::System::PacketBufferHandle mPacket;
     HeaderRef mHeader;
+    chip::Encoding::BigEndian::BufferWriter mEndianOutput;
+    RecordWriter mWriter;
     bool mBuildOk = false;
 };
 

src/lib/dnssd/minimal_mdns/core/BUILD.gn

@@ -21,6 +21,8 @@ static_library("core") {
     "DnsHeader.h",
     "QName.cpp",
     "QName.h",
+    "RecordWriter.cpp",
+    "RecordWriter.h",
   ]
 
   public_deps = [

src/lib/dnssd/minimal_mdns/core/BytesRange.h

@@ -44,6 +44,11 @@ class BytesRange
 
     size_t Size() const { return static_cast<size_t>(mEnd - mStart); }
 
+    inline static BytesRange BufferWithSize(const void * buff, size_t len)
+    {
+        return BytesRange(static_cast<const uint8_t *>(buff), static_cast<const uint8_t *>(buff) + len);
+    }
+
 private:
     const uint8_t * mStart = nullptr;
     const uint8_t * mEnd   = nullptr;

src/lib/dnssd/minimal_mdns/core/Constants.h

@@ -66,7 +66,5 @@ enum class ResourceType
     kAdditional,
 };
 
-constexpr size_t kMaxQNamePartLength = 255;
-
 } // namespace Minimal
 } // namespace mdns

src/lib/dnssd/minimal_mdns/core/QName.cpp

@@ -152,6 +152,40 @@ bool SerializedQNameIterator::operator==(const FullQName & other) const
     return ((idx == other.nameCount) && !self.Next());
 }
 
+bool SerializedQNameIterator::operator==(const SerializedQNameIterator & other) const
+{
+    SerializedQNameIterator a = *this; // allow iteration
+    SerializedQNameIterator b = other;
+
+    while (true)
+    {
+        bool hasA = a.Next();
+        bool hasB = b.Next();
+
+        if (hasA ^ hasB)
+        {
+            return false; /// one is longer than the other
+        }
+
+        if (!a.IsValid() || !b.IsValid())
+        {
+            return false; // invalid data
+        }
+
+        if (!hasA || !hasB)
+        {
+            break;
+        }
+
+        if (strcasecmp(a.Value(), b.Value()) != 0)
+        {
+            return false;
+        }
+    }
+
+    return a.IsValid() && b.IsValid();
+}
+
 bool FullQName::operator==(const FullQName & other) const
 {
     if (nameCount != other.nameCount)

src/lib/dnssd/minimal_mdns/core/QName.h

@@ -53,17 +53,6 @@ struct FullQName
     FullQName(const QNamePart (&data)[N]) : names(data), nameCount(N)
     {}
 
-    void Output(chip::Encoding::BigEndian::BufferWriter & out) const
-    {
-        for (uint16_t i = 0; i < nameCount; i++)
-        {
-
-            out.Put8(static_cast<uint8_t>(strlen(names[i])));
-            out.Put(names[i]);
-        }
-        out.Put8(0); // end of qnames
-    }
-
     bool operator==(const FullQName & other) const;
     bool operator!=(const FullQName & other) const { return !(*this == other); }
 };
@@ -108,17 +97,10 @@ class SerializedQNameIterator
     bool operator==(const FullQName & other) const;
     bool operator!=(const FullQName & other) const { return !(*this == other); }
 
-    void Put(chip::Encoding::BigEndian::BufferWriter & out) const
-    {
-        SerializedQNameIterator copy = *this;
-        while (copy.Next())
-        {
-
-            out.Put8(static_cast<uint8_t>(strlen(copy.Value())));
-            out.Put(copy.Value());
-        }
-        out.Put8(0); // end of qnames
-    }
+    bool operator==(const SerializedQNameIterator & other) const;
+    bool operator!=(const SerializedQNameIterator & other) const { return !(*this == other); }
+
+    size_t OffsetInCurrentValidData() const { return static_cast<size_t>(mCurrentPosition - mValidData.Start()); }
 
 private:
     static constexpr size_t kMaxValueSize = 63;