Don't dereference an unitialised pointer in _cogl_container_of

The previous implementation was dereferencing the sample pointer in
order to get the offset to subtract from the member pointer. The
resulting value is then only used to get a pointer to the member in
order to calculate the offset so it doesn't actually read from the
memory location and shouldn't cause any problems. However this is
probably technically invalid and could have undefined behaviour. It
looks like clang takes advantage of this undefined behaviour and
doesn't actually offset the pointer. It also generates a warning when
it does this.

This patch splits the _cogl_container_of macro into two
implementations. Previously the macro was always used in the list
iterator macros like this:

SomeType *sample = _cogl_container_of(list_node, sample, link)

Instead of doing that there is now a new macro called
_cogl_list_set_iterator which explicitly assigns to the sample pointer
with an initial value before assigning to it again with the real
offset. This redundant initialisation gets optimised out by compiler.

The second macro is still called _cogl_container_of but instead of
taking a sample pointer it just directly takes the type name. That way
it can use the standard offsetof macro.

https://bugzilla.gnome.org/show_bug.cgi?id=723530

Reviewed-by: Robert Bragg <robert@linux.intel.com>

6 files changed, 46 insertions, 32 deletions

diff --git a/cogl/cogl-fence.c b/cogl/cogl-fence.c
index 26d05014..eef7e436 100644
--- a/cogl/cogl-fence.c
+++ b/cogl/cogl-fence.c
@@ -222,7 +222,9 @@ _cogl_fence_cancel_fences_for_framebuffer (CoglFramebuffer *framebuffer)
 
   while (!_cogl_list_empty (&journal->pending_fences))
     {
-      fence = _cogl_container_of (journal->pending_fences.next, fence, link);
+      fence = _cogl_container_of (journal->pending_fences.next,
+                                  CoglFenceClosure,
+                                  link);
       cogl_framebuffer_cancel_fence_callback (framebuffer, fence);
     }
 
