[cogl] Don't dereference an unitialised pointer in _cogl_container_of

[Neil Roberts <nroberts src gnome org>]

commit 1efed1e0a2bce706eb4901979ed4e717bb13e4e2
Author: Neil Roberts <neil linux intel com>
Date:   Mon Feb 3 20:14:48 2014 +0000

    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>

 cogl/cogl-fence.c                           |    4 ++-
 cogl/cogl-gles2-context.c                   |    2 +-
 cogl/cogl-list.h                            |   48 +++++++++++++++------------
 cogl/cogl-memory-stack.c                    |   16 +++++---
 cogl/cogl-onscreen.c                        |    4 ++-
 cogl/driver/gl/cogl-pipeline-fragend-glsl.c |    4 +-
 6 files changed, 46 insertions(+), 32 deletions(-)
---
diff --git a/cogl/cogl-fence.c b/cogl/cogl-fence.c
index 26d0501..eef7e43 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 d1cbaae..c654313 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 7d716a8..20cbec8 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 6a4dda6..acb8cf2 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 b884def..3ccaa59 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 9ff5625..29cea56 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;