Fix off-by-one error in upserts (#423)

* Fix off-by-one error in upserts

There are two off-by-one errors in the upsert functions for message ID and
message type. UpsertMessageId should allow a value of 65535, while
UpsertType should allow 255.

Without this fix, a device sending a confirmable message to the server will
receive an ACK message back with an incorrect Message ID.

Co-authored-by: Vit Prajzler <28387418+vitprajzler@users.noreply.github.com>

Author: jkralik

message/getmid.go

@@ -3,6 +3,7 @@ package message
 import (
 	"crypto/rand"
 	"encoding/binary"
+	"math"
 	mathRand "math/rand"
 	"sync/atomic"
 	"time"
@@ -14,7 +15,7 @@ func init() {
 
 var msgID = uint32(RandMID())
 
-// GetMID generates a message id for UDP-coap
+// GetMID generates a message id for UDP. (0 <= mid <= 65535)
 func GetMID() int32 {
 	return int32(uint16(atomic.AddUint32(&msgID, 1)))
 }
@@ -28,3 +29,8 @@ func RandMID() int32 {
 	}
 	return int32(uint16(binary.BigEndian.Uint32(b)))
 }
+
+// ValidateMID validates a message id for UDP. (0 <= mid <= 65535)
+func ValidateMID(mid int32) bool {
+	return mid >= 0 && mid <= math.MaxUint16
+}

message/message.go

@@ -2,7 +2,6 @@ package message
 
 import (
 	"fmt"
-	"math"
 
 	"github.com/plgd-dev/go-coap/v3/message/codes"
 )
@@ -38,10 +37,10 @@ func (r *Message) String() string {
 	if err == nil {
 		buf = fmt.Sprintf("%s, Queries: %+v", buf, queries)
 	}
-	if r.Type >= 0 && r.Type < math.MaxUint8 {
+	if ValidateType(r.Type) {
 		buf = fmt.Sprintf("%s, Type: %v", buf, r.Type)
 	}
-	if r.MessageID >= 0 && r.MessageID < math.MaxUint16 {
+	if ValidateMID(r.MessageID) {
 		buf = fmt.Sprintf("%s, MessageID: %v", buf, r.MessageID)
 	}
 	if len(r.Payload) > 0 {

message/pool/message.go

@@ -6,7 +6,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"math"
 
 	"github.com/plgd-dev/go-coap/v3/message"
 	"github.com/plgd-dev/go-coap/v3/message/codes"
@@ -81,7 +80,7 @@ func (r *Message) SetMessageID(mid int32) {
 
 // UpsertMessageID set value only when origin value is invalid. Only 0 to 2^16-1 values are valid.
 func (r *Message) UpsertMessageID(mid int32) {
-	if r.msg.MessageID >= 0 && r.msg.MessageID < math.MaxUint16 {
+	if message.ValidateMID(r.msg.MessageID) {
 		return
 	}
 	r.SetMessageID(mid)
@@ -99,7 +98,7 @@ func (r *Message) SetType(typ message.Type) {
 
 // UpsertType set value only when origin value is invalid. Only 0 to 2^8-1 values are valid.
 func (r *Message) UpsertType(typ message.Type) {
-	if r.msg.Type >= 0 && r.msg.Type < math.MaxUint8 {
+	if message.ValidateType(r.msg.Type) {
 		return
 	}
 	r.SetType(typ)

message/type.go

@@ -1,6 +1,7 @@
 package message
 
 import (
+	"math"
 	"strconv"
 )
 
@@ -37,3 +38,8 @@ func (t Type) String() string {
 	}
 	return "Type(" + strconv.FormatInt(int64(t), 10) + ")"
 }
+
+// ValidateType validates the type for UDP. (0 <= typ <= 255)
+func ValidateType(typ Type) bool {
+	return typ >= 0 && typ <= math.MaxUint8
+}

udp/coder/coder.go

@@ -4,7 +4,6 @@ import (
 	"encoding/binary"
 	"errors"
 	"fmt"
-	"math"
 
 	"github.com/plgd-dev/go-coap/v3/message"
 	"github.com/plgd-dev/go-coap/v3/message/codes"
@@ -46,10 +45,10 @@ func (c *Coder) Encode(m message.Message, buf []byte) (int, error) {
 	   |1 1 1 1 1 1 1 1|    Payload (if any) ...
 	   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 	*/
-	if m.MessageID < 0 || m.MessageID > math.MaxUint16 {
+	if !message.ValidateMID(m.MessageID) {
 		return -1, fmt.Errorf("invalid MessageID(%v)", m.MessageID)
 	}
-	if m.Type < 0 || m.Type > math.MaxUint8 {
+	if !message.ValidateType(m.Type) {
 		return -1, fmt.Errorf("invalid Type(%v)", m.Type)
 	}
 	size, err := c.Size(m)

udp/coder/coder_test.go

@@ -1,6 +1,7 @@
 package coder
 
 import (
+	"math"
 	"testing"
 
 	"github.com/plgd-dev/go-coap/v3/message"
@@ -24,6 +25,27 @@ func testUnmarshalMessage(t *testing.T, msg message.Message, buf []byte, expecte
 
 func TestMarshalMessage(t *testing.T) {
 	buf := make([]byte, 1024)
+
+	// validate messageID
+	_, err := DefaultCoder.Encode(message.Message{MessageID: 0}, buf)
+	require.NoError(t, err)
+	_, err = DefaultCoder.Encode(message.Message{MessageID: -1}, buf)
+	require.Error(t, err)
+	_, err = DefaultCoder.Encode(message.Message{MessageID: math.MaxUint16}, buf)
+	require.NoError(t, err)
+	_, err = DefaultCoder.Encode(message.Message{MessageID: math.MaxUint16 + 1}, buf)
+	require.Error(t, err)
+
+	// validate type
+	_, err = DefaultCoder.Encode(message.Message{Type: 0}, buf)
+	require.NoError(t, err)
+	_, err = DefaultCoder.Encode(message.Message{Type: -1}, buf)
+	require.Error(t, err)
+	_, err = DefaultCoder.Encode(message.Message{Type: math.MaxUint8}, buf)
+	require.NoError(t, err)
+	_, err = DefaultCoder.Encode(message.Message{Type: math.MaxUint8 + 1}, buf)
+	require.Error(t, err)
+
 	testMarshalMessage(t, message.Message{}, buf, []byte{64, 0, 0, 0})
 	testMarshalMessage(t, message.Message{Code: codes.GET}, buf, []byte{64, byte(codes.GET), 0, 0})
 	testMarshalMessage(t, message.Message{Code: codes.GET, Payload: []byte{0x1}}, buf, []byte{64, byte(codes.GET), 0, 0, 0xff, 0x1})
@@ -57,7 +79,7 @@ func TestMarshalMessage(t *testing.T) {
 	bufOptionsUsed := bufOptions
 	options := make(message.Options, 0, 32)
 	enc := 0
-	options, enc, err := options.SetPath(bufOptionsUsed, "/a/b/c/d/e")
+	options, enc, err = options.SetPath(bufOptionsUsed, "/a/b/c/d/e")
 	if err != nil {
 		t.Fatalf("Cannot set uri")
 	}