From ed6fa0bd1a11f87043cd984e98db25bacea7e3d4 Mon Sep 17 00:00:00 2001
From: Michael Davidsaver <mdavidsaver@gmail.com>
Date: Mon, 19 Jul 2021 12:44:29 -0700
Subject: [PATCH] fix encoding of (Sub)Struct w/ valid set

---
 src/dataencode.cpp |  13 ++++--
 test/testxcode.cpp | 100 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 108 insertions(+), 5 deletions(-)

diff --git a/src/dataencode.cpp b/src/dataencode.cpp
index 4301418..ad85ff4 100644
--- a/src/dataencode.cpp
+++ b/src/dataencode.cpp
@@ -229,9 +229,10 @@ void to_wire_field(Buffer& buf, const FieldDesc* desc, const std::shared_ptr<con
             // serialize entire sub-structure
             for(auto off : range(desc->size())) {
                 auto cdesc = desc + off;
+                if(cdesc->code==TypeCode::Struct) // skip sub-struct nodes.  Would be redundant
+                    continue;
                 std::shared_ptr<const FieldStorage> cstore(store, store.get()+off); // TODO avoid shared_ptr/aliasing here
-                if(cdesc->code!=TypeCode::Struct)
-                    to_wire_field(buf, cdesc, cstore);
+                to_wire_field(buf, cdesc, cstore);
             }
         }
             return;
@@ -428,9 +429,13 @@ void to_wire_valid(Buffer& buf, const Value& val, const BitMask* mask)
 
     BitMask valid(desc->size());
 
-    for(auto bit : range(desc->size())) {
-        if((store.get()+bit)->valid && (!mask || (*mask)[bit]))
+    for(size_t bit=0u, N=desc->size(); bit<N;) {
+        if(store.get()[bit].valid && (!mask || (*mask)[bit])) {
             valid[bit] = true;
+            bit += desc[bit].size(); // maybe skip past entire sub-struct
+        } else {
+            bit++;
+        }
     }
 
     to_wire(buf, valid);
diff --git a/test/testxcode.cpp b/test/testxcode.cpp
index e1661e9..78cbb00 100644
--- a/test/testxcode.cpp
+++ b/test/testxcode.cpp
@@ -940,6 +940,103 @@ void testXCodeNTNDArray()
     testEq(msg, out);
 }
 
