Merge pull request #884 from cmorty/pull/ringbuf-order

Ensure that the data in ringbuff is accessed in the right order

core/lib/ringbuf.c

@@ -38,6 +38,7 @@
  */
 
 #include "lib/ringbuf.h"
+#include <sys/cc.h>
 /*---------------------------------------------------------------------------*/
 void
 ringbuf_init(struct ringbuf *r, uint8_t *dataptr, uint8_t size)
@@ -63,8 +64,15 @@ ringbuf_put(struct ringbuf *r, uint8_t c)
   if(((r->put_ptr - r->get_ptr) & r->mask) == r->mask) {
     return 0;
   }
-  r->data[r->put_ptr] = c;
+  /*
-  r->put_ptr = (r->put_ptr + 1) & r->mask;
+   * CC_ACCESS_NOW is used because the compiler is allowed to reorder
+   * the access to non-volatile variables.
+   * In this case a reader might read from the moved index/ptr before
+   * its value (c) is written. Reordering makes little sense, but
+   * better safe than sorry.
+   */
+  CC_ACCESS_NOW(uint8_t, r->data[r->put_ptr]) = c;
+  CC_ACCESS_NOW(uint8_t, r->put_ptr) = (r->put_ptr + 1) & r->mask;
   return 1;
 }
 /*---------------------------------------------------------------------------*/
@@ -84,8 +92,17 @@ ringbuf_get(struct ringbuf *r)
      most platforms, but C does not guarantee this.
   */
   if(((r->put_ptr - r->get_ptr) & r->mask) > 0) {
-    c = r->data[r->get_ptr];
+    /*
-    r->get_ptr = (r->get_ptr + 1) & r->mask;
+     * CC_ACCESS_NOW is used because the compiler is allowed to reorder
+     * the access to non-volatile variables.
+     * In this case the memory might be freed and overwritten by
+     * increasing get_ptr before the value was copied to c.
+     * Opposed to the put-operation this would even make sense,
+     * because the register used for mask can be reused to save c
+     * (on some architectures).
+     */
+    c = CC_ACCESS_NOW(uint8_t, r->data[r->get_ptr]);
+    CC_ACCESS_NOW(uint8_t, r->get_ptr) = (r->get_ptr + 1) & r->mask;
     return c;
   } else {
     return -1;

core/sys/cc.h

@@ -123,6 +123,17 @@
 #define CC_NO_VA_ARGS CC_CONF_VA_ARGS
 #endif
 
+/** \def CC_ACCESS_NOW(x)
+ * This macro ensures that the access to a non-volatile variable can
+ * not be reordered or optimized by the compiler.
+ * See also https://lwn.net/Articles/508991/ - In Linux the macro is
+ * called ACCESS_ONCE
+ * The type must be passed, because the typeof-operator is a gcc
+ * extension
+ */
+
+#define CC_ACCESS_NOW(type, variable) (*(volatile type *)&(variable))
+
 #ifndef NULL
 #define NULL 0
 #endif /* NULL */