OnServerDisconnect is not called when a DISCONNECT packet has a reason code but no properties

[nickajacks1]

Describe the bug
A clear and concise description of what the bug is.

To reproduce

package main

import (
	"context"
	"fmt"
	"log"
	"net/url"

	"github.com/eclipse/paho.golang/autopaho"
	"github.com/eclipse/paho.golang/paho"
)

func main() {
	broker, _ := url.Parse("mqtt://127.0.0.1:1883")
	cm, err := autopaho.NewConnection(context.Background(), autopaho.ClientConfig{
		ServerUrls: []*url.URL{broker},
		ClientConfig: paho.ClientConfig{
			ClientID: "client",
			OnServerDisconnect: func(*paho.Disconnect) {
				fmt.Println("OnServerDisconnect called")
			},
		},
		ConnectPassword: []byte("foo"),
		ConnectUsername: "foo",
		PahoDebug:       log.Default(),
		PahoErrors:      log.Default(),
	})
	if err != nil {
		panic(err)
	}
	cm.AwaitConnection(context.Background())

	select {}
}

Run the above client. In another window, run mosquitto_pub -V mqttv5 -t foo -m bar -i client.
This should cause mosquitto to send a DISCONNECT packet with the bytes e0 1 8e where 1 is the message length and 8e is reason code 142, "Session taken over". The OnServerDisconnect callback does not get called.

Expected behaviour
The OnServerDisconnect callback is called when the server sends a well-formed DISCONNECT packet.

Software used:

• Mosquitto 2.0.21.
• paho.golang v0.22.0

Additional context
The MQTT v5 Spec is a little bit vague about whether the property length must be included in a DISCONNECT packet. EMQX, for example, does include the length even when there are zero properties. However, my interpretation is that it is valid to omit it. In section 3.14.2.2.1, it states

If the Remaining Length is less than 2, a value of 0 is used.

In section 3.14.2.1, it uses very similar language to describe the default reason code:

If the Remaining Length is less than 1 the value of 0x00 (Normal disconnection) is used.

...and here, I know for a fact that it's valid to omit the reason code if it's 0x00 (and there are no properties). So, I extrapolate that when 3.14.2.2.1 states that a value of 0 is used, it means from the perspective of the one decoding the packet. All this to say, I think this is a bug in paho.golang and not mosquitto.

[nickajacks1]

The issue is in Disconnect.Unpack. It always unpacks the properties without checking whether there are properties, giving an error. Other packet types, like Auth, skip unpacking properties if they are absent.
I have a change sitting ready for a PR, but I still have to wait for my employer to give me the green light to sign the eca.

[MattBrittan]

Looks sensible to me, will await yoru PR (with ECA). Just let me know if this is not possible (the fix should be fairly simple).

[MattBrittan]

Thanks, I'll accept the PR. However I wonder if this needs more work (as by your reading of the spec 3.4.2.2.1 indicates the same treatment is needed for PUBACK). I agree that the spec seems to contradict itself ("If there are no properties, this MUST be indicated by including a Property Length of zero", "If the Remaining Length is less than 2, a value of 0 is used") so will be interesting to see if there are issues with any other packets.