From 6b428371e6d0f6caf366aea8ecf3312c17d6bf70 Mon Sep 17 00:00:00 2001
From: Matthias Schiffer <matthias.schiffer@tq-group.com>
Date: Mon, 20 Jan 2020 10:27:54 +0100
Subject: [PATCH] Fix handling of map entries with omitted key or value

According to [1], map encoding must be compatible with a repeated message
using indices 1 and 2 for key and value. In particular this implies that
both key and value may be omitted when they are equal to the default
value - which some protobuf implementations like protobuf-c actually do.

The comments in the added testcase are based on the output of
protobuf-inspector [2].

[1] https://developers.google.com/protocol-buffers/docs/proto3#backwards-compatibility
[2] https://github.com/jmendeth/protobuf-inspector

Based-on-patch-by: Shrimpz <Shrimpz@qq.com>
---
 src/decoder.js     | 52 +++++++++++++++++++++++++++++++++-------------
 tests/comp_maps.js | 44 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+), 15 deletions(-)

diff --git a/src/decoder.js b/src/decoder.js
index 6dde6f82e..491dd3059 100644
--- a/src/decoder.js
+++ b/src/decoder.js
@@ -19,7 +19,7 @@ function decoder(mtype) {
     var gen = util.codegen(["r", "l"], mtype.name + "$decode")
     ("if(!(r instanceof Reader))")
         ("r=Reader.create(r)")
-    ("var c=l===undefined?r.len:r.pos+l,m=new this.ctor" + (mtype.fieldsArray.filter(function(field) { return field.map; }).length ? ",k" : ""))
+    ("var c=l===undefined?r.len:r.pos+l,m=new this.ctor" + (mtype.fieldsArray.filter(function(field) { return field.map; }).length ? ",k,value" : ""))
     ("while(r.pos<c){")
         ("var t=r.uint32()");
     if (mtype.group) gen
@@ -37,22 +37,44 @@ function decoder(mtype) {
 
         // Map fields
         if (field.map) { gen
-                ("r.skip().pos++") // assumes id 1 + key wireType
                 ("if(%s===util.emptyObject)", ref)
                     ("%s={}", ref)
-                ("k=r.%s()", field.keyType)
-                ("r.pos++"); // assumes id 2 + value wireType
-            if (types.long[field.keyType] !== undefined) {
-                if (types.basic[type] === undefined) gen
-                ("%s[typeof k===\"object\"?util.longToHash(k):k]=types[%i].decode(r,r.uint32())", ref, i); // can't be groups
-                else gen
-                ("%s[typeof k===\"object\"?util.longToHash(k):k]=r.%s()", ref, type);
-            } else {
-                if (types.basic[type] === undefined) gen
-                ("%s[k]=types[%i].decode(r,r.uint32())", ref, i); // can't be groups
-                else gen
-                ("%s[k]=r.%s()", ref, type);
-            }
+                ("var c2 = r.uint32()+r.pos");
+
+            if (types.defaults[field.keyType] !== undefined) gen
+                ("k=%j", types.defaults[field.keyType]);
+            else gen
+                ("k=null");
+
+            if (types.defaults[type] !== undefined) gen
+                ("value=%j", types.defaults[type]);
+            else gen
+                ("value=null");
+
+            gen
+                ("while(r.pos<c2){")
+                    ("var tag2=r.uint32()")
+                    ("switch(tag2>>>3){")
+                        ("case 1: k=r.%s(); break", field.keyType)
+                        ("case 2:");
+
+            if (types.basic[type] === undefined) gen
+                            ("value=types[%i].decode(r,r.uint32())", i); // can't be groups
+            else gen
+                            ("value=r.%s()", type);
+
+            gen
+                            ("break")
+                        ("default:")
+                            ("r.skipType(tag2&7)")
+                            ("break")
+                    ("}")
+                ("}");
+
+            if (types.long[field.keyType] !== undefined) gen
+                ("%s[typeof k===\"object\"?util.longToHash(k):k]=value", ref);
+            else gen
+                ("%s[k]=value", ref);
 
         // Repeated fields
         } else if (field.repeated) { gen
diff --git a/tests/comp_maps.js b/tests/comp_maps.js
index 7c5768a17..37051f6b0 100644
--- a/tests/comp_maps.js
+++ b/tests/comp_maps.js
@@ -92,6 +92,50 @@ tape.test("maps", function(test) {
         test.end();
     });
 
+    test.test(test.name + " - omitted fields", function(test) {
+
+        var mapRoot = protobuf.Root.fromJSON({
+            nested: {
+                MapMessage: {
+                    fields: {
+                        value: {
+                            keyType: "int32",
+                            type: "string",
+                            id: 1
+                        }
+                    }
+                }
+            }
+        });
+
+        var MapMessage = mapRoot.lookup("MapMessage");
+
+        var value = {
+            value: {
+                0: ''
+            }
+        };
+        var dec;
+
+        // 1 <chunk> = message(1 <varint> = 0, 2 <chunk> = empty chunk)
+        dec = MapMessage.decode(Uint8Array.of(0x0a, 0x04, 0x08, 0x00, 0x12, 0x00));
+        test.deepEqual(dec, value, "should correct decode the buffer without omitted fields");
+
+        // 1 <chunk> = message(1 <varint> = 0)
+        dec = MapMessage.decode(Uint8Array.of(0x0a, 0x02, 0x08, 0x00));
+        test.deepEqual(dec, value, "should correct decode the buffer with omitted value");
+
+        // 1 <chunk> = message(2 <chunk> = empty chunk)
+        dec = MapMessage.decode(Uint8Array.of(0x0a, 0x02, 0x12, 0x00));
+        test.deepEqual(dec, value, "should correct decode the buffer with omitted key");
+
+        // 1 <chunk> = empty chunk
+        dec = MapMessage.decode(Uint8Array.of(0x0a, 0x00));
+        test.deepEqual(dec, value, "should correct decode the buffer with both key and value omitted");
+
+        test.end();
+    });
+
     test.end();
 });