+void testRegressRedundantBitMask()
+{
+    testDiag("%s", __func__);
+
+    // NTScalar w/ uint32_t
+    uint8_t payload_type[] =
+            "\xfd\x01\x00\x80\x15\x65\x70\x69\x63\x73\x3a\x6e\x74\x2f\x4e\x54" \
+            "\x53\x63\x61\x6c\x61\x72\x3a\x31\x2e\x30\x06\x05\x76\x61\x6c\x75" \
+            "\x65\x22\x05\x61\x6c\x61\x72\x6d\xfd\x02\x00\x80\x07\x61\x6c\x61" \
+            "\x72\x6d\x5f\x74\x03\x08\x73\x65\x76\x65\x72\x69\x74\x79\x22\x06" \
+            "\x73\x74\x61\x74\x75\x73\x22\x07\x6d\x65\x73\x73\x61\x67\x65\x60" \
+            "\x09\x74\x69\x6d\x65\x53\x74\x61\x6d\x70\xfd\x03\x00\x80\x00\x03" \
+            "\x10\x73\x65\x63\x6f\x6e\x64\x73\x50\x61\x73\x74\x45\x70\x6f\x63" \
+            "\x68\x23\x0b\x6e\x61\x6e\x6f\x73\x65\x63\x6f\x6e\x64\x73\x22\x07" \
+            "\x75\x73\x65\x72\x54\x61\x67\x22\x07\x64\x69\x73\x70\x6c\x61\x79" \
+            "\xfd\x04\x00\x80\x00\x06\x08\x6c\x69\x6d\x69\x74\x4c\x6f\x77\x43" \
+            "\x09\x6c\x69\x6d\x69\x74\x48\x69\x67\x68\x43\x0b\x64\x65\x73\x63" \
+            "\x72\x69\x70\x74\x69\x6f\x6e\x60\x05\x75\x6e\x69\x74\x73\x60\x09" \
+            "\x70\x72\x65\x63\x69\x73\x69\x6f\x6e\x22\x04\x66\x6f\x72\x6d\xfd" \
+            "\x05\x00\x80\x06\x65\x6e\x75\x6d\x5f\x74\x02\x05\x69\x6e\x64\x65" \
+            "\x78\x22\x07\x63\x68\x6f\x69\x63\x65\x73\x68\x07\x63\x6f\x6e\x74" \
+            "\x72\x6f\x6c\xfd\x06\x00\x80\x09\x63\x6f\x6e\x74\x72\x6f\x6c\x5f" \
+            "\x74\x03\x08\x6c\x69\x6d\x69\x74\x4c\x6f\x77\x43\x09\x6c\x69\x6d" \
+            "\x69\x74\x48\x69\x67\x68\x43\x07\x6d\x69\x6e\x53\x74\x65\x70\x43" \
+            "\x0a\x76\x61\x6c\x75\x65\x41\x6c\x61\x72\x6d\xfd\x07\x00\x80\x0c" \
+            "\x76\x61\x6c\x75\x65\x41\x6c\x61\x72\x6d\x5f\x74\x0a\x06\x61\x63" \
+            "\x74\x69\x76\x65\x00\x0d\x6c\x6f\x77\x41\x6c\x61\x72\x6d\x4c\x69" \
+            "\x6d\x69\x74\x43\x0f\x6c\x6f\x77\x57\x61\x72\x6e\x69\x6e\x67\x4c" \
+            "\x69\x6d\x69\x74\x43\x10\x68\x69\x67\x68\x57\x61\x72\x6e\x69\x6e" \
+            "\x67\x4c\x69\x6d\x69\x74\x43\x0e\x68\x69\x67\x68\x41\x6c\x61\x72" \
+            "\x6d\x4c\x69\x6d\x69\x74\x43\x10\x6c\x6f\x77\x41\x6c\x61\x72\x6d" \
+            "\x53\x65\x76\x65\x72\x69\x74\x79\x22\x12\x6c\x6f\x77\x57\x61\x72" \
+            "\x6e\x69\x6e\x67\x53\x65\x76\x65\x72\x69\x74\x79\x22\x13\x68\x69" \
+            "\x67\x68\x57\x61\x72\x6e\x69\x6e\x67\x53\x65\x76\x65\x72\x69\x74" \
+            "\x79\x22\x11\x68\x69\x67\x68\x41\x6c\x61\x72\x6d\x53\x65\x76\x65" \
+            "\x72\x69\x74\x79\x22\x0a\x68\x79\x73\x74\x65\x72\x65\x73\x69\x73\x20"
+            ;
+
+
+    // initial monitor data payload from QSRV (with redundant bit mask)
+    // bit 0 is set for entire structure, but so are other bits
+    uint8_t payload_value[] =
+            "\x04\x81\xfb\x32\x1e\xf6\x00\x00\x70\x00\x00\x00\x00\x00\x00\x00" \
+            "\x00\x08\x4e\x4f\x5f\x41\x4c\x41\x52\x4d\x31\xa8\xf5\x60\x00\x00" \
+            "\x00\x00\xe5\xfd\x73\x04\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" \
+            "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" \
+            "\x00\x00\x00\x00\x07\x07\x44\x65\x66\x61\x75\x6c\x74\x06\x53\x74" \
+            "\x72\x69\x6e\x67\x06\x42\x69\x6e\x61\x72\x79\x07\x44\x65\x63\x69" \
+            "\x6d\x61\x6c\x03\x48\x65\x78\x0b\x45\x78\x70\x6f\x6e\x65\x6e\x74" \
+            "\x69\x61\x6c\x0b\x45\x6e\x67\x69\x6e\x65\x65\x72\x69\x6e\x67\x00" \
+            "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" \
+            "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xf8\x7f" \
+            "\x00\x00\x00\x00\x00\x00\xf8\x7f\x00\x00\x00\x00\x00\x00\xf8\x7f" \
+            "\x00\x00\x00\x00\x00\x00\xf8\x7f\x00\x00\x00\x00\x00\x00\x00\x00" \
+            "\x00\x00\x00\x00\x00\x00\x00\x00\x00"
+            ;
+
+    const char reencoded_value[] =
+            "\x01\x01\xf6\x00\x00\x70\x00\x00\x00\x00\x00\x00\x00" \
+            "\x00\x08\x4e\x4f\x5f\x41\x4c\x41\x52\x4d\x31\xa8\xf5\x60\x00\x00" \
+            "\x00\x00\xe5\xfd\x73\x04\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" \
+            "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" \
+            "\x00\x00\x00\x00\x07\x07\x44\x65\x66\x61\x75\x6c\x74\x06\x53\x74" \
+            "\x72\x69\x6e\x67\x06\x42\x69\x6e\x61\x72\x79\x07\x44\x65\x63\x69" \
+            "\x6d\x61\x6c\x03\x48\x65\x78\x0b\x45\x78\x70\x6f\x6e\x65\x6e\x74" \
+            "\x69\x61\x6c\x0b\x45\x6e\x67\x69\x6e\x65\x65\x72\x69\x6e\x67\x00" \
+            "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" \
+            "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xf8\x7f" \
+            "\x00\x00\x00\x00\x00\x00\xf8\x7f\x00\x00\x00\x00\x00\x00\xf8\x7f" \
+            "\x00\x00\x00\x00\x00\x00\xf8\x7f\x00\x00\x00\x00\x00\x00\x00\x00" \
+            "\x00\x00\x00\x00\x00\x00\x00\x00\x00"
+            ;
+
+    Value prototype;
+    TypeStore registry;
+    {
+        FixedBuf buf(false, payload_type);
+        from_wire_type(buf, registry, prototype);
+        testOk1(buf.good());
+        testEq(buf.size(), 0u);
+    }
+    Value payload(prototype.cloneEmpty());
+    {
+        FixedBuf buf(false, payload_value);
+        from_wire_valid(buf, registry, payload);
+        testOk1(buf.good());
+        testEq(buf.size(), 0u);
+    }
+    testShow()<<payload;
+    testEq(payload["value"].as<uint32_t>(), 0x700000f6u);
+    std::vector<uint8_t> actual_payload;
+    testToBytes(false, [&payload](Buffer& buf) {
+        to_wire_valid(buf, payload);
+        testOk1(buf.good());
+    }, reencoded_value);
+}
+
 // test the common case for a pvRequest of caching an empty Struct
 void testEmptyRequest()
 {
@@ -983,7 +1080,7 @@ void testEmptyRequest()
 
 MAIN(testxcode)
 {
-    testPlan(122);
+    testPlan(129);
     testSetup();
     testDeserializeString();
     testSerialize1();
@@ -996,6 +1093,7 @@ MAIN(testxcode)
     testArrayXCode();
     testXCodeNTScalar();
     testXCodeNTNDArray();
+    testRegressRedundantBitMask();
     testEmptyRequest();
     return testDone();
 }