From d68cbb298012314c41403d1f336392cd300766d7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Moritz=20=27Morty=27=20Str=C3=BCbe?= <morty@gmx.net>
Date: Fri, 28 Nov 2014 12:22:24 +0100
Subject: [PATCH] Ensure that the data in ringbuff is accessed in the right
 order

---
 core/lib/ringbuf.c | 25 +++++++++++++++++++++----
 core/sys/cc.h      | 11 +++++++++++
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/core/lib/ringbuf.c b/core/lib/ringbuf.c
index a10127c17..89bcad514 100644
--- a/core/lib/ringbuf.c
+++ b/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;
diff --git a/core/sys/cc.h b/core/sys/cc.h
index 06b8889ec..f105a78c3 100644
--- a/core/sys/cc.h
+++ b/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 */