Incorrect decoding of packets including Properties exceeding 127 bytes in length

[ghost]

Observation: The payload of inbound PUBLISH packets with Properties exceeding 127 bytes in length is not decoded correctly. The decoding logic does not consider any extra bytes in the Properties Variable Byte Integer header when decoding subsequent data.

The issue has been discovered using Mochi MQTT v2.1.6 and is also present in later (and earlier) v2 versions.

To reproduce: Publish the following PUBLISH packet on an MQTT v5 client connected to Mochi MQTT using protocol version 5:

// Using Eclipse Paho MQTT Go client v0.10.0 (https://github.com/eclipse/paho.golang).

props := &paho.PublishProperties{
    PayloadFormat: paho.Byte(0),
    ResponseTopic: "dda/mqtt5_noauth/com/acr/oneof/ba96c8ce-2bb0-442a-a63d-4d9d65400fa1",
    CorrelationData: []byte{48 57 102 50 50 54 48 100 45 51 51 57 50 45 52 53 100 55 45 57 98 101 100 45 56 55 101 97 52 97 52 102 56 100 101 55},
    User: paho.UserProperties{
      {Key: "id", Value: "42"},
      {Key: "sr", Value: "pub"},
    },
}

packet := &paho.Publish{
    QoS:        0,
    Retain:     false,
    Topic:      "dda/mqtt5_noauth/com/act/oneof",
    Payload:    []byte{1 2 3 4 5},
    Properties: props,
}

Enabling Mochi debug hook, the PUBLISH packet received and decoded by Mochi looks like this:

DBG PUBLISH << pubba96c8ce02bb00442a0a hook=debug 
m={"id":0,"packet":{"Connect":{"clean":false,"clientId":"","keepalive":0,"password":null,"passwordFlag":false,"protocolName":nul
l,"username":null,"usernameFlag":false,"willFlag":false,"willPayload":null,"willProperties":{"aci":"","ad":null,"am":"","cd":nul
l,"ct":"","fmqos":false,"fpf":false,"fra":false,"frpi":false,"fsei":false,"fsida":false,"fska":false,"fssa":false,"fta":false,"f
wsa":false,"me":0,"mps":0,"mqos":0,"pf":0,"ra":0,"ri":"","rm":0,"rpi":0,"rri":0,"rs":"","rt":"","sei":0,"si":null,"sida":0,"ska"
:0,"sr":"","ssa":0,"ta":0,"tam":0,"user":null,"wdi":0,"wsa":0},"willQos":0,"willRetain":false,"willTopic":""},"Created":0,"Expir
y":0,"Filters":null,"FixedHeader":{"dup":false,"qos":0,"remaining":167,"retain":false,"type":3},"Mods":{"AllowResponseInfo":fals
e,"DisallowProblemInfo":false,"MaxSize":0},"Origin":"","PacketID":0,"Payload":"YgECAwQF","Properties":{"aci":"","ad":null,"am":"
","cd":"MDlmMjI2MGQtMzM5Mi00NWQ3LTliZWQtODdlYTRhNGY4ZGU3","ct":"","fmqos":false,"fpf":false,"fra":false,"frpi":false,"fsei":fals
e,"fsida":false,"fska":false,"fssa":false,"fta":false,"fwsa":false,"me":0,"mps":0,"mqos":0,"pf":0,"ra":0,"ri":"","rm":0,"rpi":0,
"rri":0,"rs":"","rt":"dda/mqtt5_noauth/com/acr/oneof/ba96c8ce-2bb0-442a-a63d-4d9d65400fa1","sei":0,"si":null,"sida":0,"ska":0,"s
r":"","ssa":0,"ta":0,"tam":0,"user":[{"k":"id","v":"42"},{"k":"sr","v":"pub"}]
,"wdi":0,"wsa":0},"ProtocolVersion":5,"ReasonCode":0,"ReasonCodes":null,"ReservedBit":0,"SessionPresent":false,"TopicName":"dda/
mqtt5_noauth/com/act/oneof"},"payload":"b\u0001\u0002\u0003\u0004\u0005","qos":0,"raw":"YgECAwQF","topic":"dda/mqtt5_noauth/com/
act/oneof"}

The corresponding PUBLISH packet bytes look like this:

[
    // Fixed Header of length 3, Remaining bytes 167
    48 167 1

    // Topic of length 30 with 2 byte VBI
    0 30 100 100 97 47 109 113 116 116 53 95 110 111 97 117 116 104 47 99 111 109 47 97 99 116 47 111 110 101 111 102

    // Properties of length 128 with 2 byte VBI
    128 1 8 0 67 100 100 97 47 109 113 116 116 53 95 110 111 97 117 116 104 47 99 111 109 47 97 99 114 47 111 110 101
    111 102 47 98 97 57 54 99 56 99 101 45 50 98 98 48 45 52 52 50 97 45 97 54 51 100 45 52 100 57 100 54 53 52 48 48
    102 97 49 9 0 36 48 57 102 50 50 54 48 100 45 51 51 57 50 45 52 53 100 55 45 57 98 101 100 45 56 55 101 97 52 97
    52 102 56 100 101 55 38 0 2 105 100 0 2 52 50 38 0 2 115 114 0 3 112 117 98

    // Payload
    1 2 3 4 5
]

After decoding, the payload bytes which are sent to the subscribers look like this (see debug hook output above):

[98 1 2 3 4 5]

The decoded payload includes an extra leading byte 98 ("b"), which is the last byte of the encoded Properties bytes, i.e. the last character of the value "pub" belonging to User Property key "sr".

This issue happens whenever Properties of an encoded CONNECT, CONNACK, PUBLISH, SUBACK, SUBSCRIBE, UNSUBACK, UNSUBSCRIBE packet require a VBI which consists of more than 1 byte. Subsequent data "overlaps" with the preceding Properties, including up to 3 of the Properties final bytes at the beginning. The reason is that Mochi decoding function (packets.Properties).Decode does not take into account any extra bytes occupied by the Properties VBI header for decoding subsequent data.

To fix this issue, I have prepared a PR.

Thanks for providing a patch release!

[mochi-co]

Fixed in v2.2.2, thank you very much for your work on this @ffa500! This is wonderful. Let me know if it is working now 👍🏻

[mochi-co]

I'm going to close this for now, let me know if you experience any more issues :) Thanks again

[ghost]

Works perfectly now! Thanks so much for providing a new release with the patch so quickly! Mochi is awesome!