Allow buffer_dyn to handle more data (ARMmbed#2311)

Buffer fails to handle more than INT16_MAX bytes of data.

To fix the problem:
 -Modify buffer checks to allow handling of 65532 (0xFFFC) bytes of
  data (as we have 16-bit counters).
 -Deny allocation of buffer that have more than 0xFFFF bytes of data.

Author: Arto Kinnunen

source/Core/buffer_dyn.c

@@ -16,7 +16,8 @@
  */
 
 #include "nsconfig.h"
-#include "string.h"
+#include <string.h>
+#include <limits.h>
 #include "ns_types.h"
 #include "nsdynmemLIB.h"
 #include "Core/include/ns_address_internal.h"
@@ -30,6 +31,13 @@
 
 #define TRACE_GROUP "buff"
 
+// Get module working also on 16-bit platform
+#if INT_MAX < 0xFFFF
+#define BUFFER_MAX_SIZE ((size_t)INT_MAX)
+#else
+#define BUFFER_MAX_SIZE ((size_t)0xFFFF)
+#endif
+
 volatile unsigned int buffer_count = 0;
 
 uint8_t *(buffer_corrupt_check)(buffer_t *buf)
@@ -38,15 +46,12 @@ uint8_t *(buffer_corrupt_check)(buffer_t *buf)
         return NULL;
     }
 
-    if (buf->buf_ptr > buf->buf_end) {
-        tr_error("Buffer pointer end not set");
-    } else if (buffer_data_length(buf) < 0) {
-        tr_error("Buffer length overflow");
-        while (1);
-    } else if (buf->buf_end > buf->size || buf->buf_ptr > buf->size) {
-        tr_error("buffer pointer overridden");
+    if (buf->buf_ptr > buf->buf_end || buf->buf_end > buf->size) {
+        tr_error("Invalid buffer, size=%"PRIu16", buf_ptr=%"PRIu16", buf_end=%"PRIu16"", buf->size, buf->buf_ptr, buf->buf_end);
+        tr_error("Data: %s", tr_array(buffer_data_pointer(buf), 56));
         while (1);
     }
+
     return buffer_data_pointer(buf);
 }
 
