[PATCH] Don't deref the sample pointer in the wl_container_of macro

[Kristian Høgsberg]

On Tue, Feb 04, 2014 at 02:21:48PM +0000, Neil Roberts wrote:
> The previous implementation of the wl_container_of macro was
> dereferencing the sample pointer in order to get an address of the
> member to calculate the offset. Ideally this shouldn't cause any
> problems because the dereference doesn't actually cause the address to
> be read from so it shouldn't matter if the pointer is uninitialised.
> However this is probably technically invalid and could cause undefined
> behavior. Clang appears to take advantage of this undefined behavior
> and doesn't bother doing the subtraction. It also gives a warning when
> it does this.
> 
> The documentation for wl_container_of implies that it should only be
> given an initialised pointer and if that is done then there is no
> problem with clang. However this is quite easy to forget and doesn't
> cause any problems or warnings with gcc so it's quite easy to
> accidentally break clang.
> 
> To fix the problem this changes the macro to use pointer -
> offsetof(__typeof__(sample), member) so that it doesn't need to deref
> the sample pointer. This does however require that the __typeof__
> operator is supported by the compiler. In practice we probably only
> care about gcc and clang and both of these happily support the
> operator.
> 
> The previous implementation was also using __typeof__ but it had a
> fallback path avoiding it when the operator isn't available. The
> fallback effectively has undefined behaviour and it is targetting
> unknown compilers so it is probably not a good idea to leave it in.
> Instead, this patch just removes it. If someone finds a compiler that
> doesn't have __typeof__ but does work with the old implementation then
> maybe they could add it back in as a special case.
> 
> This patch removes the initialisation anywhere where the sample
> pointer was being unitialised before using wl_container_of. The
> documentation for the macro has also been updated to specify that this
> is OK.

Thanks, that's nice.  Patch applied.

Kristian
> ---
>  src/wayland-server.c |  2 +-
>  src/wayland-server.h |  2 +-
>  src/wayland-util.h   | 30 +++++++++++++-----------------
>  3 files changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 489b99d..294497d 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -571,7 +571,7 @@ wl_resource_get_link(struct wl_resource *resource)
>  WL_EXPORT struct wl_resource *
>  wl_resource_from_link(struct wl_list *link)
>  {
> -	struct wl_resource *resource = NULL;
> +	struct wl_resource *resource;
>  
>  	return wl_container_of(link, resource, link);
>  }
> diff --git a/src/wayland-server.h b/src/wayland-server.h
> index 3fcaaf6..7fc5b47 100644
> --- a/src/wayland-server.h
> +++ b/src/wayland-server.h
> @@ -162,7 +162,7 @@ wl_client_post_no_memory(struct wl_client *client);
>   * \code
>   * void your_listener(struct wl_listener *listener, void *data)
>   * {
> - * 	struct your_data *data = NULL;
> + * 	struct your_data *data;
>   *
>   * 	your_data = wl_container_of(listener, data, your_member_name);
>   * }
> diff --git a/src/wayland-util.h b/src/wayland-util.h
> index 68d91e2..57764d9 100644
> --- a/src/wayland-util.h
> +++ b/src/wayland-util.h
> @@ -135,7 +135,7 @@ void wl_list_insert_list(struct wl_list *list, struct wl_list *other);
>   *
>   * void example_container_destroy(struct wl_listener *listener, void *data)
>   * {
> - *     struct example_container *ctr = NULL;
> + *     struct example_container *ctr;
>   *
>   *     ctr = wl_container_of(listener, ctr, destroy_listener);
>   *     \comment{destroy ctr...}
> @@ -144,44 +144,40 @@ void wl_list_insert_list(struct wl_list *list, struct wl_list *other);
>   *
>   * \param ptr A valid pointer to the contained item.
>   *
> - * \param sample A pointer to the type of content that the list item stores.
> - * Sample does not need be a valid pointer; a null pointer will suffice.
> + * \param sample A pointer to the type of content that the list item
> + * stores. Sample does not need be a valid pointer; a null or
> + * an uninitialised pointer will suffice.
>   *
>   * \param member The named location of ptr within the sample type.
>   *
>   * \return The container for the specified pointer.
>   */
> -#ifdef __GNUC__
>  #define wl_container_of(ptr, sample, member)				\
> -	(__typeof__(sample))((char *)(ptr)	-			\
> -		 ((char *)&(sample)->member - (char *)(sample)))
> -#else
> -#define wl_container_of(ptr, sample, member)				\
> -	(void *)((char *)(ptr)	-				        \
> -		 ((char *)&(sample)->member - (char *)(sample)))
> -#endif
> +	(__typeof__(sample))((char *)(ptr) -				\
> +			     offsetof(__typeof__(*sample), member))
> +/* If the above macro causes problems on your compiler you might be
> + * able to find an alternative name for the non-standard __typeof__
> + * operator and add a special case here */
>  
>  #define wl_list_for_each(pos, head, member)				\
> -	for (pos = 0, pos = wl_container_of((head)->next, pos, member);	\
> +	for (pos = wl_container_of((head)->next, pos, member);	\
>  	     &pos->member != (head);					\
>  	     pos = wl_container_of(pos->member.next, pos, member))
>  
>  #define wl_list_for_each_safe(pos, tmp, head, member)			\
> -	for (pos = 0, tmp = 0, 						\
> -	     pos = wl_container_of((head)->next, pos, member),		\
> +	for (pos = wl_container_of((head)->next, pos, member),		\
>  	     tmp = wl_container_of((pos)->member.next, tmp, member);	\
>  	     &pos->member != (head);					\
>  	     pos = tmp,							\
>  	     tmp = wl_container_of(pos->member.next, tmp, member))
>  
>  #define wl_list_for_each_reverse(pos, head, member)			\
> -	for (pos = 0, pos = wl_container_of((head)->prev, pos, member);	\
> +	for (pos = wl_container_of((head)->prev, pos, member);	\
>  	     &pos->member != (head);					\
>  	     pos = wl_container_of(pos->member.prev, pos, member))
>  
>  #define wl_list_for_each_reverse_safe(pos, tmp, head, member)		\
> -	for (pos = 0, tmp = 0, 						\
> -	     pos = wl_container_of((head)->prev, pos, member),	\
> +	for (pos = wl_container_of((head)->prev, pos, member),	\
>  	     tmp = wl_container_of((pos)->member.prev, tmp, member);	\
>  	     &pos->member != (head);					\
>  	     pos = tmp,							\
> -- 
> 1.8.4.2
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel