From 62f44e45df111fcfcc68e9a56bbfb562d2246154 Mon Sep 17 00:00:00 2001
From: Piyush Shah <piyush@espressif.com>
Date: Fri, 17 Nov 2017 15:24:17 +0530
Subject: [PATCH] ringbuf: Bugfixes for managing arbitrary sizes of ring buffer

It was observed that if the ring buffer size provided by application
is not a multiple of 4, some checks were failing (as read_ptr and write_ptr
could shoot beyond the ring buffer boundary), thereby causing asserts.
Simply wrapping around the pointers for such cases fixes the issue.

Moreover, because of the logic for maintaining 4-byte boundary,
it was also possible that a wrap-around occurred for some data,
even when the actual size remaining was sufficient for it.
Eg. Buffer available: 34, data size: 34, 4-byte aligned size: 36
Since the logic compares against 36, it writes 34 bytes and does a
wraparound. But since all 34 bytes are already written, there is
nothing to write after wrapping. It is better to just re-set the
write pointer to the dtart of ring buffer in such cases.

Tested send/receive for various arbitrary sizes of data and for
arbitrary sizes of ring buffer.

Alternative Solutions:
1) Allow only sizes which are multiples of 4, and return error otherwise.
Appropriate code and documentation change would be required
2) Allow arbitrary sizes, but internally add upto 3 bytes to make
the total size a multiple of 4

Signed-off-by: Piyush Shah <piyush@espressif.com>
---
 components/freertos/ringbuf.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/components/freertos/ringbuf.c b/components/freertos/ringbuf.c
index ec4322df87..d7ee790a5e 100644
--- a/components/freertos/ringbuf.c
+++ b/components/freertos/ringbuf.c
@@ -190,8 +190,20 @@ static BaseType_t copyItemToRingbufAllowSplit(ringbuf_t *rb, uint8_t *buffer, si
             memcpy(rb->write_ptr, buffer, rem_len);
             //Update vars so the code later on will write the rest of the data.
             buffer+=rem_len;
-            rbuffer_size-=rem_len;
             buffer_size-=rem_len;
+            //Re-adjust the rbuffer value to be 4 byte aligned
+            rbuffer_size=(buffer_size+3)&~3;
+            //It is possible that we are here because we checked for 4byte aligned
+            //size, but actual data was smaller.
+            //Eg. For buffer_size = 34, rbuffer_size will be 36. Suppose we had only
+            //42 bytes of memory available, the top level check will fail, as it will
+            //check for availability of 36 + 8 = 44 bytes.
+            //However, the 42 bytes available memory is sufficient for 34 + 8 bytes data
+            //and so, we can return after writing the data. Hence, this check
+            if (buffer_size == 0) {
+                rb->write_ptr=rb->data;
+                return pdTRUE;
+            }
         } else {
             //Huh, only the header fit. Mark as dummy so the receive function doesn't receive
             //an useless zero-byte packet.
@@ -286,7 +298,10 @@ static uint8_t *getItemFromRingbufDefault(ringbuf_t *rb, size_t *length, int wan
     //...and move the read pointer past the data.
     rb->read_ptr+=sizeof(buf_entry_hdr_t)+((hdr->len+3)&~3);
     //The buffer will wrap around if we don't have room for a header anymore.
-    if ((rb->data + rb->size) - rb->read_ptr < sizeof(buf_entry_hdr_t)) {
+    //Integer typecasting is used because the first operand can result into a -ve
+    //value for cases wherein the ringbuffer size is not a multiple of 4, but the
+    //implementation logic aligns read_ptr to 4-byte boundary
+    if ((int)((rb->data + rb->size) - rb->read_ptr) < (int)sizeof(buf_entry_hdr_t)) {
         rb->read_ptr=rb->data;
     }
     return ret;
@@ -355,12 +370,20 @@ static void returnItemToRingbufDefault(ringbuf_t *rb, void *item) {
             rb->free_ptr=rb->data;
         } else {
             //Skip past item
+            rb->free_ptr+=sizeof(buf_entry_hdr_t);
+            //Check if the free_ptr overshoots the buffer.
+            //Checking this before aligning free_ptr since it is possible that alignment
+            //will cause pointer to overshoot, if the ringbuf size is not a multiple of 4
+            configASSERT(rb->free_ptr+hdr->len<=rb->data+rb->size);
+            //Align free_ptr to 4 byte boundary. Overshoot condition will result in wrap around below
             size_t len=(hdr->len+3)&~3;
-            rb->free_ptr+=len+sizeof(buf_entry_hdr_t);
-            configASSERT(rb->free_ptr<=rb->data+rb->size);
+            rb->free_ptr+=len;
         }
         //The buffer will wrap around if we don't have room for a header anymore.
-        if ((rb->data+rb->size)-rb->free_ptr < sizeof(buf_entry_hdr_t)) {
+        //Integer typecasting is used because the first operand can result into a -ve
+        //value for cases wherein the ringbuffer size is not a multiple of 4, but the
+        //implementation logic aligns free_ptr to 4-byte boundary
+        if ((int)((rb->data+rb->size)-rb->free_ptr) < (int)sizeof(buf_entry_hdr_t)) {
             rb->free_ptr=rb->data;
         }
         //The free_ptr can not exceed read_ptr, otherwise write_ptr might overwrite read_ptr.