@@ -71,8 +76,8 @@ buffer_t *buffer_get_minimal(uint16_t size)
  */
 buffer_t *buffer_get_specific(uint16_t headroom, uint16_t size, uint16_t minspace)
 {
-    buffer_t *buf;
-    uint16_t total_size;
+    buffer_t *buf = NULL;
+    uint32_t total_size;
 
     total_size = headroom + size;
     if (total_size < minspace) {
@@ -83,10 +88,12 @@ buffer_t *buffer_get_specific(uint16_t headroom, uint16_t size, uint16_t minspac
      * anyway be this much aligned. */
     total_size = (total_size + 3) & ~ 3;
 
-    // Note - as well as this alloc+init, buffers can also be "realloced"
-    // in buffer_headroom()
+    if (total_size <= BUFFER_MAX_SIZE) {
+        // Note - as well as this alloc+init, buffers can also be "realloced"
+        // in buffer_headroom()
+        buf = ns_dyn_mem_temporary_alloc(sizeof(buffer_t) + total_size);
+    }
 
-    buf = ns_dyn_mem_temporary_alloc(sizeof(buffer_t) + total_size);
     if (buf) {
         platform_enter_critical();
         buffer_count++;
@@ -110,7 +117,7 @@ buffer_t *buffer_get_specific(uint16_t headroom, uint16_t size, uint16_t minspac
 #endif
         buf->size = total_size;
     } else {
-        tr_error("buffer_get failed: alloc(%d)", (int) sizeof(buffer_t) + total_size);
+        tr_error("buffer_get failed: alloc(%"PRIu32")", (uint32_t)(sizeof(buffer_t) + total_size));
     }
 
     protocol_stats_update(STATS_BUFFER_ALLOC, 1);
@@ -130,10 +137,14 @@ buffer_t *buffer_headroom(buffer_t *buf, uint16_t size)
     uint16_t curr_len = buffer_data_length(buf);
 
     if (buf->size < (curr_len + size)) {
+        buffer_t *restrict new_buf = NULL;
         /* This buffer isn't big enough at all - allocate a new block */
         // TODO - should we be giving them extra? probably
-        uint16_t new_total = (curr_len + size + 3) & ~ 3;
-        buffer_t *restrict new_buf = ns_dyn_mem_temporary_alloc(sizeof(buffer_t) + new_total);
+        uint32_t new_total = (curr_len + size + 3) & ~ 3;
+        if (new_total <= BUFFER_MAX_SIZE) {
+            new_buf = ns_dyn_mem_temporary_alloc(sizeof(buffer_t) + new_total);
+        }
+
         if (new_buf) {
             // Copy the buffer_t header
             *new_buf = *buf;

source/Core/include/ns_buffer.h

@@ -322,7 +322,7 @@ struct socket *buffer_socket_set(buffer_t *buf, struct socket *socket);
         } while(0)
 
 /** get data length*/
-#define buffer_data_length(x)  (int16_t)(x->buf_end - x->buf_ptr)
+#define buffer_data_length(x)  (int)(x->buf_end - x->buf_ptr)
 
 /** get data length Set*/
 #define buffer_data_length_set(x,z)  ((x)->buf_end = (x)->buf_ptr + (z))

test/nanostack/unittest/Core/buffer_dyn/buffer_dyntest.cpp

@@ -31,3 +31,7 @@ TEST(buffer_dyn, test_buffer_get)
     CHECK(test_buffer_get());
 }
 
+TEST(buffer_dyn, test_buffer_get_minimal)
+{
+    CHECK(test_buffer_get_minimal());
+}

test/nanostack/unittest/Core/buffer_dyn/test_buffer_dyn.c

@@ -14,15 +14,151 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-#include "test_buffer_dyn.h"
+
+#include "nsconfig.h"
 #include <string.h>
+#include "ns_types.h"
+#include "ns_buffer.h"
+#include "test_buffer_dyn.h"
+#include "nsdynmemLIB_stub.h"
 
+#define FILL_MARK 0x05
+bool test_buffer_get_and_data_add(uint16_t size, bool use_minimal);
+
+
+bool test_buffer_get_minimal_with_size(uint16_t size)
+{
+    return test_buffer_get_and_data_add(size, true);
+}
+
+bool test_buffer_get_with_size(uint16_t size)
+{
+    return test_buffer_get_and_data_add(size, false);
+}
+
+bool test_buffer_get_and_data_add(uint16_t size, bool use_minimal)
+{
+    buffer_t *buf;
+    uint8_t *data_ptr;
+    uint16_t data_len;
+
+    nsdynmemlib_stub.returnCounter = 1;
+
+    data_len = size;
+
+    if (use_minimal) {
+        buf = buffer_get_minimal(data_len);
+    } else {
+        buf = buffer_get(data_len);
+    }
+    if (!buf) {
+        return false;
+    }
+
+    // Add data to buffer
+    data_ptr = malloc(data_len);
+    memset(data_ptr, FILL_MARK, data_len);
+    buffer_data_add(buf, data_ptr, data_len);
+
+    // validate content
+    if (memcmp(buffer_data_pointer(buf), data_ptr, data_len) != 0) {
+        return false;
+    }
+
+    // validate buffer length
+    if (buffer_data_length(buf) != data_len) {
+        return false;
+    }
+
+    buffer_free(buf);
+    free(data_ptr);
+
+    return true;
+}
 
 bool test_buffer_get()
 {
-    if (buffer_get(1)) {
+    buffer_t *buf;
+    uint8_t *data_ptr;
+    uint16_t data_len;
+
+    // memory allocation failure
+    buf = buffer_get(1);
+    if (buf) {
+        return false;
+    }
+
+    // Test with (0xffff - BUFFER_DEFAULT_HEADROOM) aligned)
+    uint16_t len = (0xffff - BUFFER_DEFAULT_HEADROOM - 4) + 3 & ~ 3;
+    if (test_buffer_get_with_size(len) == false) {
         return false;
     }
+
+    len = len + 1;
+    if (test_buffer_get_with_size(len) == true) {
+        return false;
+    }
+
+    // Test with 32767
+    if (test_buffer_get_with_size(0x7fff) == false) {
+        return false;
+    }
+
+    // Test with 32768
+    if (test_buffer_get_with_size(0x7fff + 1) == false) {
+        return false;
+    }
+
+    // test with 0-256
+    for (int i = 0; i <= 256; i++) {
+        if (test_buffer_get_with_size(i) == false) {
+            return false;
+        }
+    }
+
     return true;
 }
 
+bool test_buffer_get_minimal()
+{
+    buffer_t *buf;
+    uint8_t *data_ptr;
+    uint16_t data_len;
+
+    // memory allocation failure
+    buf = buffer_get_minimal(1);
+    if (buf) {
+        return false;
+    }
+
+    // Test with (0xffff - 4) aligned)
+    uint16_t len = (0xffff - 4) + 3 & ~ 3;
+    if (test_buffer_get_minimal_with_size(len) == false) {
+        return false;
+    }
+
+    // Test with too long size, fails
+    len = len + 1;
+    if (test_buffer_get_minimal_with_size(len) == true) {
+        return false;
+    }
+
+    // Test with 32767
+    if (test_buffer_get_minimal_with_size(0x7fff) == false) {
+        return false;
+    }
+
+    // Test with 32768
+    if (test_buffer_get_minimal_with_size(0x7fff + 1) == false) {
+        return false;
+    }
+
+    // test with 0-256
+    for (int i = 0; i <= 256; i++) {
+        if (test_buffer_get_minimal_with_size(i) == false) {
+            return false;
+        }
+    }
+
+    return true;
+}

test/nanostack/unittest/Core/buffer_dyn/test_buffer_dyn.h

@@ -25,6 +25,8 @@ extern "C" {
 
 bool test_buffer_get();
 
+bool test_buffer_get_minimal();
+
 #ifdef __cplusplus
 }
 #endif