[Mesa-dev] question about container_of

[Kristian H. Kristensen]

Emil Velikov <emil.l.velikov at gmail.com> writes:

> On 18 April 2017 at 13:55, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>> On Mon, 27 Feb 2017 13:26:11 +0000
>> Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>
>>> Hi Julien,
>>>
>>> On 27 February 2017 at 12:08, Julien Isorce <julien.isorce at gmail.com> wrote:
>>> > Hi,
>>> >
>>> > Since 2012 commit ccff74971203b533bf16b46b49a9e61753f75e6c it is said:
>>> > "sample must be initialized, or else the result is undefined" in the
>>> > description of mesa/src/util/list.h::container_of .
>>> >
>>> > But I can find a few places where it is used without initializing that
>>> > second parameter, i.e. like:
>>> >
>>> > struct A a;
>>> > container_of(ptr, a, member);
>>> >
>>> > Then I can add the "= NULL" but should it be just
>>> > container_of(ptr, struct A, member);
>>> > like in the kernel and some other places in mesa ?
>>> >
>>> Strictly peaking these are toolchain (ASAN iirc) bugs, since there is
>>> no pointer deref, as we're doing pointer arithmetic.
>>
>> Hi Emil,
>>
>> that's what people would usually think. It used to work with GCC. Then
>> came Clang.
>>
>>> Afaict the general decision was to to merge the patch(es) since they
>>> will make actual bugs stand out amongst the noise. In the long run,
>>> it's better to fix the tool (ASAN/other) than trying to "fix" all the
>>> cases in mesa and dozens of other projects. But until then patches are
>>> safe enough ;-)
>>>
>>> That's my take on it, at least.
>>
>> It depends on how container_of() has been defined. For more details,
>> see e.g.:
>> https://cgit.freedesktop.org/wayland/wayland/commit/?id=a18e34417ba3fefeb81d891235e8ebf394a20a74
>>
>> The comment in the code in Mesa is correct for the fallback
>> implementation it has. Maybe things rely on the fallback implementation
>> never being used when compiling with Clang.
>>
>> Julien, some alternatives for container_of use a pointer, others expect
>> a type instead. I believe one must use the correct form.
>>
> Thanks for the correction Pekka - yes, the issue (to deref or not) is
> implementation specific.
> A 'minor' detail that I've missed.

The issue isn't whether it derefs the pointer or not. The undefined
version looks like this:

 #define wl_container_of(ptr, sample, member)
	(__typeof__(sample))((char *)(ptr)	-
		 ((char *)&(sample)->member - (char *)(sample)))

and the problem is that it expects an uninitialized variable (sample) to
have consistent values across two references ( (char *)&(sample)->member and
(char *)sample ), but referencing an uninitialized variable even once is
undefined behavior.

Kristian

> Seems like we use the more prone one - having the second argument
> being a pointer as, opposed to a type.
> Perhaps we should reconsider and update mesa, sooner rather than later.
>
> Thanks again!
> Emil
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev