Merge pull request #152 from arduino-libraries/fix-cbor-msg-exceeds-buffer

Fix: Handle encoded CBOR message exceeding buffer size

Author: aentinger

extras/test/src/test_encode.cpp

@@ -20,7 +20,7 @@
    TEST CODE
  **************************************************************************************/
 
-SCENARIO("Arduino Cloud Properties are encoded", "[ArduinoCloudThing::encode]") {
+SCENARIO("Arduino Cloud Properties are encoded", "[ArduinoCloudThing::encode-1]") {
   /************************************************************************************/
 
   WHEN("A 'bool' property is added") {
@@ -381,4 +381,59 @@ SCENARIO("Arduino Cloud Properties are encoded", "[ArduinoCloudThing::encode]")
   }
 
   /************************************************************************************/
+
+  WHEN("The size of the encoded properties is exceeding the CBOR buffer size") {
+    GIVEN("CloudProtocol::V2") {
+      PropertyContainer property_container;
+
+      CloudString str_0; str_0 = "This string is 30 bytes long.";
+      CloudString str_1; str_1 = "This string is 30 bytes long.";
+      CloudString str_2; str_2 = "This string is 30 bytes long.";
+      CloudString str_3; str_3 = "This string is 30 bytes long.";
+      CloudString str_4; str_4 = "This string is 30 bytes long.";
+      CloudString str_5; str_5 = "This string is 30 bytes long.";
+      CloudString str_6; str_6 = "This string is 30 bytes long.";
+      CloudString str_7; str_7 = "This string is 30 bytes long.";
+      CloudString str_8; str_8 = "This string is 30 bytes long.";
+      CloudString str_9; str_9 = "This string is 30 bytes long.";
+
+      addPropertyToContainer(property_container, str_0, "str_0", Permission::ReadWrite);
+      addPropertyToContainer(property_container, str_1, "str_1", Permission::ReadWrite);
+      addPropertyToContainer(property_container, str_2, "str_2", Permission::ReadWrite);
+      addPropertyToContainer(property_container, str_3, "str_3", Permission::ReadWrite);
+      addPropertyToContainer(property_container, str_4, "str_4", Permission::ReadWrite);
+      addPropertyToContainer(property_container, str_5, "str_5", Permission::ReadWrite);
+      addPropertyToContainer(property_container, str_6, "str_6", Permission::ReadWrite);
+      addPropertyToContainer(property_container, str_7, "str_7", Permission::ReadWrite);
+      addPropertyToContainer(property_container, str_8, "str_8", Permission::ReadWrite);
+      addPropertyToContainer(property_container, str_9, "str_9", Permission::ReadWrite);
+
+      /* Due to the size if the encoded properties exceeding 256 bytes if encoded all at
+       * once they are encoded in subsequent calls to CBOREncoder::encode.
+       */
+
+      /* [{0: "str_0", 3: "This string is 30 bytes long."}, {0: "str_1", 3: "This string is 30 bytes long."}, {0: "str_2", 3: "This string is 30 bytes long."}, {0: "str_3", 3: "This string is 30 bytes long."}]
+       * = 9F A2 00 65 73 74 72 5F 30 03 78 1D 54 68 69 73 20 73 74 72 69 6E 67 20 69 73 20 33 30 20 62 79 74 65 73 20 6C 6F 6E 67 2E A2 00 65 73 74 72 5F 31 03 78 1D 54 68 69 73 20 73 74 72 69 6E 67 20 69 73 20 33 30 20 62 79 74 65 73 20 6C 6F 6E 67 2E A2 00 65 73 74 72 5F 32 03 78 1D 54 68 69 73 20 73 74 72 69 6E 67 20 69 73 20 33 30 20 62 79 74 65 73 20 6C 6F 6E 67 2E A2 00 65 73 74 72 5F 33 03 78 1D 54 68 69 73 20 73 74 72 69 6E 67 20 69 73 20 33 30 20 62 79 74 65 73 20 6C 6F 6E 67 2E FF
+       */
+      std::vector<uint8_t> const expected_1 = {0x9F, 0xA2, 0x00, 0x65, 0x73, 0x74, 0x72, 0x5F, 0x30, 0x03, 0x78, 0x1D, 0x54, 0x68, 0x69, 0x73, 0x20, 0x73, 0x74, 0x72, 0x69, 0x6E, 0x67, 0x20, 0x69, 0x73, 0x20, 0x33, 0x30, 0x20, 0x62, 0x79, 0x74, 0x65, 0x73, 0x20, 0x6C, 0x6F, 0x6E, 0x67, 0x2E, 0xA2, 0x00, 0x65, 0x73, 0x74, 0x72, 0x5F, 0x31, 0x03, 0x78, 0x1D, 0x54, 0x68, 0x69, 0x73, 0x20, 0x73, 0x74, 0x72, 0x69, 0x6E, 0x67, 0x20, 0x69, 0x73, 0x20, 0x33, 0x30, 0x20, 0x62, 0x79, 0x74, 0x65, 0x73, 0x20, 0x6C, 0x6F, 0x6E, 0x67, 0x2E, 0xA2, 0x00, 0x65, 0x73, 0x74, 0x72, 0x5F, 0x32, 0x03, 0x78, 0x1D, 0x54, 0x68, 0x69, 0x73, 0x20, 0x73, 0x74, 0x72, 0x69, 0x6E, 0x67, 0x20, 0x69, 0x73, 0x20, 0x33, 0x30, 0x20, 0x62, 0x79, 0x74, 0x65, 0x73, 0x20, 0x6C, 0x6F, 0x6E, 0x67, 0x2E, 0xA2, 0x00, 0x65, 0x73, 0x74, 0x72, 0x5F, 0x33, 0x03, 0x78, 0x1D, 0x54, 0x68, 0x69, 0x73, 0x20, 0x73, 0x74, 0x72, 0x69, 0x6E, 0x67, 0x20, 0x69, 0x73, 0x20, 0x33, 0x30, 0x20, 0x62, 0x79, 0x74, 0x65, 0x73, 0x20, 0x6C, 0x6F, 0x6E, 0x67, 0x2E, 0xFF};
+      std::vector<uint8_t> const actual_1 = cbor::encode(property_container);
+      REQUIRE(actual_1 == expected_1);
+
+      /* [{0: "str_4", 3: "This string is 30 bytes long."}, {0: "str_5", 3: "This string is 30 bytes long."}, {0: "str_6", 3: "This string is 30 bytes long."}, {0: "str_7", 3: "This string is 30 bytes long."}]
+       * = 9F A2 00 65 73 74 72 5F 34 03 78 1D 54 68 69 73 20 73 74 72 69 6E 67 20 69 73 20 33 30 20 62 79 74 65 73 20 6C 6F 6E 67 2E A2 00 65 73 74 72 5F 35 03 78 1D 54 68 69 73 20 73 74 72 69 6E 67 20 69 73 20 33 30 20 62 79 74 65 73 20 6C 6F 6E 67 2E A2 00 65 73 74 72 5F 36 03 78 1D 54 68 69 73 20 73 74 72 69 6E 67 20 69 73 20 33 30 20 62 79 74 65 73 20 6C 6F 6E 67 2E A2 00 65 73 74 72 5F 37 03 78 1D 54 68 69 73 20 73 74 72 69 6E 67 20 69 73 20 33 30 20 62 79 74 65 73 20 6C 6F 6E 67 2E FF
+       */
+      std::vector<uint8_t> const expected_2 = {0x9F, 0xA2, 0x00, 0x65, 0x73, 0x74, 0x72, 0x5F, 0x34, 0x03, 0x78, 0x1D, 0x54, 0x68, 0x69, 0x73, 0x20, 0x73, 0x74, 0x72, 0x69, 0x6E, 0x67, 0x20, 0x69, 0x73, 0x20, 0x33, 0x30, 0x20, 0x62, 0x79, 0x74, 0x65, 0x73, 0x20, 0x6C, 0x6F, 0x6E, 0x67, 0x2E, 0xA2, 0x00, 0x65, 0x73, 0x74, 0x72, 0x5F, 0x35, 0x03, 0x78, 0x1D, 0x54, 0x68, 0x69, 0x73, 0x20, 0x73, 0x74, 0x72, 0x69, 0x6E, 0x67, 0x20, 0x69, 0x73, 0x20, 0x33, 0x30, 0x20, 0x62, 0x79, 0x74, 0x65, 0x73, 0x20, 0x6C, 0x6F, 0x6E, 0x67, 0x2E, 0xA2, 0x00, 0x65, 0x73, 0x74, 0x72, 0x5F, 0x36, 0x03, 0x78, 0x1D, 0x54, 0x68, 0x69, 0x73, 0x20, 0x73, 0x74, 0x72, 0x69, 0x6E, 0x67, 0x20, 0x69, 0x73, 0x20, 0x33, 0x30, 0x20, 0x62, 0x79, 0x74, 0x65, 0x73, 0x20, 0x6C, 0x6F, 0x6E, 0x67, 0x2E, 0xA2, 0x00, 0x65, 0x73, 0x74, 0x72, 0x5F, 0x37, 0x03, 0x78, 0x1D, 0x54, 0x68, 0x69, 0x73, 0x20, 0x73, 0x74, 0x72, 0x69, 0x6E, 0x67, 0x20, 0x69, 0x73, 0x20, 0x33, 0x30, 0x20, 0x62, 0x79, 0x74, 0x65, 0x73, 0x20, 0x6C, 0x6F, 0x6E, 0x67, 0x2E, 0xFF};
+      std::vector<uint8_t> const actual_2 = cbor::encode(property_container);
+      REQUIRE(actual_2 == expected_2);
+
+      /* [{0: "str_8", 3: "This string is 30 bytes long."}, {0: "str_9", 3: "This string is 30 bytes long."}]
+       * = 9F A2 00 65 73 74 72 5F 38 03 78 1D 54 68 69 73 20 73 74 72 69 6E 67 20 69 73 20 33 30 20 62 79 74 65 73 20 6C 6F 6E 67 2E A2 00 65 73 74 72 5F 39 03 78 1D 54 68 69 73 20 73 74 72 69 6E 67 20 69 73 20 33 30 20 62 79 74 65 73 20 6C 6F 6E 67 2E FF
+       */
+      std::vector<uint8_t> const expected_3 = {0x9F, 0xA2, 0x00, 0x65, 0x73, 0x74, 0x72, 0x5F, 0x38, 0x03, 0x78, 0x1D, 0x54, 0x68, 0x69, 0x73, 0x20, 0x73, 0x74, 0x72, 0x69, 0x6E, 0x67, 0x20, 0x69, 0x73, 0x20, 0x33, 0x30, 0x20, 0x62, 0x79, 0x74, 0x65, 0x73, 0x20, 0x6C, 0x6F, 0x6E, 0x67, 0x2E, 0xA2, 0x00, 0x65, 0x73, 0x74, 0x72, 0x5F, 0x39, 0x03, 0x78, 0x1D, 0x54, 0x68, 0x69, 0x73, 0x20, 0x73, 0x74, 0x72, 0x69, 0x6E, 0x67, 0x20, 0x69, 0x73, 0x20, 0x33, 0x30, 0x20, 0x62, 0x79, 0x74, 0x65, 0x73, 0x20, 0x6C, 0x6F, 0x6E, 0x67, 0x2E, 0xFF};
+      std::vector<uint8_t> const actual_3 = cbor::encode(property_container);
+      REQUIRE(actual_3 == expected_3);
+    }
+  }
+
+  /************************************************************************************/
 }

extras/test/src/util/CBORTestUtil.cpp

@@ -25,13 +25,13 @@ namespace cbor
  **************************************************************************************/
 
 std::vector<uint8_t> encode(PropertyContainer & property_container, bool lightPayload) {
+  int bytes_encoded = 0;
   uint8_t buf[200] = {0};
-  int const bytes_buf = CBOREncoder::encode(property_container, buf, 200, lightPayload);
-  if (bytes_buf == -1) {
+
+  if (CBOREncoder::encode(property_container, buf, 200, bytes_encoded, lightPayload) == CborNoError)
+    return std::vector<uint8_t>(buf, buf + bytes_encoded);
+  else
     return std::vector<uint8_t>();
-  } else {
-    return std::vector<uint8_t>(buf, buf + bytes_buf);
-  }
 }
 
 void print(std::vector<uint8_t> const & vect) {

src/ArduinoIoTCloudLPWAN.cpp

@@ -126,11 +126,12 @@ void ArduinoIoTCloudLPWAN::disconnect()
 
 void ArduinoIoTCloudLPWAN::sendPropertiesToCloud()
 {
+  int bytes_encoded = 0;
   uint8_t data[CBOR_LORA_MSG_MAX_SIZE];
-  int const length = CBOREncoder::encode(_property_container, data, sizeof(data), true);
-  if (length > 0) {
-    writeProperties(data, length);
-  }
+
+  if (CBOREncoder::encode(_property_container, data, sizeof(data), bytes_encoded, true) == CborNoError)
+    if (bytes_encoded > 0)
+      writeProperties(data, bytes_encoded);
 }
 
 int ArduinoIoTCloudLPWAN::writeProperties(const byte data[], int length)

src/ArduinoIoTCloudTCP.cpp

@@ -283,18 +283,20 @@ void ArduinoIoTCloudTCP::handleMessage(int length)
 
 void ArduinoIoTCloudTCP::sendPropertiesToCloud()
 {
+  int bytes_encoded = 0;
   uint8_t data[MQTT_TRANSMIT_BUFFER_SIZE];
-  int const length = CBOREncoder::encode(_property_container, data, sizeof(data));
-  if (length > 0)
-  {
-    /* If properties have been encoded store them in the back-up buffer
-     * in order to allow retransmission in case of failure.
-     */
-    _mqtt_data_len = length;
-    memcpy(_mqtt_data_buf, data, _mqtt_data_len);
-    /* Transmit the properties to the MQTT broker */
-    write(_dataTopicOut, _mqtt_data_buf, _mqtt_data_len);
-  }
+
+  if (CBOREncoder::encode(_property_container, data, sizeof(data), bytes_encoded, false) == CborNoError)
+    if (bytes_encoded > 0)
+    {
+      /* If properties have been encoded store them in the back-up buffer
+       * in order to allow retransmission in case of failure.
+       */
+      _mqtt_data_len = bytes_encoded;
+      memcpy(_mqtt_data_buf, data, _mqtt_data_len);
+      /* Transmit the properties to the MQTT broker */
+      write(_dataTopicOut, _mqtt_data_buf, _mqtt_data_len);
+    }
 }
 
 void ArduinoIoTCloudTCP::requestLastValue()

src/cbor/CBOREncoder.cpp

@@ -21,31 +21,53 @@
 
 #include "CBOREncoder.h"
 
+#undef max
+#undef min
+#include <algorithm>
+
 #include "lib/tinycbor/cbor-lib.h"
 
 /******************************************************************************
  * PUBLIC MEMBER FUNCTIONS
  ******************************************************************************/
 
-int CBOREncoder::encode(PropertyContainer & property_container, uint8_t * data, size_t const size, bool lightPayload)
+CborError CBOREncoder::encode(PropertyContainer & property_container, uint8_t * data, size_t const size, int & bytes_encoded, bool lightPayload)
 {
   CborEncoder encoder, arrayEncoder;
 
   cbor_encoder_init(&encoder, data, size, 0);
 
-  if (cbor_encoder_create_array(&encoder, &arrayEncoder, CborIndefiniteLength) != CborNoError)
-    return -1;
+  CHECK_CBOR(cbor_encoder_create_array(&encoder, &arrayEncoder, CborIndefiniteLength));
 
   /* Check if backing storage and cloud has diverged
    * time interval may be elapsed or property may be changed
    * and if that's the case encode the property into the CBOR.
    */
-  if (appendChangedProperties(property_container, &arrayEncoder, lightPayload) < 1)
-    return -1;
+  CborError error = CborNoError;
+  int num_encoded_properties = 0;
+  std::for_each(property_container.begin(),
+                property_container.end(),
+                [lightPayload, &arrayEncoder, &error, &num_encoded_properties](Property * p)
+                {
+                  if (p->shouldBeUpdated() && p->isReadableByCloud())
+                  {
+                    error = p->append(&arrayEncoder, lightPayload);
+                    if(error == CborNoError)
+                      num_encoded_properties++;
+                    else
+                      return;
+                  }
+                });
+  if ((CborNoError != error) && 
+      (CborErrorOutOfMemory != error))
+    return error;
+
+  CHECK_CBOR(cbor_encoder_close_container(&encoder, &arrayEncoder));
 
-  if (cbor_encoder_close_container(&encoder, &arrayEncoder) != CborNoError)
-    return -1;
+  if (num_encoded_properties > 0)
+    bytes_encoded = cbor_encoder_get_buffer_size(&encoder, data);
+  else
+    bytes_encoded = 0;
 
-  int const bytes_encoded = cbor_encoder_get_buffer_size(&encoder, data);
-  return bytes_encoded;
+  return CborNoError;
 }

src/cbor/CBOREncoder.h

@@ -35,7 +35,7 @@ class CBOREncoder
 
     /* encode return > 0 if a property has changed and encodes the changed properties in CBOR format into the provided buffer */
     /* if lightPayload is true the integer identifier of the property will be encoded in the message instead of the property name in order to reduce the size of the message payload*/
-    static int encode(PropertyContainer & property_container, uint8_t * data, size_t const size, bool lightPayload = false);
+    static CborError encode(PropertyContainer & property_container, uint8_t * data, size_t const size, int & bytes_encoded, bool lightPayload = false);
 
 private:
 

src/cbor/lib/tinycbor/cbor-lib.h

@@ -1,6 +1,24 @@
 #ifndef CBOR_LIB_H
 #define CBOR_LIB_H
 
+/******************************************************************************
+   INCLUDE
+ ******************************************************************************/
+
 #include "src/cbor.h"
 
+/******************************************************************************
+ * DEFINE
+ ******************************************************************************/
+
+#ifndef CHECK_CBOR
+  #define CHECK_CBOR(expr)           \
+    do {                             \
+      CborError error = CborNoError; \
+      error = (expr);                \
+      if (CborNoError != error)      \
+        return error;                \
+    } while(0);
+#endif /* CHECK_CBOR */
+
 #endif

src/property/Property.cpp

@@ -142,78 +142,92 @@ void Property::execCallbackOnSync() {
   }
 }
 
-void Property::append(CborEncoder *encoder, bool lightPayload) {
+CborError Property::append(CborEncoder *encoder, bool lightPayload) {
   _lightPayload = lightPayload;
   _attributeIdentifier = 0;
-  appendAttributesToCloudReal(encoder);
+  CHECK_CBOR(appendAttributesToCloudReal(encoder));
   fromLocalToCloud();
   _has_been_updated_once = true;
   _update_requested = false;
   _last_updated_millis = millis();
+  return CborNoError;
 }
 
-void Property::appendAttributeReal(bool value, String attributeName, CborEncoder *encoder) {
-  appendAttributeName(attributeName, [value](CborEncoder & mapEncoder) {
-    cbor_encode_int(&mapEncoder, static_cast<int>(CborIntegerMapKey::BooleanValue));
-    cbor_encode_boolean(&mapEncoder, value);
+CborError Property::appendAttributeReal(bool value, String attributeName, CborEncoder *encoder) {
+  return appendAttributeName(attributeName, [value](CborEncoder & mapEncoder)
+  {
+    CHECK_CBOR(cbor_encode_int(&mapEncoder, static_cast<int>(CborIntegerMapKey::BooleanValue)));
+    CHECK_CBOR(cbor_encode_boolean(&mapEncoder, value));
+    return CborNoError;
   }, encoder);
 }
 
-void Property::appendAttributeReal(int value, String attributeName, CborEncoder *encoder) {
-  appendAttributeName(attributeName, [value](CborEncoder & mapEncoder) {
-    cbor_encode_int(&mapEncoder, static_cast<int>(CborIntegerMapKey::Value));
-    cbor_encode_int(&mapEncoder, value);
+CborError Property::appendAttributeReal(int value, String attributeName, CborEncoder *encoder) {
+  return appendAttributeName(attributeName, [value](CborEncoder & mapEncoder)
+  {
+    CHECK_CBOR(cbor_encode_int(&mapEncoder, static_cast<int>(CborIntegerMapKey::Value)));
+    CHECK_CBOR(cbor_encode_int(&mapEncoder, value));
+    return CborNoError;
   }, encoder);
 }
 
-void Property::appendAttributeReal(float value, String attributeName, CborEncoder *encoder) {
-  appendAttributeName(attributeName, [value](CborEncoder & mapEncoder) {
-    cbor_encode_int(&mapEncoder, static_cast<int>(CborIntegerMapKey::Value));
-    cbor_encode_float(&mapEncoder, value);
+CborError Property::appendAttributeReal(float value, String attributeName, CborEncoder *encoder) {
+  return appendAttributeName(attributeName, [value](CborEncoder & mapEncoder)
+  {
+    CHECK_CBOR(cbor_encode_int(&mapEncoder, static_cast<int>(CborIntegerMapKey::Value)));
+    CHECK_CBOR(cbor_encode_float(&mapEncoder, value));
+    return CborNoError;
   }, encoder);
 }
 
-void Property::appendAttributeReal(String value, String attributeName, CborEncoder *encoder) {
-  appendAttributeName(attributeName, [value](CborEncoder & mapEncoder) {
-    cbor_encode_int(&mapEncoder, static_cast<int>(CborIntegerMapKey::StringValue));
-    cbor_encode_text_stringz(&mapEncoder, value.c_str());
+CborError Property::appendAttributeReal(String value, String attributeName, CborEncoder *encoder) {
+  return appendAttributeName(attributeName, [value](CborEncoder & mapEncoder)
+  {
+    CHECK_CBOR(cbor_encode_int(&mapEncoder, static_cast<int>(CborIntegerMapKey::StringValue)));
+    CHECK_CBOR(cbor_encode_text_stringz(&mapEncoder, value.c_str()));
+    return CborNoError;
   }, encoder);
 }
 
-void Property::appendAttributeName(String attributeName, std::function<void (CborEncoder& mapEncoder)>appendValue, CborEncoder *encoder) {
+CborError Property::appendAttributeName(String attributeName, std::function<CborError (CborEncoder& mapEncoder)>appendValue, CborEncoder *encoder) {
   if (attributeName != "") {
     // when the attribute name string is not empty, the attribute identifier is incremented in order to be encoded in the message if the _lightPayload flag is set
     _attributeIdentifier++;
   }
   CborEncoder mapEncoder;
   unsigned int num_map_properties = _encode_timestamp ? 3 : 2;
-  cbor_encoder_create_map(encoder, &mapEncoder, num_map_properties);
-  cbor_encode_int(&mapEncoder, static_cast<int>(CborIntegerMapKey::Name));
+  CHECK_CBOR(cbor_encoder_create_map(encoder, &mapEncoder, num_map_properties));
+  CHECK_CBOR(cbor_encode_int(&mapEncoder, static_cast<int>(CborIntegerMapKey::Name)));
 
   // if _lightPayload is true, the property and attribute identifiers will be encoded instead of the property name
-  if (_lightPayload) {
+  if (_lightPayload)
+  {
     // the most significant byte of the identifier to be encoded represent the property identifier
     int completeIdentifier = _attributeIdentifier * 256;
     // the least significant byte of the identifier to be encoded represent the attribute identifier
     completeIdentifier += _identifier;
-    cbor_encode_int(&mapEncoder, completeIdentifier);
-  } else {
+    CHECK_CBOR(cbor_encode_int(&mapEncoder, completeIdentifier));
+  }
+  else
+  {
     String completeName = _name;
     if (attributeName != "") {
       completeName += ":" + attributeName;
     }
-    cbor_encode_text_stringz(&mapEncoder, completeName.c_str());
+    CHECK_CBOR(cbor_encode_text_stringz(&mapEncoder, completeName.c_str()));
   }
   /* Encode the value */
-  appendValue(mapEncoder);
+  CHECK_CBOR(appendValue(mapEncoder));
+
   /* Encode the timestamp if that has been required. */
   if(_encode_timestamp)
   {
-    cbor_encode_int (&mapEncoder, static_cast<int>(CborIntegerMapKey::Time));
-    cbor_encode_uint(&mapEncoder, _timestamp);
+    CHECK_CBOR(cbor_encode_int (&mapEncoder, static_cast<int>(CborIntegerMapKey::Time)));
+    CHECK_CBOR(cbor_encode_uint(&mapEncoder, _timestamp));
   }
   /* Close the container */
-  cbor_encoder_close_container(encoder, &mapEncoder);
+  CHECK_CBOR(cbor_encoder_close_container(encoder, &mapEncoder));
+  return CborNoError;
 }
 
 void Property::setAttributesFromCloud(std::list<CborMapData> * map_data_list) {