diff --git a/cogl/cogl-gles2-context.c b/cogl/cogl-gles2-context.c
index d1cbaae1..c654313a 100644
--- a/cogl/cogl-gles2-context.c
+++ b/cogl/cogl-gles2-context.c
@@ -1521,7 +1521,7 @@ _cogl_gles2_context_free (CoglGLES2Context *gles2_context)
     {
       CoglGLES2Offscreen *gles2_offscreen =
         _cogl_container_of (gles2_context->foreign_offscreens.next,
-                            gles2_offscreen,
+                            CoglGLES2Offscreen,
                             link);
 
       /* Note: this will also indirectly free the gles2_offscreen by
diff --git a/cogl/cogl-list.h b/cogl/cogl-list.h
index 7d716a89..20cbec82 100644
--- a/cogl/cogl-list.h
+++ b/cogl/cogl-list.h
@@ -26,6 +26,8 @@
 #ifndef COGL_LIST_H
 #define COGL_LIST_H
 
+#include <stddef.h>
+
 /**
  * CoglList - linked list
  *
@@ -84,40 +86,44 @@ void
 _cogl_list_insert_list (CoglList *list,
                         CoglList *other);
 
-#ifdef __GNUC__
-#define _cogl_container_of(ptr, sample, member)                         \
-  (__typeof__(sample))((char *)(ptr)    -                               \
-                       ((char *)&(sample)->member - (char *)(sample)))
-#else
-#define _cogl_container_of(ptr, sample, member)                 \
-  (void *)((char *)(ptr)        -                               \
-           ((char *)&(sample)->member - (char *)(sample)))
-#endif
+/* This assigns to iterator first so that taking a reference to it
+ * later in the second step won't be an undefined operation. It
+ * assigns the value of list_node rather than 0 so that it is possible
+ * have list_node be based on the previous value of iterator. In that
+ * respect iterator is just used as a convenient temporary variable.
+ * The compiler optimises all of this down to a single subtraction by
+ * a constant */
+#define _cogl_list_set_iterator(list_node, iterator, member)    \
+  ((iterator) = (void *) (list_node),                           \
+   (iterator) = (void *) ((char *) (iterator) -                 \
+                          (((char *) &(iterator)->member) -     \
+                           (char *) (iterator))))
+
+#define _cogl_container_of(ptr, type, member)           \
+  (type *) ((char *) (ptr) - offsetof (type, member))
 
 #define _cogl_list_for_each(pos, head, member)                          \
-  for (pos = 0, pos = _cogl_container_of((head)->next, pos, member);    \
+  for (_cogl_list_set_iterator ((head)->next, pos, member);             \
        &pos->member != (head);                                          \
-       pos = _cogl_container_of(pos->member.next, pos, member))
+       _cogl_list_set_iterator (pos->member.next, pos, member))
 
 #define _cogl_list_for_each_safe(pos, tmp, head, member)                \
-  for (pos = 0, tmp = 0,                                                \
-         pos = _cogl_container_of((head)->next, pos, member),           \
-         tmp = _cogl_container_of((pos)->member.next, tmp, member);     \
+  for (_cogl_list_set_iterator ((head)->next, pos, member),             \
+         _cogl_list_set_iterator ((pos)->member.next, tmp, member);     \
        &pos->member != (head);                                          \
        pos = tmp,                                                       \
-         tmp = _cogl_container_of(pos->member.next, tmp, member))
+         _cogl_list_set_iterator (pos->member.next, tmp, member))
 
 #define _cogl_list_for_each_reverse(pos, head, member)                  \
-  for (pos = 0, pos = _cogl_container_of((head)->prev, pos, member);    \
+  for (_cogl_list_set_iterator ((head)->prev, pos, member);             \
        &pos->member != (head);                                          \
-       pos = _cogl_container_of(pos->member.prev, pos, member))
+       _cogl_list_set_iterator (pos->member.prev, pos, member))
 
 #define _cogl_list_for_each_reverse_safe(pos, tmp, head, member)        \
-  for (pos = 0, tmp = 0,                                                \
-         pos = _cogl_container_of((head)->prev, pos, member),           \
-         tmp = _cogl_container_of((pos)->member.prev, tmp, member);     \
+  for (_cogl_list_set_iterator ((head)->prev, pos, member),             \
+         _cogl_list_set_iterator ((pos)->member.prev, tmp, member);     \
        &pos->member != (head);                                          \
        pos = tmp,                                                       \
-         tmp = _cogl_container_of(pos->member.prev, tmp, member))
+         _cogl_list_set_iterator (pos->member.prev, tmp, member))
 
 #endif /* COGL_LIST_H */
diff --git a/cogl/cogl-memory-stack.c b/cogl/cogl-memory-stack.c
index 6a4dda66..acb8cf28 100644
--- a/cogl/cogl-memory-stack.c
+++ b/cogl/cogl-memory-stack.c
@@ -130,9 +130,9 @@ _cogl_memory_stack_alloc (CoglMemoryStack *stack, size_t bytes)
    * is made then we may need to skip over one or more of the
    * sub-stacks that are too small for the requested allocation
    * size... */
-  for (sub_stack = _cogl_container_of (sub_stack->link.next, sub_stack, link);
+  for (_cogl_list_set_iterator (sub_stack->link.next, sub_stack, link);
        &sub_stack->link != &stack->sub_stacks;
-       sub_stack = _cogl_container_of (sub_stack->link.next, sub_stack, link))
+       _cogl_list_set_iterator (sub_stack->link.next, sub_stack, link))
     {
       if (sub_stack->bytes >= bytes)
         {
@@ -149,11 +149,15 @@ _cogl_memory_stack_alloc (CoglMemoryStack *stack, size_t bytes)
    * requested allocation if that's bigger.
    */
 
-  sub_stack = _cogl_container_of (stack->sub_stacks.prev, sub_stack, link);
+  sub_stack = _cogl_container_of (stack->sub_stacks.prev,
+                                  CoglMemorySubStack,
+                                  link);
 
   _cogl_memory_stack_add_sub_stack (stack, MAX (sub_stack->bytes, bytes) * 2);
 
-  sub_stack = _cogl_container_of (stack->sub_stacks.prev, sub_stack, link);
+  sub_stack = _cogl_container_of (stack->sub_stacks.prev,
+                                  CoglMemorySubStack,
+                                  link);
 
   stack->sub_stack_offset += bytes;
 
@@ -164,7 +168,7 @@ void
 _cogl_memory_stack_rewind (CoglMemoryStack *stack)
 {
   stack->sub_stack = _cogl_container_of (stack->sub_stacks.next,
-                                         stack->sub_stack,
+                                         CoglMemorySubStack,
                                          link);
   stack->sub_stack_offset = 0;
 }
@@ -183,7 +187,7 @@ _cogl_memory_stack_free (CoglMemoryStack *stack)
   while (!_cogl_list_empty (&stack->sub_stacks))
     {
       CoglMemorySubStack *sub_stack =
-        _cogl_container_of (stack->sub_stacks.next, sub_stack, link);
+        _cogl_container_of (stack->sub_stacks.next, CoglMemorySubStack, link);
       _cogl_list_remove (&sub_stack->link);
       _cogl_memory_sub_stack_free (sub_stack);
     }
diff --git a/cogl/cogl-onscreen.c b/cogl/cogl-onscreen.c
index b884def9..3ccaa59a 100644
--- a/cogl/cogl-onscreen.c
+++ b/cogl/cogl-onscreen.c
@@ -156,7 +156,9 @@ _cogl_dispatch_onscreen_cb (CoglContext *context)
   while (!_cogl_list_empty (&context->onscreen_dirty_queue))
     {
       CoglOnscreenQueuedDirty *qe =
-        _cogl_container_of (context->onscreen_dirty_queue.next, qe, link);
+        _cogl_container_of (context->onscreen_dirty_queue.next,
+                            CoglOnscreenQueuedDirty,
+                            link);
 
       _cogl_list_remove (&qe->link);
 
diff --git a/cogl/driver/gl/cogl-pipeline-fragend-glsl.c b/cogl/driver/gl/cogl-pipeline-fragend-glsl.c
index 9ff56258..29cea56c 100644
--- a/cogl/driver/gl/cogl-pipeline-fragend-glsl.c
+++ b/cogl/driver/gl/cogl-pipeline-fragend-glsl.c
@@ -902,7 +902,7 @@ _cogl_pipeline_fragend_glsl_add_layer (CoglPipeline *pipeline,
   else
     {
       LayerData *first =
-        _cogl_container_of (shader_state->layers.next, first, link);
+        _cogl_container_of (shader_state->layers.next, LayerData, link);
       layer_data->previous_layer_index = first->layer->index;
     }
 
@@ -1011,7 +1011,7 @@ _cogl_pipeline_fragend_glsl_end (CoglPipeline *pipeline,
           LayerData *layer_data, *tmp;
 
           layer_data = _cogl_container_of (shader_state->layers.next,
-                                           layer_data,
+                                           LayerData,
                                            link);
           last_layer = layer_data->layer;